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.pex: drop phaer from maintainers #382636

Closed
wants to merge 1 commit into from

Conversation

phaer
Copy link
Member

@phaer phaer commented Feb 16, 2025

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/)
  • 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.

@bengsparks
Copy link

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 382636


aarch64-linux

❌ 4 packages failed to build:
  • python312Packages.pex
  • python312Packages.pex.dist
  • python313Packages.pex
  • python313Packages.pex.dist

aarch64-darwin

❌ 4 packages failed to build:
  • python312Packages.pex
  • python312Packages.pex.dist
  • python313Packages.pex
  • python313Packages.pex.dist

@bengsparks
Copy link

bengsparks commented Feb 16, 2025

Since pex-tool/pex#2599, it doesn't seem as if this repository uses hatchling anymore, and instead prefers setuptools, so you should probably be using that instead?

Using setuptools, the build fails due to pex_build attempting to fetch from astral-sh's repository.

If you intend to use the pex_build, you'll have to vendor the fetched files.
pex_build will avoid fetching the file if it is found

@@ -8,14 +8,14 @@

buildPythonPackage rec {
pname = "pex";
version = "2.29.0";
version = "2.33.1";
pyproject = true;

disabled = pythonOlder "3.7";

Choose a reason for hiding this comment

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

Can be removed; Python 3.7 is not in nixpkgs

pyproject = true;

disabled = pythonOlder "3.7";

src = fetchPypi {
inherit pname version;
hash = "sha256-2JeQ1zGKZorVqx+2OzjdF7nln00t42yxP2O0e6AVGx4=";
hash = "sha256-kHTvgXe51TencKDkFQAAdyPXuJLBNpJ0NIy1KB8p5JQ=";
};

build-system = [ hatchling ];

Choose a reason for hiding this comment

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

The referenced PR migrated away from hatchling to setuptools, so this should be updated as well

@@ -8,14 +8,14 @@

Choose a reason for hiding this comment

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

let
  uv-trampoline = {
    aarch64-gui = fetchurl {
      url = "https://raw.githubusercontent.com/astral-sh/uv/refs/tags/0.5.29/crates/uv-trampoline/trampolines/uv-trampoline-aarch64-gui.exe";
      hash = "sha256-mb8x1FpyH+wy11X5YgWfqh/VUwBb62M4Zf9aFr5V4EE=";
    };

    aarch64-console = fetchurl {
      url = "https://raw.githubusercontent.com/astral-sh/uv/refs/tags/0.5.29/crates/uv-trampoline/trampolines/uv-trampoline-aarch64-console.exe";
      hash = "sha256-1S2aM+6CV7rKz+3ncM5X7kk7uDQeuha1+8lUEMYC75k=";
    };

    x86_64-gui = fetchurl {
      url = "https://raw.githubusercontent.com/astral-sh/uv/refs/tags/0.5.29/crates/uv-trampoline/trampolines/uv-trampoline-x86_64-gui.exe";
      hash = "sha256-icnp1oXrOZpc+dHTGvDbTHjr+D8M0eamvRrC9bPI/KI=";
    };

    x86_64-console = fetchurl {
      url = "https://raw.githubusercontent.com/astral-sh/uv/refs/tags/0.5.29/crates/uv-trampoline/trampolines/uv-trampoline-x86_64-console.exe";
      hash = "sha256-nLopBrlCMMFjkKVRlY7Ke2zFGpQOyF5mSlLs0d7+HRQ=";
    };
  };
in

Copy link

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member Author

Choose a reason for hiding this comment

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

@bengsparks Happy to review a PR and close this one in favor of yours - just include the commit which drops me from the maintainers list pls.

@phaer
Copy link
Member Author

phaer commented Feb 16, 2025

@bengsparks Ah, you were a bit too quick for me!

Anyway, my main motivation for this PR was to remove myself from maintainers here as I stopped using pex a while ago. Maybe shouldn't have attempted an update at all; but did patch the the stub fetching out know as that's only used on windows. Feel free to adopt the package and handle that properly! :)

@bengsparks
Copy link

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 382636


aarch64-linux

✅ 4 packages built:
  • python312Packages.pex
  • python312Packages.pex.dist
  • python313Packages.pex
  • python313Packages.pex.dist

aarch64-darwin

✅ 4 packages built:
  • python312Packages.pex
  • python312Packages.pex.dist
  • python313Packages.pex
  • python313Packages.pex.dist

@bengsparks
Copy link

@phaer I figured the trampolines were needed due to the nature of the pex. Glad to know they can just be patched out.
I'm not interested in maintaining this package, but thanks for the offer.

(installed_file.path, installed_file) for installed_file in Record.read(fp)
)

- for stub in windows.fetch_all_stubs():
Copy link

@jsirois jsirois Feb 16, 2025

