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.

Sign up for GitHub

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

Jump to bottom

pythonPackages.oauthenticator & dependencies #34079

Merged
merged 4 commits into from Feb 3, 2018
Merged

pythonPackages.oauthenticator & dependencies #34079

merged 4 commits into from Feb 3, 2018

Conversation

ixxie
Copy link
Contributor

@ixxie ixxie commented Jan 20, 2018

Packaged pythonPackages.oauthenticator and its dependencies, pythonPackages.mwoauth and pythonPackages.globus-sdk.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-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 nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@ixxie ixxie requested a review from FRidh as a code owner January 20, 2018 13:38
@FRidh FRidh self-assigned this Jan 20, 2018
@FRidh
Copy link
Member

FRidh commented Jan 20, 2018

#31871 (comment) needs to be fixed first. When that is done, the additional commits that are in this PR can be merged.

@ixxie
Copy link
Contributor Author

ixxie commented Jan 20, 2018

@FRidh - fixed #31871 and rebased this PR.

@FRidh
Copy link
Member

FRidh commented Jan 20, 2018

@ixxie can you rebase, just so the duplicate commits are gone.

@ixxie
Copy link
Contributor Author

ixxie commented Jan 20, 2018

@FRidh done!

@FRidh
Copy link
Member

FRidh commented Jan 20, 2018

Please check whether tests are run. In case a package does not provide any tests, we set doCheck = false; and include a comment stating the reason.

@ixxie
Copy link
Contributor Author

ixxie commented Jan 20, 2018

@FRidh - I double checked and as far as I can tell all these packages run tests. When distinct, I made the test dependencies explicit by stating buildInputs. It also seems some dependencies were propagated from other dependencies; I made these explicit as well.

@FRidh
Copy link
Member

FRidh commented Jan 21, 2018

Good. Note that with buildPython* test dependencies are better specified using checkInputs. In that case they are only included when doCheck = true

@ixxie
Copy link
Contributor Author

ixxie commented Jan 21, 2018

@FRidh - switched them to checkInputs now.

aborsu
aborsu reviewed Jan 22, 2018
View reviewed changes
pkgs/development/python-modules/globus-sdk/default.nix Outdated
@@ -0,0 +1,31 @@
{ lib
, python
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need python?
You already have buildPythonPackage

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

pkgs/development/python-modules/mwoauth/default.nix Outdated
@@ -0,0 +1,32 @@
{ lib
, python
Copy link
Contributor

Choose a reason for hiding this comment

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

idem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

pkgs/development/python-modules/oauthenticator/default.nix Outdated
@@ -0,0 +1,40 @@
{ lib
, python
Copy link
Contributor

Choose a reason for hiding this comment

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

idem

pkgs/development/python-modules/pytest-tornado/default.nix Outdated
@@ -0,0 +1,26 @@
{ lib
, python
Copy link
Contributor

Choose a reason for hiding this comment

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

Idem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

pkgs/development/python-modules/oauthenticator/default.nix

propagatedBuildInputs = [ jupyterhub ];

disabled = pythonOlder "3.4";
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this would work
inherit (jupyterhub) disabled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't work unfortunately.

@FRidh
Copy link
Member

FRidh commented Jan 24, 2018 

Ran 0 tests in 0.000s

No tests are found. Either the test runner cannot find tests and needs to be patched, or there simply are no tests in which case the tests need to be disabled. Do include a comment explaining why the tests are disabled.

This applies to each package.

@ixxie
Copy link
Contributor Author

ixxie commented Jan 25, 2018

@FRidh - this is why I was baffled and asking questions about how to really check what is going on: if you remove the checkInputs on any of the packages you will see that the build will run tests and they will miss inputs, making the build fail. If I add them, the build is successful but indeed as you note it claims no tests are run. From my understanding of the packages, the checkInputs are indeed necessary for tests but don't need to be propagated; indeed they are sometimes explicitly listed in the test-requirements.txt.

FRidh
FRidh reviewed Feb 3, 2018
View reviewed changes
pkgs/development/python-modules/oauthenticator/default.nix
inherit pname version;
sha256 = "0rlg63ii7sxj1xad2nx6wk1xgv3a8dm0az0q9g2v6hdv9cnc4b55";
};

Copy link
Member

@FRidh FRidh Feb 3, 2018 •
edited

Choose a reason for hiding this comment

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

  checkPhase = ''
    py.test oauthenticator/tests
  '';

  # No tests in archive
  doCheck = false;

The tests folder does exist on GitHub.

@ixxie
Copy link
Contributor Author

ixxie commented Feb 3, 2018

@FRidh - followed your suggestion with oauthenticator and reused it for globus-sdk, and then disabled testing explicitly for the other packages.

@FRidh
Copy link
Member

FRidh commented Feb 3, 2018

@ixxie Good! It seems some commits that don't belong in this PR are here.

@ixxie
Copy link
Contributor Author

ixxie commented Feb 3, 2018

@FRidh whoopsy! Fixed now.

@FRidh FRidh merged commit fa71b1a into NixOS:master Feb 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FRidh FRidh FRidh left review comments

@aborsu aborsu aborsu left review comments

Assignees

@FRidh FRidh

Labels
2.status: work-in-progress 6.topic: python 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ixxie @FRidh @aborsu @GrahamcOfBorg