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

uv: Build as a standalone without Python #357113

Merged
merged 1 commit into from
Nov 24, 2024

Conversation

adisbladis
Copy link
Member

Things done

Using buildPythonPackage triggers dependency propagation, meaning that using uv with nix-shell results in a a shell with the uv Python input in it.
This is problematic for development usage where you only want the one specified version.

Often this design bug in the Python package builders is worked around by deleting $out/nix-support/propagated-build-inputs, but since uv is written in Rust and can be built without a Python interpreter so it's better to just build without a Python interpreter.

  • 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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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.

@fpletz
Copy link
Member

fpletz commented Nov 21, 2024

LGTM. Since the latest updates of uv unfortunately went to staging, we should target staging for this PR so that the staging merge to master doesn't result in merge conflicts.

@adisbladis adisbladis changed the base branch from master to staging November 21, 2024 23:47
adisbladis added a commit to adisbladis/nixpkgs that referenced this pull request Nov 22, 2024
Using `buildPythonPackage` make `ruff` propagate the Python used for the build, which might not be the same Python as you want in your development shell.

NixOS#350654 changed the ruff top-level attribute to be built with Python modules.
This doesn't actually make much sense, we have versioned Python sets, and including just the one Python in the top-level package isn't helpful, and causes issues downstream related to PATH & PYTHONPATH.