Choose a reason for hiding this comment

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

This creates a broken Pex distribution since Pex has always support creating a PEX for a target platform while running from a totally different platform. If a Nix user wants to build a PEX for Windows users, they'll get an ugly error with this patch. Not using Nix, but say some Linux distro or macOS straight up, they won't get the error.

Copy link
Member Author

Choose a reason for hiding this comment

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

True - I didn't test building a PEX for different platforms.

Would that have worked without the stubs with pex 2.29.0 or earlier? Asking because I personally never used that functionality and tests were already disabled when I added myself as a maintainer in nixpkgs. I don't use pex at all anymore and am removing myself as a maintainer here. The other maintainer @copumpkin doesn't seem to be very active on github anymore, so we could either keep it on a best-effort basis or drop it from nixpkgs altogether.

Not sure why @bengsparks opened up an upstream issue when he explicitly does not want to maintain this package.

@@ -2,23 +2,26 @@
lib,
buildPythonPackage,
fetchPypi,
hatchling,
setuptools,
Copy link

@jsirois jsirois Feb 16, 2025

Choose a reason for hiding this comment

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

So, you're already super-fragile and have to re-do dep grabbing by hand in your build process (by design I think). As such, it would seem uv could be fetched in this manual list of deps and then maybe some sort of Pex build plumbing could be made aware of this fact... just thinking out loud.

Copy link

@jsirois jsirois Feb 16, 2025

Choose a reason for hiding this comment

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

Ah - right, no good. The uv distribution is fake, just a binary blob. So this won't work.

Choose a reason for hiding this comment

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

Sort of; dep grabbing is cached after the package has been successfully built by the build infra behind nixpkgs.
So an approach like the one suggested here would ultimately yield a built wheel containing the exes.
Users of this package would not have to go through the build process again afaik.

Choose a reason for hiding this comment

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

A related topic is that the pex_build build system is unsuited to nix, due to its fetching of dependencies during build time.
A nix-compatible solution would be to create a package for the pex_build package so we can plug it into the build-system field. This package would then be responsible for fetching the EXEs, instead of the packages it is meant to be building

Copy link

Choose a reason for hiding this comment

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

Great - sounds like you have this under control. Thanks for filling me in a bit on the details. From a tool maintainer perspective re-packagers are a strange case (few of them) vs "normal" users. Making it clear the gymnastics were coming from an effort to re-package is super-good up-front info for things like pex-tool/pex#2687.

Copy link

@jsirois jsirois Feb 16, 2025

Choose a reason for hiding this comment

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

A nix-compatible solution would be to create a package for the pex_build package.

So publish my private one off build system to PyPI? .. or in other words, nix does not (like to) support PEP-517 in-tree build systems at all?

Choose a reason for hiding this comment

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

So-called nix fetchers require passing a hash (usually sha256) to verify that the file has not changed between downloads.
This does, of course, assume that the initial hash represents a safe binary, but otherwise prevents attacks that involve substituting the binaries

Copy link

@jsirois jsirois Feb 16, 2025

Choose a reason for hiding this comment

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

This does, of course, assume that the initial hash represents a safe binary, but otherwise prevents attacks that involve substituting the binaries

But you could simply do that for the pex whl itself directly, right? Or the sdist, from which you could build the whl. The sdist both has the Pex sources and the binary blobs, all in one shot. This is seeming contrived but I'm sure there is some good reason the git clone for sources is better than the sdist. I actually hemmed and hawed about sticking the blobs in the sdist and chose to do it for cases like yours! But maybe re-packagers don't like working with sdists?

Copy link

@bengsparks bengsparks Feb 16, 2025

Choose a reason for hiding this comment

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

So publish my private one off build system to PyPI?

Sounds a bit like overkill; if it's a one off then there's no reason to do so, as packages with few usages aren't desired. If there were further packages that used this, then it could be considered.

Copy link

Choose a reason for hiding this comment

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

Ok. So you're aware, these are a general and official thing in the Python world: https://peps.python.org/pep-0517/#in-tree-build-backends

@bengsparks bengsparks self-requested a review February 16, 2025 23:17
Copy link

@bengsparks bengsparks left a comment

Choose a reason for hiding this comment

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

Requires the changes specified by @jsirois

@phaer
Copy link
Member Author

phaer commented Feb 16, 2025

As written above: Please feel free to improve and maintain this yourself, @bengsparks - I won't. So I dropped the update altogether and am just removing myself here now.

@phaer phaer changed the title python3Packages.pex: 2.29.0 -> 2.33.1, drop phaer as maintainer python3Packages.pex: drop phaer from maintainers Feb 16, 2025
@phaer phaer closed this Feb 17, 2025
@phaer phaer deleted the pex-update branch February 17, 2025 11:39
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.

3 participants