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

pythonPackages.poetry: init at 0.12.10 #53599

Merged
merged 9 commits into from Jan 10, 2019

Conversation

@jbaum98
Copy link
Contributor

jbaum98 commented Jan 7, 2019

Motivation for this change

Add the poetry package and its dependencies.

Closes #53132.

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 nox --run "nox-review wip"
  • 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

This comment has been minimized.

Copy link
Member

worldofpeace commented Jan 7, 2019

@jbaum98 Why enableParallelBuilding = true; in all the packages you've added?

@jbaum98

This comment has been minimized.

Copy link
Contributor Author

jbaum98 commented Jan 8, 2019

Thanks for your thorough feedback and sorry for what appears to have been a pretty sloppy PR.

@jbaum98 Why enableParallelBuilding = true; in all the packages you've added?

This could be entirely misguided, but I always set enableParallelBuilding = true; because I think 99% you want to be using all your cores, in part based on #5489.

@jbaum98 jbaum98 referenced this pull request Jan 8, 2019

Open

Add v1.3.0 tag/release #10

@jbaum98 jbaum98 force-pushed the jbaum98:poetry branch from fed8819 to 9bd792c Jan 8, 2019

@worldofpeace

This comment has been minimized.

Copy link
Member

worldofpeace commented Jan 8, 2019

Although for pylev getting the GitHub source worked so we could run tests because it uses setup.py, for clikit I'm running into a chicken-egg problem because it wants to use poetry as its build system to build from pyproject.toml. Thoughts?

My instinct is we bootstrap just by not using tests, and then somehow use that that poetry to rebuild its dependencies with tests, but I'm not sure how to do that.

I think we'd have to add a build-python-package-poetry at https://github.com/NixOS/nixpkgs/tree/master/pkgs/development/interpreters/python if we wanted to build these project that use poetry. And obviously we'd have to merge this pr first before that.

However it is confusing that they'd distribute a setup.py in the tarball but not on their github repos.

@jbaum98

This comment has been minimized.

Copy link
Contributor Author

jbaum98 commented Jan 8, 2019

However it is confusing that they'd distribute a setup.py in the tarball but not on their github repos.

I'm guessing poetry produces the setup.py automatically when distributing because poetry isn't ubiquitous.

@jbaum98

This comment has been minimized.

Copy link
Contributor Author

jbaum98 commented Jan 8, 2019

Okay, I think I've fixed anything that isn't the poetry package test situation. Should I squash these commits one per package or one all together and we can land this PR, and then figure out how to bootstrap poetry?

@worldofpeace

This comment has been minimized.

Copy link
Member

worldofpeace commented Jan 8, 2019

Okay, I think I've fixed anything that isn't the poetry package test situation. Should I squash these commits one per package or one all together and we can land this PR, and then figure out how to bootstrap poetry?

I'll give your new commits a quick look to see if they're resolved properly. And yes we'd like a commit for each package.

As for the poetry test situation, I agree with that flow.

@worldofpeace

This comment has been minimized.

Copy link
Member

worldofpeace commented Jan 8, 2019

Remember to add doCheck = false; for the ones that we still can't get tests for.
Also add a comment above with why.

@worldofpeace

This comment has been minimized.

Copy link
Member

worldofpeace commented Jan 8, 2019

@jbaum98 Great, squash at will. Then I will begin some testing locally.

@worldofpeace
Copy link
Member

worldofpeace left a comment

I successfully produced a wheel with poetry build --format wheel.

Though I'd like another set of eyes to look this over before merging.
And see how they feel about enableParallelBuilding here.
cc @Ma27 perhaps?

@worldofpeace

This comment has been minimized.

Copy link
Member

worldofpeace commented Jan 8, 2019

As for being able to build poetry based python packages, I think we can do something similar to what's done for flit.

@Ma27
Copy link
Member

Ma27 left a comment

Regarding the enableParallelBuilding thing: I might've missed something while skimming through the thread, but as it's only used for make parallelism (which happens to be the "default" build approach when using stdenv.mkDerivation), why is it used here? IIRC buildPythonPackage directly invokes setup.py.

Another concern I have is that none of these packages has maintainers listed: we already have numerous breaking python packages on Hydra (due to too strict dependency constraints mostly), so I'd like to see as much of these packages as possible having maintainers that can help us keeping this package set as stable as possible.

@jbaum98

This comment has been minimized.

Copy link
Contributor Author

jbaum98 commented Jan 8, 2019

I'm happy to be the maintainer for these packages if that makes sense, but I'll need to add myself to the maintainers list.

@jbaum98 jbaum98 force-pushed the jbaum98:poetry branch 2 times, most recently from 95275bc to 196b3ee Jan 8, 2019

@worldofpeace

This comment has been minimized.

Copy link
Member

worldofpeace commented Jan 8, 2019

#53599 (comment)
we don't have Python 3.4 on master anymore, so this shouldn't be needed.

My thought was that this is the constraint in the build file and it doesn't hurt to leave it here as documentation/just in case.

I agree with @jbaum98 even if it's not needed. I tend to try and make the derivation respect the things specified by upstream as close as possible.

Unless they want crazy nonsense that is, then maybe patch it all away 😄

@Ma27

This comment has been minimized.

Copy link
Member

Ma27 commented Jan 8, 2019

I'm happy to be the maintainer for these packages if that makes sense, but I'll need to add myself to the maintainers list.

That shouldn't be a problem I guess. Just add yourself in the first commit in this branch (this is what most new contributors do).

@jbaum98 jbaum98 force-pushed the jbaum98:poetry branch from 196b3ee to 573e838 Jan 8, 2019

@jbaum98 jbaum98 force-pushed the jbaum98:poetry branch from 9dffd2c to 9c9049a Jan 9, 2019

checkInputs = [ pytestrunner pytest hypothesis ];

# pytestrunner is only needed to run tests
patches = [ ./no-setup-requires-pytestrunner.patch ];

This comment has been minimized.

@dotlambda

dotlambda Jan 10, 2019

Member

Did you submit that upstream?

This comment has been minimized.

@worldofpeace

worldofpeace Jan 10, 2019

Member

Wasn't sure, pytestrunner instructs people to do this https://github.com/pytest-dev/pytest-runner#usage

@dotlambda
Copy link
Member

dotlambda left a comment

The last few commits should be squashed into the appropriate ones. Otherwise, this is fine.

@jbaum98 jbaum98 force-pushed the jbaum98:poetry branch from 9c9049a to 85ed2d2 Jan 10, 2019

@jbaum98 jbaum98 force-pushed the jbaum98:poetry branch from 85ed2d2 to 53b996a Jan 10, 2019

@Ma27

Ma27 approved these changes Jan 10, 2019

@worldofpeace worldofpeace merged commit 1b1ea35 into NixOS:master Jan 10, 2019

10 checks passed

grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@worldofpeace

This comment has been minimized.

Copy link
Member

worldofpeace commented Jan 10, 2019

@jbaum98 Thanks for your contribution 🎆

@jbaum98 jbaum98 deleted the jbaum98:poetry branch Jan 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment