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

feat: allow to split buildInputs in python derivations #684

Merged
merged 1 commit into from Sep 22, 2023

Conversation

yajo
Copy link
Contributor

@yajo yajo commented Sep 15, 2023

Follow-up of #676 (comment), now dependencies marked as buildInputs won't be included as propagatedBuildInputs for python derivations.

@DavHau
Copy link
Member

DavHau commented Sep 15, 2023

If my understanding is correct, and this is merely a build graph / closure size optimization, then I think it is unnecessary to put this complexity in the example.

Concerning excluding all buildInputs from propagatedBuildInputs by default. I wonder if this can have undesired impacts when cross compiling.

For the long term, it might be better if we had a python specific interface to differentiate between install and setup requirements.
... which then maps to equivalent mkDerivation build inputs.

We can hot fix the issue, but I would like to know exactly what problem we are facing before doing so. If nothing is broken, there is no need to fix it.

@yajo
Copy link
Contributor Author

yajo commented Sep 18, 2023

You could just add this to a d2n module:

{config, ...}: {
  ...
  mkDerivation.buildInputs = [config.deps.python.pkgs.poetry-core];
}

However, very likely you'd get a conflict in some duplicated dependency, if the project you're building requires something that is also required by poetry-core.

Dream2nix, instead, adds all inputs as propagatedBuildInputs. This works for most cases.

The improvement I'm proposing here is that you can now do:

{config, ...}: {
  ...
  mkDerivation.buildInputs = [config.pip.drvs.poetry-core.public.out];
}

And, by doing this, you're implicitly removing poetry-core from propagatedBuildInputs. Thus, optimizing the package is now simpler.

The separation of buildInputs and propagatedBuildInputs should become more relevant after NixOS/nixpkgs#170577 is fixed. When that happens, you will not need to rebuild a wheel just because one dependency gets bumped, unless it's in buildInputs.

@DavHau
Copy link
Member

DavHau commented Sep 18, 2023

How about adding an option setupDependencies analog to rootDependencies. so the user can simply do this:

pip.setupDependencies = {
  poetry-core = true
}

With the effect that dependencies listed there will be put into buildInputs instead of propagatedBuildInputs.

To me that feels better than manually having to builtins.map (name: config.pip.drvs.${name}.public.out) ....
Also it would prevent possible infinite recursions occurring from making propagatedBuildInputs directly depend on buildInputs.

As a follow up we could then make the locking script put that information into the lock file and populate these fields automatically.

@yajo
Copy link
Contributor Author

yajo commented Sep 19, 2023

Looks good to me. I'd call it pip.buildDependencies just to keep the name closer to buildInputs. WDYT?

@DavHau
Copy link
Member

DavHau commented Sep 19, 2023

Sounds good

@yajo
Copy link
Contributor Author

yajo commented Sep 20, 2023

Refactored.

@DavHau
Copy link
Member

DavHau commented Sep 20, 2023

Thanks and sorry for the broken CI.
Could you please make sure this passes: nix flake check github:nix-community/dream2nix/pull/684/merge

Follow-up of nix-community#676 (comment), now dependencies marked as buildInputs won't be included as propagatedBuildInputs for python derivations.
@DavHau DavHau merged commit 16552e5 into nix-community:main Sep 22, 2023
1 check passed
@yajo yajo deleted the define-python-buildinputs branch September 22, 2023 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants