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

python3.pkgs.buildPythonPackage: Separate runtime & build time dependencies #271597

Merged
merged 4 commits into from
Feb 20, 2024

Conversation

adisbladis
Copy link
Member

@adisbladis adisbladis commented Dec 2, 2023

Description of changes

See #272178 for more context.

Dependency separation

Adds a separation between Python runtime dependencies, Python build-time dependencies & non-Python dependencies arguments to buildPythonPackage.
It's important to have these distinctions so we can solve our long standing issues re dependency propagation in Python.
With this change in place it's possible to implement a solution for #170577.

It's similar to #102613 (a subset, really) but with the notable changes that:

  • Uses PEP-621 standard names/idioms
    WIP: Python: wrap and patch using requiredPythonModules instead of propagatedBuildInputs #102613 introduces the argument requiredPythonModules, but with Python metadata being standardised I'd like to be more closely aligned with that.
  • Only implements the API that enables changing the behaviour
    It's probably going to take a while to refactor all of nixpkgs to use the appropriate arguments.
    This change is purposely minimal in the hopes that we can move forward with it more quickly.

For now dependencies are always mapped onto propgatedBuildInputs, so there is no change in behaviour in the immediate term.
2 packages are migrated as a PoC, one is using format = "pyproject" and the other is setuptools.

Interface changes

This also unifies regular dependencies with optional dependencies:

{
  passthru.optional-dependencies.socks = [ pysocks ];
  nativeBuildInputs = [ poetry-core ];
  propagatedBuildInputs = [ requests ];
}

becomes

{
  optional-dependencies.socks = [ pysocks ];
  build-system = [ poetry-core ];
  dependencies = [ requests ];
}

which looks much more like what a standard pyproject.toml file contains.
I think the interface change alone is a pretty good improvement.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Priorities

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: python 8.has: documentation This PR adds or changes documentation labels Dec 2, 2023
@adisbladis adisbladis requested a review from figsoda December 2, 2023 05:36
@adisbladis adisbladis force-pushed the python-runtime-build-time-sep branch from b63f4d3 to 65f1f51 Compare December 2, 2023 11:23
@adisbladis adisbladis marked this pull request as draft December 2, 2023 11:43
@adisbladis adisbladis marked this pull request as ready for review December 2, 2023 12:06
@adisbladis adisbladis force-pushed the python-runtime-build-time-sep branch 2 times, most recently from b3515e8 to c330367 Compare December 2, 2023 23:05
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Dec 3, 2023
@jonringer
Copy link
Contributor

I wonder if it would be better to just expose a new mkDerivation helper (e.g. buildPyprojectPackage). I think within this context, it's simpler to expose the PEP-621terminology without having to compound it with the legacy behavior of buildPythonPackage.

@adisbladis
Copy link
Member Author

adisbladis commented Dec 7, 2023

I wonder if it would be better to just expose a new mkDerivation helper (e.g. buildPyprojectPackage). I think within this context, it's simpler to expose the PEP-621terminology without having to compound it with the legacy behavior of buildPythonPackage.

As discussed on Matrix I don't like adding another function because:

  • The naming is already confusing between buildPythonPackage/buildPythonApplication

    This would be even more murky with a third name thrown in the mix.

  • This PR doesn't semantically change much, it just allows you to speak about Python dependencies specifically as opposed to explaining the Nix semantics explicitly like propagatedBuildInputs.

    buildPythonPackage has no real notion of Python dependencies which is a bit silly when you think about it.

  • Python builds are converging on pyproject

    PEP-517 style builds are becoming the default in the Python ecosystem. It would be unfortunate if our default Python builder wasn't aligned with the default Python package formats.

  • Optional dependencies

    Optional dependencies are currently "supported" by buildPythonPackage by the ad-hoc use of passthru.
    This PR makes optional-dependencies (setuptools calls this extras) an explicit concept known to buildPythonPackage.
    These are not new concepts, PEP-621 just gave them better more clear naming.

@SuperSandro2000
Copy link
Member

  • The naming is already confusing between buildPythonPackage/buildPythonApplication

💯
I have done a lot with nixpkgs python and it is still hard for me to remember which I am supposed to use.


