Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

watson: 1.5.2 -> 1.7.0 #63972

Merged
merged 3 commits into from Jul 1, 2019
Merged

watson: 1.5.2 -> 1.7.0 #63972

merged 3 commits into from Jul 1, 2019

Conversation

nathyong
Copy link
Contributor

@nathyong nathyong commented Jul 1, 2019

Motivation for this change

Update to the latest version on PyPI.

Current nixos-version:

19.03.172927.30a82bba734 (Koi)
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review 63972"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@worldofpeace
Copy link
Contributor

Hi @nathyong. Most contributions like this should target the master branch.
If you'd like to update a package on the stable branch you should backport this after it has been merged to master.

@worldofpeace
Copy link
Contributor

worldofpeace commented Jul 1, 2019

The error you mentioned with nix-review is because a dependency pytest-mock tests are failing on master.

I probably should be able to fix this shortly 👍

See #63826 and #149

@nathyong
Copy link
Contributor Author

nathyong commented Jul 1, 2019

I assume that the merge will be blocked until those are fixed, then.

Just to ensure that I've understood it correctly: The package is located in pkgs/applications/office/watson, and it is declared in nixpkgs at pkgs/top-level/all-packages.nix:20820 under the name watson. The name td-watson is merely a PyPI detail to build this package.

@worldofpeace
Copy link
Contributor

I assume that the merge will be blocked until those are fixed, then.

I can bypass that locally by disabling the checkPhase of pytest-mock.
Then I can review this properly watson.

Just to ensure that I've understood it correctly: The package is located in pkgs/applications/office/watson, and it is declared in nixpkgs at pkgs/top-level/all-packages.nix:20820 under the name watson. The name td-watson is merely a PyPI detail to build this package.

Yes that's correct. However I usually advise people to normalize pname to something expected here, watson, and then give fetchPypi td-watson.

Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please drop doCheck = true; (some reason can't suggest that on GitHub) and use the new pytest command in the checkPhase?

Other than that LGTM.

@worldofpeace worldofpeace merged commit d9b7cfd into NixOS:master Jul 1, 2019
@nathyong nathyong deleted the update-td-watson branch July 1, 2019 03:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants