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.

Sign up for GitHub

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

Upstream overrides #1796

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
+43,790 −8
Open

Upstream overrides #1796

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f9ee56b
overrides: add lice
PerchunPak Sep 6, 2024
c2a75a3
overrides: add pycln
PerchunPak Sep 6, 2024
89d4c1c
overrides: add libcst
PerchunPak Sep 6, 2024
709c5dc
overrides: fix gitpython
PerchunPak Sep 6, 2024
3d2a620
overrides: add asyncache
PerchunPak Sep 6, 2024
9457c2c
overrides: add poetry-dynamic-versioning to mcstatus
PerchunPak Sep 6, 2024
ce2d9a0
overrides: add apykuma
PerchunPak Sep 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions overrides/build-systems.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1135,6 +1135,9 @@
"setuptools",
"setuptools-scm"
],
"apykuma": [
"poetry-core"
],
"aqipy-atmotech": [
"setuptools"
],
Expand Down Expand Up @@ -1435,6 +1438,9 @@
"async-upnp-client": [
"setuptools"
],
"asyncache": [
"poetry-core"
],
"asyncclick": [
"setuptools",
"setuptools-scm"
Expand Down Expand Up @@ -8117,7 +8123,8 @@
}
],
"gitpython": [
"setuptools"
"setuptools",
"typing-extensions"
],
"glad": [
"setuptools"
Expand Down Expand Up @@ -10920,8 +10927,7 @@
"setuptools"
],
"libcst": [
"setuptools",
"setuptools-scm"
"setuptools-rust"
],
"libevdev": [
"setuptools"
Expand Down Expand Up @@ -11020,6 +11026,9 @@
"libvirt-python": [
"setuptools"
],
"lice": [
"setuptools"
],
"license-expression": [
"setuptools",
"setuptools-scm"
Expand Down Expand Up @@ -11766,6 +11775,7 @@
],
"mcstatus": [
"poetry-core",
"poetry-dynamic-versioning",
"setuptools"
],
"md-toc": [
Expand Down Expand Up @@ -17073,6 +17083,9 @@
"setuptools",
"setuptools-scm"
],
"pycln": [
"poetry-core"
],
"pycm": [
"setuptools"
],
Expand Down
26 changes: 21 additions & 5 deletions overrides/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1023,11 +1023,12 @@ lib.composeManyExtensions [
}
));

gitpython = prev.gitpython.overridePythonAttrs (
old: {
buildInputs = old.buildInputs or [ ] ++ [ final.typing-extensions ];
}
);
gitpython = prev.gitpython.overridePythonAttrs {
postPatch = ''
substituteInPlace git/cmd.py \
--replace 'git_exec_name = "git"' 'git_exec_name = "${pkgs.gitMinimal}/bin/git"'
Copy link
Contributor

Choose a reason for hiding this comment

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

gitpython should not decide on it's own to pull in a specific git as a dependency imo

Copy link
Author

Choose a reason for hiding this comment

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

How it will function then? It is a general approach to patch packages to provide their dependencies without propagating them. You can change git version used using an overlay

Copy link
Contributor

Choose a reason for hiding this comment

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

Provide the git binary in PATH or override the variable in your application code

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not expect gitpython the python package to install git for me

Copy link
Author

Choose a reason for hiding this comment

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

Well, this is how gitpython in nixpkgs is packaged:

https://github.com/NixOS/nixpkgs/blob/a8be94c0af330325c8c96730cdae5e168a0632a1/pkgs/development/python-modules/gitpython/default.nix#L34

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like it was introduced 6 years ago in upstream: NixOS/nixpkgs#41844

I guess I'm fine with this behaviour then

'';
};

grpcio = prev.grpcio.overridePythonAttrs (old: {
nativeBuildInputs = old.nativeBuildInputs or [ ] ++ [ pkg-config ];
Expand Down Expand Up @@ -1401,6 +1402,21 @@ lib.composeManyExtensions [
'';
});

libcst = prev.libcst.overridePythonAttrs (
old: lib.optionalAttrs (!(old.src.isWheel or false))
{
cargoDeps = pkgs.rustPlatform.importCargoLock {
Copy link
Member

Choose a reason for hiding this comment

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

This is using IFD. Please use cargoVendorHash instead.

Copy link
Author

Choose a reason for hiding this comment

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

You mean cargoHash? I cannot find anything about cargoVendorHash in nixpkgs

Copy link
Author

Choose a reason for hiding this comment

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

I assume you meant pkgs.rustPlatform.fetchCargoTarball, but why? importCargoLock gives an ability to have less maintenance burden because of not having hash for every single release. This would still be reproducible, as hashes are in poetry.lock, but also will make that any new version of libcst is automatically supported:

libcst = prev.libcst.overridePythonAttrs (
  old: lib.optionalAttrs (!(old.src.isWheel or false)) (
    let
      unpackedSrc = stdenv.mkDerivation {
        name = "unpacked-${old.src.name}";
        inherit (old) src;
        buildPhase = "cp -r . $out";
      };
    in
    {
      cargoDeps = pkgs.rustPlatform.importCargoLock {
        lockFile = "${unpackedSrc.out}/native/Cargo.lock";
      };
      cargoRoot = "native";
      nativeBuildInputs = old.nativeBuildInputs or [ ] ++ [
        pkgs.rustPlatform.cargoSetupHook # handles `importCargoLock`
        pkgs.rustc
        pkgs.cargo
      ];
    }
  )
);

There are also multiple other places, where poetry2nix could automatically get dependencies, but instead you chose to manually add them to every package. E.g. if poetry or setuptools are required for build: https://github.com/py-mine/mcstatus/blob/0bbd4842178fa1db252371ce5599651e4b867efd/pyproject.toml#L164

Copy link

Choose a reason for hiding this comment

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

@adisbladis can you elaborate?

Copy link
Author

Choose a reason for hiding this comment

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

I learned about IFD since then and will try to fix the issue

lockFile = ./libcst/${old.version}-Cargo.lock;
};
cargoRoot = "native";
nativeBuildInputs = old.nativeBuildInputs or [ ] ++ [
pkgs.rustPlatform.cargoSetupHook # handles `importCargoLock`
pkgs.rustc
pkgs.cargo
];
}
);

libvirt-python = prev.libvirt-python.overridePythonAttrs (old: {
nativeBuildInputs = old.nativeBuildInputs or [ ] ++ [ pkg-config ];
propagatedBuildInputs = [ pkgs.libvirt ];
Expand Down
Loading
Loading