Also if we can get away with not fragmenting things and just shove this under the current system without breaking everything then I would be super happy.

@FRidh
Copy link
Member

FRidh commented Jan 2, 2024

The main topic here is the separation. We need that, without a doubt.

Then there is the naming. This PR introduces non-standard names for Nixpkgs, but names that are the standard in Python packaging. While I've long held the opinion that we should have names that are similar to what is used elsewhere in Nixpkgs, this builder is used by other higher-level tools such as dream2nix and pdm2nix, which would preferably work with an interface more closely aligned to Python packaging, and for direct users of this function it is I think also more convenient.

I'd say let's go ahead with this.

Copy link
Member

@natsukium natsukium left a comment

Choose a reason for hiding this comment

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

I have no particular opinion on the naming and have left some comments about the document.

Then, not just for this PR, but I would like to see a section like the changelog.
Since not everyone follows these discussions and changes, it would be easier to catch up if we had something to list all the changes.

@adisbladis adisbladis force-pushed the python-runtime-build-time-sep branch 3 times, most recently from f97727e to 62fafde Compare February 17, 2024 09:20
@adisbladis
Copy link
Member Author

Rebased, refactored & fixed comments.

@adisbladis adisbladis force-pushed the python-runtime-build-time-sep branch from 68e2446 to cbc7eac Compare February 18, 2024 03:33
Since NixOS#161835 we've had the
concept of `passthru.optional-dependencies` for Python optional deps.

Having to explicitly put optional-dependencies in the passthru attrset
is a bit strange API-wise, even though it semantically makes sense.

This change unifies the handling of non-optional & optional Python
dependencies using the names established from PEP-621 (standardized pyproject.toml project metadata).
Much like the previous commit that adds dependencies &
optional-dependencies this aligns PEP-517 build systems with how they
are defined in PEP-518/PEP-621.

The naming `build-system` (singular) is aligned with upstream Python standards.
@adisbladis adisbladis force-pushed the python-runtime-build-time-sep branch from cbc7eac to 07a221b Compare February 18, 2024 04:40
@mweinelt mweinelt merged commit fa83add into NixOS:master Feb 20, 2024
19 checks passed
@natsukium natsukium mentioned this pull request Mar 23, 2024
13 tasks
yajo added a commit to moduon/dream2nix that referenced this pull request Apr 30, 2024
NixOS/nixpkgs#271597 implemented a new format for python packages that is starting to get traction in nixpkgs/unstable (soon to become NixOS 24.05).

That is a progress towards NixOS/nixpkgs#272178, which would be a massive improvement for dream2nix once completed.

This first step just makes sure that `buildPythonPackage` supports the new arguments properly. Without this change, many dream2nix python derivations fail to build on nixos-unstable right now.

Probably other python auto-builders should leverage these new options when available for the d2n user. That can be added later.

@moduon MT-1075
yajo added a commit to moduon/dream2nix that referenced this pull request Apr 30, 2024
NixOS/nixpkgs#271597 implemented a new format for python packages that is starting to get traction in nixpkgs/unstable (soon to become NixOS 24.05).

That is a progress towards NixOS/nixpkgs#272178, which would be a massive improvement for dream2nix once completed.

This first step just makes sure that `buildPythonPackage` supports the new arguments properly. Without this change, many dream2nix python derivations fail to build on nixos-unstable right now.

Probably other python auto-builders should leverage these new options when available for the d2n user. That can be added later.

@moduon MT-1075
yajo added a commit to moduon/dream2nix that referenced this pull request May 13, 2024
NixOS/nixpkgs#271597 implemented a new format for python packages that is starting to get traction in nixpkgs/unstable (soon to become NixOS 24.05).

That is a progress towards NixOS/nixpkgs#272178, which would be a massive improvement for dream2nix once completed.

This first step just makes sure that `buildPythonPackage` supports the new arguments properly. Without this change, many dream2nix python derivations fail to build on nixos-unstable right now.

Probably other python auto-builders should leverage these new options when available for the d2n user. That can be added later.

@moduon MT-1075
phaer pushed a commit to phaer/dream2nix that referenced this pull request Jun 11, 2024
NixOS/nixpkgs#271597 implemented a new format for python packages that is starting to get traction in nixpkgs/unstable (soon to become NixOS 24.05).

