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

python3Packages.mkPythonEditablePackage: init #339228

Merged
merged 1 commit into from
Sep 11, 2024

Conversation

adisbladis
Copy link
Member

@adisbladis adisbladis commented Sep 3, 2024

Description of changes

When developing Python packages it's common to install packages in editable mode.
Like mkPythonMetaPackage this function exists to create an otherwise empty package, but also containing a pointer to an impure location outside the Nix store that can be changed without rebuilding.

The editable root is passed as a string. Normally .pth files contains absolute paths to the mutable location. This isn't always ergonomic with Nix, so I have decided to expand environment variables at runtime.
This means that a shell hook setting up something like a $REPO_ROOT variable can be used as the relative package root.

Note that overriding packages deeper in the dependency graph can work, but it's not the primary usecase and overriding existing packages can make others break in unexpected ways.

{ pkgs ? import <nixpkgs> { } }:

let
  myPython = pkgs.python.override {
    self = myPython;
    packageOverrides = pyfinal: pyprev: {
      # An editable package with a script that loads our mutable location
      my-editable = pyfinal.mkPythonEditablePackage {
        pname = "my-editable";
        version = "0.1.0";
        root = "$REPO_ROOT/src"; # Use environment variable expansion at runtime
        # Inject a script (other PEP-621 entrypoints are also accepted)
        scripts = {
          my-script = "my_editable.main:main";
        };
      };
    };
  };

  pythonEnv =  testPython.withPackages (ps: [ ps.my-editable ]);

in pkgs.mkShell {
  packages = [ pythonEnv ];
}

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.11 Release Notes (or backporting 23.11 and 24.05 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.

Add a 👍 reaction to pull requests you find important.

Copy link
Member

@phaer phaer left a comment

Choose a reason for hiding this comment

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

Thank you for this!

Think i found a typo, commented a bit on docs and found a single semantic change I'd like to see: prepending, not appending to sys.path.

doc/languages-frameworks/python.section.md Outdated Show resolved Hide resolved
pkgs/development/interpreters/python/editable.nix Outdated Show resolved Hide resolved
doc/languages-frameworks/python.section.md Show resolved Hide resolved
@phaer phaer requested a review from DavHau September 3, 2024 11:52
@adisbladis adisbladis force-pushed the python-editable branch 2 times, most recently from e37568f to 41611f8 Compare September 3, 2024 22:09
adisbladis added a commit to adisbladis/uv2nix that referenced this pull request Sep 7, 2024
adisbladis added a commit to adisbladis/uv2nix that referenced this pull request Sep 7, 2024
adisbladis added a commit to adisbladis/uv2nix that referenced this pull request Sep 7, 2024
adisbladis added a commit to adisbladis/uv2nix that referenced this pull request Sep 7, 2024
adisbladis added a commit to adisbladis/uv2nix that referenced this pull request Sep 7, 2024
adisbladis added a commit to adisbladis/uv2nix that referenced this pull request Sep 7, 2024
@adisbladis
Copy link
Member Author

Unless someone gives me a good reason not to I intend to self-merge this sooner rather than later.

@natsukium
Copy link
Member

I'm not sure if adding more buildPythonPackage friends is the direction we should go.
Can we take an approach that injects into existing builders, maybe editableInstallHook?

@adisbladis
Copy link
Member Author

adisbladis commented Sep 8, 2024

I'm not sure if adding more buildPythonPackage friends is the direction we should go.
Can we take an approach that injects into existing builders, maybe editableInstallHook?

No. We need to create package metadata and pointer files in the store as regular Python packages, and these should be built without sources.
The metadata files needs to be consumed by creating a venv-like created by withPackages.

There is no place to set up a hook to get editable behaviour for development shells.

@adisbladis
Copy link
Member Author

I could conjure up an implementation of a hook-based editable builder, but it's usage would have to look like:

buildPythonPackage {
  pname = "my-editable";
  version = "0.1.0";

  # Assuming that you'd want to use pyproject.toml from current directory, don't know if there is a more elegant way to express this
  # If you'd forget to filter your sources the point of the editable is kinda moot since you'd rebuild all the time anyway.
  src = lib.filesets.toSource {
    root = ./.;
    fileset = [ ./pyproject.toml ];
  };

  pyproject = true;

  editableRoot = "$REPO_ROOT";

  nativeBuildInputs = [
    editablePatchHook # Patches pyproject.toml with the editable root build and forces hatchling to be the build-system
  ];
}

That's a lot of stuff to get right every time for every user. I prefer a function abstraction based approach in this instance, even though I generally agree that we should move towards hooks.

@adisbladis
Copy link
Member Author

Giving a 24 hour notice to merge.

@natsukium
Copy link
Member

Is the primary concern the handling of src?
A hook-based interface looks good, but I agree that it could be confusing to the end user.
I'm leaning towards having a simple function since it's for external use.

@adisbladis
Copy link
Member Author

adisbladis commented Sep 9, 2024

Is the primary concern the handling of src?

That's one of the main two concerns, but also handling of editableRoot. It's very appealing to go for editableRoot = ./. or something similar, which will end up pointing to a store path, and not your editable location.

By making it a hook it's not possible to enforce the type of editableRoot at eval time.
We could of course capture this at build time and error out there instead, but I find the user experience of that terrible compared to erroring out at eval time.

I'm leaning towards having a simple function since it's for external use.

I think this is really the core of it. This is a development helper, not a composable builder.

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.

Thanks for your explanation. I support this implementation.

Copy link
Member

@phaer phaer left a comment

Choose a reason for hiding this comment

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

I think this a good step forward, thank you!

@adisbladis adisbladis merged commit 3fd6481 into NixOS:master Sep 11, 2024
24 checks passed
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.

4 participants