See my related [uv PR](NixOS#357113 (comment)).
adisbladis added a commit to adisbladis/nixpkgs that referenced this pull request Nov 22, 2024
Using `buildPythonPackage` make `ruff` propagate the Python used for the build, which might not be the same Python as you want in your development shell.

NixOS#350654 changed the ruff top-level attribute to be built with Python modules.
This doesn't actually make much sense, we have versioned Python sets, and including just the one Python in the top-level package isn't helpful, and causes issues downstream related to PATH & PYTHONPATH.

See my related [uv PR](NixOS#357113 (comment)).
pkgs/by-name/uv/uv/package.nix Outdated Show resolved Hide resolved
@adisbladis adisbladis force-pushed the uv-standalone branch 2 times, most recently from acfc901 to c011a4d Compare November 22, 2024 12:36
@GaetanLepage
Copy link
Contributor

Thank you for submitting this change. I was quite skeptical about #350654 but didn't have a better idea at the time.
Your solution seems the best to me.

@dwt
Copy link
Contributor

dwt commented Nov 22, 2024

I was able to build this on apple silicon by checking out the pull request and running nix build .#uv. This worked (while still slow). However I am having trouble getting Nixpkgs-review pr --checkout commit 357113 to work as it seems to build all of the world (including gfortran and friends).

I would like to check that this pull request works on apple silicon - can somebody give me a hint how I can do so without having a compile farm?

@GaetanLepage
Copy link
Contributor

I was able to build this on apple silicon by checking out the pull request and running nix build .#uv. This worked (while still slow). However I am having trouble getting Nixpkgs-review pr --checkout commit 357113 to work as it seems to build all of the world (including gfortran and friends).

I would like to check that this pull request works on apple silicon - can somebody give me a hint how I can do so without having a compile farm?

This is expected. Maybe you haven't noticed, but this PR now targets the staging branch, where almost nothing is cached.
Running nixpkgs-review on this is not really feasible.

@dwt
Copy link
Contributor

dwt commented Nov 22, 2024

@GaetanLepage Thanks! I am still quite new to Nixpkgs and this is something I did not know yet. Could you recommend a different way in which I could contribute the fact that this works on Darwin?

Copy link
Member

@baloo baloo left a comment

Choose a reason for hiding this comment

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

See comment in the ruff package
#358029 (review)

But same idea here, you're creating duplication and going astray of what upstream intends. This will cause issues further down the line.

@baloo
Copy link
Member

baloo commented Nov 22, 2024

If you want to strip off the propagation, you can do so in your nix-shell:

let
  pkgs = import <nixpkgs> {};
  ruff = pkgs.runCommandNoCC "ruff" {} ''
    mkdir -p $out/bin
    ln -s ${pkgs.ruff}/bin/ruff $out/bin
  '';
  uv = pkgs.runCommandNoCC "uv" {} ''
    mkdir -p $out/bin
    ln -s ${pkgs.uv}/bin/uv $out/bin
  '';
in
  pkgs.mkShell {
    buildInputs = [ ruff uv ];
}

@GaetanLepage
Copy link
Contributor

I would be glad to have the opinion of more experienced maintainers on this matter.
cc @SuperSandro2000 @natsukium

@adisbladis
Copy link
Member Author

If you want to strip off the propagation, you can do so in your nix-shell:

This isn't just problematic for development shells, but also for builds.

I'm using uv in low level packaging scripts (replacing all the nixpkgs python build infra), and uv needlessly leaking a Python is extremely problematic.

Uv isn't a Python package. It's a Rust package to build Python.

If you want to strip off the propagation, you can do so in your nix-shell:

Propagation is only part of the problem, pulling in a Python needlessly is a problem in itself.

@purepani
Copy link
Contributor

purepani commented Nov 23, 2024

See comment in the ruff package #358029 (review)

But same idea here, you're creating duplication and going astray of what upstream intends. This will cause issues further down the line.

We shouldn't include python when it isn't a build requirement, especially when these tools not only can, but also are commonly be used without python at all.

You can just follow other package's lead and have a top level ruff and uv, and keep and have pythonPackages3.uv and pythonPackages3.uv be the only ones with python deps(i.e., what this pr is doing).
See examples:

zstd = callPackage ../development/python-modules/zstd {
inherit (pkgs) zstd;
};

https://github.com/NixOS/nixpkgs/blob/d92e4e2f270b3417dc91182eb11612f97b635986/pkgs/top-level/python-packages.nix#L6414C3-L6417C6

adisbladis added a commit to adisbladis/nixpkgs that referenced this pull request Nov 23, 2024
Using `buildPythonPackage` make `ruff` propagate the Python used for the build, which might not be the same Python as you want in your development shell.

NixOS#350654 changed the ruff top-level attribute to be built with Python modules.
This doesn't actually make much sense, we have versioned Python sets, and including just the one Python in the top-level package isn't helpful, and causes issues downstream related to PATH & PYTHONPATH.

See my related [uv PR](NixOS#357113 (comment)).
adisbladis added a commit to adisbladis/nixpkgs that referenced this pull request Nov 23, 2024
Using `buildPythonPackage` make `ruff` propagate the Python used for the build, which might not be the same Python as you want in your development shell.

NixOS#350654 changed the ruff top-level attribute to be built with Python modules.
This doesn't actually make much sense, we have versioned Python sets, and including just the one Python in the top-level package isn't helpful, and causes issues downstream related to PATH & PYTHONPATH.

See my related [uv PR](NixOS#357113 (comment)).
@baloo
Copy link
Member

baloo commented Nov 23, 2024

The zstd example you're pointing to is a wrapper around the zstd binary and this is a completely different situation.

You should be comparing the situation with other maturin builds:

Any single one of those can be built without python. That doesn't mean we should side-step the original build system and make up our own. Those tools also provide python APIs (and both ruff and uv do) and we should retain them.

@baloo
Copy link
Member

baloo commented Nov 23, 2024

An alternative approach would be to make multiple outputs:

--- a/pkgs/by-name/uv/uv/package.nix
+++ b/pkgs/by-name/uv/uv/package.nix
@@ -22,6 +22,11 @@ python3Packages.buildPythonApplication rec {
     hash = "sha256-xy/fgy3+YvSdfq5ngPVbAmRpYyJH27Cft5QxBwFQumU=";
   };

+  outputs = [
+    "bin"
+    "out"
+  ];
+
   cargoDeps = rustPlatform.importCargoLock {
     lockFile = ./Cargo.lock;
     outputHashes = {
@@ -48,11 +53,15 @@ python3Packages.buildPythonApplication rec {
   ];

   postInstall = ''
+    mkdir -p $bin/bin
+    mv $out/bin/uv $out/bin/uvx $bin/bin/
+    rmdir $out/bin
+
     export HOME=$TMPDIR
     installShellCompletion --cmd uv \
-      --bash <($out/bin/uv --generate-shell-completion bash) \
-      --fish <($out/bin/uv --generate-shell-completion fish) \
-      --zsh <($out/bin/uv --generate-shell-completion zsh)
+      --bash <($bin/bin/uv --generate-shell-completion bash) \
+      --fish <($bin/bin/uv --generate-shell-completion fish) \
+      --zsh <($bin/bin/uv --generate-shell-completion zsh)
   '';

   pythonImportsCheck = [ "uv" ];

You don't get the PYTHON_PATH propagation this way nor any python binary in the PATH:

with import <nixpkgs> {};

mkShell {
    buildInputs = [ ruff.bin uv.bin ];
}

Their dependency tree:

/nix/store/85z1nbxvjlysgpf7xsix8zxccb3qhpiv-uv-0.4.30-bin
├───/nix/store/pacbfvpzqz2mksby36awvbcn051zcji3-glibc-2.40-36
│   ├───/nix/store/2d5spnl8j5r4n1s4bj1zmra7mwx0f1n8-xgcc-13.3.0-libgcc
│   ├───/nix/store/qwjjm4j652ck9izaid7bz63s4hd5bnha-libidn2-2.3.7
│   │   ├───/nix/store/6pqgj71r0850b0cd95yxx0d52zax016i-libunistring-1.2
│   │   │   └───/nix/store/6pqgj71r0850b0cd95yxx0d52zax016i-libunistring-1.2 [...]
│   │   └───/nix/store/qwjjm4j652ck9izaid7bz63s4hd5bnha-libidn2-2.3.7 [...]
│   └───/nix/store/pacbfvpzqz2mksby36awvbcn051zcji3-glibc-2.40-36 [...]
└───/nix/store/97f3gw9vpyxvwjv2i673isvg92q65mwn-gcc-13.3.0-lib
    ├───/nix/store/nhdjqagplar2fdrqnrxlab8vg9i36b3d-gcc-13.3.0-libgcc
    ├───/nix/store/pacbfvpzqz2mksby36awvbcn051zcji3-glibc-2.40-36 [...]
    └───/nix/store/97f3gw9vpyxvwjv2i673isvg92q65mwn-gcc-13.3.0-lib [...]
/nix/store/fr45ms4qa181m5896qp0dvypy6qy1a3h-python3.12-ruff-0.7.4-bin
├───/nix/store/pacbfvpzqz2mksby36awvbcn051zcji3-glibc-2.40-36 [...]
├───/nix/store/97f3gw9vpyxvwjv2i673isvg92q65mwn-gcc-13.3.0-lib [...]
└───/nix/store/b64yimnj6ckkdn17r77zllwj0bxvb4ky-jemalloc-5.3.0
    ├───/nix/store/pacbfvpzqz2mksby36awvbcn051zcji3-glibc-2.40-36 [...]
    ├───/nix/store/0irlcqx2n3qm6b1pc9rsd2i8qpvcccaj-bash-5.2p37
    │   ├───/nix/store/pacbfvpzqz2mksby36awvbcn051zcji3-glibc-2.40-36 [...]
    │   └───/nix/store/0irlcqx2n3qm6b1pc9rsd2i8qpvcccaj-bash-5.2p37 [...]
    ├───/nix/store/97f3gw9vpyxvwjv2i673isvg92q65mwn-gcc-13.3.0-lib [...]
    └───/nix/store/b64yimnj6ckkdn17r77zllwj0bxvb4ky-jemalloc-5.3.0 [...]

the find_uv_bin helper method still needs to be patched up in a similar fashion as ruff:

substituteInPlace python/ruff/__main__.py \

@purepani
Copy link
Contributor

In the case of uv(though not ruff),cargo install --git https://github.com/astral-sh/uv uv mentioned as a way to install uv, which doesn't touch the maturin build system at all: https://docs.astral.sh/uv/getting-started/installation, so I wouldn't quite consider that sidestepping the original build system at least in that case.

@baloo
Copy link
Member

baloo commented Nov 23, 2024

And the one just above relying on wheel and maturin?

Their curl| bash installer does not appear to ship the python libraries but prebuilt-binaries are built using maturin: https://github.com/astral-sh/uv/blob/main/.github/workflows/build-binaries.yml

@purepani
Copy link
Contributor

purepani commented Nov 23, 2024

My point is that:
A. They say that installing without Python/maturin is a thing you can do
B. There are a variety of reasons why you don't want Python to be installed, particularly for uv.

  • at least on a surface level, I would be personally quite surprised if I installed uv and Python came bundled in even as a normal user.
  • uv can be used as a key component of build systems, and as mentioned, having Python installed is quite annoying for that use case.
  • using uv in shells can also be annoying for the same reason

Of course those can be worked around, but they can be quite annoying and difficult to understand.

So, given that there is a reasonable way to package that avoids those problems, it makes sense to me to just avoid those problems.

@baloo
Copy link
Member

baloo commented Nov 23, 2024

Okay. Have you considered the suggestion I've made just above then? What about splitting the outputs? Because of the ordering choice I'm making, by default you would only get the binaries.

We retain the single binary output, they don't bring in python, and we keep the original maturin build?

@ofborg ofborg bot requested a review from GaetanLepage November 23, 2024 08:38
Using `buildPythonPackage` triggers dependency propagation, meaning that using `uv` with `nix-shell` results in a a shell with the `uv` Python input in it.
This is problematic for development usage where you only want the one specified version.

Often this design bug in the Python package builders is worked around by deleting `$out/nix-support/propagated-build-inputs`, but since `uv` is written in Rust and can be built without a Python interpreter so it's better to just build without a Python interpreter.
@purepani
Copy link
Contributor

It think splitting outputs also makes sense
(For whatever reason my brain wasn't working last night and I didn't read that comment correctly).

@adisbladis
Copy link
Member Author

What about splitting the outputs?

No that doesn't make sense.
Again: Uv is a build tool, it can build and install packages for any Python3 interpreter, and wrapping one up in the build of Uv makes no semantic sense what so bloody ever.

Even bringing an unrelated Python into the build closure is hugely problematic.
This causes rebuilds of downstream packages that depend on the top-level attribute just because it's needlessly tacked on Python interpreter of some arbitrary version.
This is unacceptable for a build tool.

@softinio
Copy link
Member

softinio commented Nov 24, 2024

As far as I know uv is a build tool written in Rust with no python dependencies so I would agree with @adisbladis trying to do in this PR. Thought I add my two cents FWIW in case helpful in reaching a positive outcome.

@baloo
Copy link
Member

baloo commented Nov 24, 2024

Even bringing an unrelated Python into the build closure is hugely problematic.
This causes rebuilds of downstream packages that depend on the top-level attribute just because it's needlessly tacked on Python interpreter of some arbitrary version.

So just to make sure I understand, what you're railing against is the following dependency graph:

python3.12 -> uv -> (package built with uv)

And instead what you expect is:

             python3.12
                 |
                 v
uv -> (package built with uv)

If python3.12 changes, then package built with uv is going to rebuild anyway. But this is the extra step that bothers you?

This is unacceptable for a build tool.

What is your opinion about using setuptools as a build tool for python? should we remove the python dependency there too? or is this one acceptable?

Should we remove haskell dependency from cabal? Do you think cargo should not depend on rust (yes I know it ships in the same tree, playing devil's advocate here)?

As far as I can tell, this looks fairly acceptable.

@baloo
Copy link
Member

baloo commented Nov 24, 2024

I don't actually care, I removed my block.

I'd still like to see more experienced maintainers opinion on this before this merges

@mweinelt mweinelt merged commit 7e7177c into NixOS:staging Nov 24, 2024
16 of 17 checks passed
@mweinelt
Copy link
Member

Thank you!

@adisbladis adisbladis mentioned this pull request Dec 22, 2024
13 tasks
GaetanLepage pushed a commit to adisbladis/nixpkgs that referenced this pull request Dec 23, 2024
Using `buildPythonPackage` make `ruff` propagate the Python used for the build, which might not be the same Python as you want in your development shell.

NixOS#350654 changed the ruff top-level attribute to be built with Python modules.
This doesn't actually make much sense, we have versioned Python sets, and including just the one Python in the top-level package isn't helpful, and causes issues downstream related to PATH & PYTHONPATH.

See my related [uv PR](NixOS#357113 (comment)).
GaetanLepage pushed a commit to adisbladis/nixpkgs that referenced this pull request Dec 23, 2024
Using `buildPythonPackage` make `ruff` propagate the Python used for the build, which might not be the same Python as you want in your development shell.

NixOS#350654 changed the ruff top-level attribute to be built with Python modules.
This doesn't actually make much sense, we have versioned Python sets, and including just the one Python in the top-level package isn't helpful, and causes issues downstream related to PATH & PYTHONPATH.

See my related [uv PR](NixOS#357113 (comment)).
GaetanLepage pushed a commit to adisbladis/nixpkgs that referenced this pull request Dec 23, 2024
Using `buildPythonPackage` make `ruff` propagate the Python used for the build, which might not be the same Python as you want in your development shell.

NixOS#350654 changed the ruff top-level attribute to be built with Python modules.
This doesn't actually make much sense, we have versioned Python sets, and including just the one Python in the top-level package isn't helpful, and causes issues downstream related to PATH & PYTHONPATH.

See my related [uv PR](NixOS#357113 (comment)).
GaetanLepage pushed a commit to adisbladis/nixpkgs that referenced this pull request Dec 24, 2024
Using `buildPythonPackage` make `ruff` propagate the Python used for the build, which might not be the same Python as you want in your development shell.

NixOS#350654 changed the ruff top-level attribute to be built with Python modules.
This doesn't actually make much sense, we have versioned Python sets, and including just the one Python in the top-level package isn't helpful, and causes issues downstream related to PATH & PYTHONPATH.

See my related [uv PR](NixOS#357113 (comment)).
GaetanLepage pushed a commit to adisbladis/nixpkgs that referenced this pull request Dec 24, 2024
Using `buildPythonPackage` make `ruff` propagate the Python used for the build, which might not be the same Python as you want in your development shell.

NixOS#350654 changed the ruff top-level attribute to be built with Python modules.
This doesn't actually make much sense, we have versioned Python sets, and including just the one Python in the top-level package isn't helpful, and causes issues downstream related to PATH & PYTHONPATH.

See my related [uv PR](NixOS#357113 (comment)).
GaetanLepage pushed a commit to adisbladis/nixpkgs that referenced this pull request Dec 24, 2024
Using `buildPythonPackage` make `ruff` propagate the Python used for the build, which might not be the same Python as you want in your development shell.

NixOS#350654 changed the ruff top-level attribute to be built with Python modules.
This doesn't actually make much sense, we have versioned Python sets, and including just the one Python in the top-level package isn't helpful, and causes issues downstream related to PATH & PYTHONPATH.

See my related [uv PR](NixOS#357113 (comment)).
GaetanLepage pushed a commit to adisbladis/nixpkgs that referenced this pull request Dec 24, 2024
Using `buildPythonPackage` make `ruff` propagate the Python used for the build, which might not be the same Python as you want in your development shell.

NixOS#350654 changed the ruff top-level attribute to be built with Python modules.
This doesn't actually make much sense, we have versioned Python sets, and including just the one Python in the top-level package isn't helpful, and causes issues downstream related to PATH & PYTHONPATH.

See my related [uv PR](NixOS#357113 (comment)).
GaetanLepage pushed a commit to adisbladis/nixpkgs that referenced this pull request Dec 24, 2024
Using `buildPythonPackage` make `ruff` propagate the Python used for the build, which might not be the same Python as you want in your development shell.

NixOS#350654 changed the ruff top-level attribute to be built with Python modules.
This doesn't actually make much sense, we have versioned Python sets, and including just the one Python in the top-level package isn't helpful, and causes issues downstream related to PATH & PYTHONPATH.

See my related [uv PR](NixOS#357113 (comment)).
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.

8 participants