That is a progress towards NixOS/nixpkgs#272178, which would be a massive improvement for dream2nix once completed.

This first step just makes sure that `buildPythonPackage` supports the new arguments properly. Without this change, many dream2nix python derivations fail to build on nixos-unstable right now.

Probably other python auto-builders should leverage these new options when available for the d2n user. That can be added later.

@moduon MT-1075
@uninsane
Copy link
Contributor

on the Nix side, i feel like we've spent a good deal of effort pushing language-specific logic away from mkDerivation wrappers and toward things like setup hooks. in other threads, i'm now being asked to change my packages from:

nativeBuildInputs = [ poetry-core ];

to

build-system = [ poetry-core ];

isn't this a regression? as far as i can tell, build-system is some bespoke extra concept every future nixpkgs packager will have to learn once they encounter Python. either we should rework the packaging for every other language and use build-system = [ cmake ];, build-system = [ meson ];, to regain the uniformity we had before this change, or the benefit from this is really so great as to outweigh the increased load it puts on first-time packagers.

@mweinelt
Copy link
Member

isn't this a regression?

No, a conscious decision to adopt the PEP518/621 lingo in the Python-specific builders.

@uninsane
Copy link
Contributor

No, a conscious decision to adopt the PEP518/621 lingo in the Python-specific builders.

it's not clear to me reading just this PR thread if there is wide agreement in adopting python-specific lingo at this level within nixpkgs. at least, in the handful of people who weighed in here, the issue of naming/jargon was mentioned more than once. honestly, i don't mean to bikeshed, and if this was discussed more thoroughly in a different thread then link me there and i'll be quiet. yet i think it has to be acknowledged that this has real repercussions (some bad, some good) for first-time packagers. new contributors have a hard enough time writing their first package. it's made worse if none of the knowledge learned in that first package carries through to their second package.

@natsukium
Copy link
Member

or the benefit from this is really so great as to outweigh the increased load it puts on first-time packagers.

Yes, this change would significantly benefit nixpkgs.
It is described in several issues and PRs linked to this PR.

Adding Python modules to the nativeBuildInputs, buildInputs, propagatedBuildInputs, and nativeCheckInputs that are commonly used in current nixpkgs has caused problems for years.
To solve this, we decided to introduce new parameters to handle inputs for Python.

We did not change nativeBuildInputs -> build-system and propagatedBuildInputs -> dependencies just to follow standard Python naming.
We introduced the new concepts and referenced PEP518/621 for the names.
Honestly, I still don't care about parameter names, but I think it's a good decision to follow them now that Python packaging is converging to PEP517/518/621, unlike a few years ago.

You will also encounter these concepts when using hook with mkDerivation.

I've packaged and maintained hundreds of packages, but I still can't work in an unfamiliar category without reading the manual (or the source code) though.

Comment on lines +221 to +224

Aside from propagating dependencies,
`buildPythonPackage` also injects code into and wraps executables with the
paths included in this list. Items listed in `extras_requires` go here.
Copy link
Member

Choose a reason for hiding this comment

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

That's a duplicate.

Suggested change
Aside from propagating dependencies,
`buildPythonPackage` also injects code into and wraps executables with the
paths included in this list. Items listed in `extras_requires` go here.

Copy link
Contributor

Choose a reason for hiding this comment

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

fix is out for review here: #322267

@Yarny0 Yarny0 mentioned this pull request Oct 19, 2024
13 tasks
chaoflow pushed a commit to chaoflow/dream2nix that referenced this pull request Nov 16, 2024
NixOS/nixpkgs#271597 implemented a new format for python packages that is starting to get traction in nixpkgs/unstable (soon to become NixOS 24.05).

That is a progress towards NixOS/nixpkgs#272178, which would be a massive improvement for dream2nix once completed.

This first step just makes sure that `buildPythonPackage` supports the new arguments properly. Without this change, many dream2nix python derivations fail to build on nixos-unstable right now.

Probably other python auto-builders should leverage these new options when available for the d2n user. That can be added later.

@moduon MT-1075
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: python 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants