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

stdenv: introduce isBootstrap for conditionals #43439

Closed
wants to merge 1 commit into from

Conversation

LnL7
Copy link
Member

@LnL7 LnL7 commented Jul 12, 2018

Motivation for this change

This is useful for conditionals like optional dependencies that we don't
need when bootstrapping the stdenv, reducing the set of build time
dependencies that are required for a full rebuild.

On darwin it's even easier to pollute the stdenv build closure with extra dependencies because of cmake, python, etc.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Fits CONTRIBUTING.md.

This is useful for conditionals like optional dependencies that we don't
need when bootstrapping the stdenv, reducing the set of build time
dependencies that are required for a full rebuild.
@GrahamcOfBorg
Copy link

Success on aarch64-linux (full log)

Attempted: stdenv

Partial log (click to expand)

/nix/store/s7p3hkplihq7g4njaq1gn77id2bn2aqm-stdenv-linux

@GrahamcOfBorg
Copy link

Success on x86_64-linux (full log)

Attempted: stdenv

Partial log (click to expand)

/nix/store/bcrwa5b8z54xrl95ipfrq6nqhb0530yi-stdenv-linux

@GrahamcOfBorg
Copy link

Success on x86_64-darwin (full log)

Attempted: stdenv

Partial log (click to expand)

/nix/store/7amnc2662p390bgzj6c0snylj19vxmj2-stdenv-darwin

@matthewbauer
Copy link
Member

matthewbauer commented Jul 13, 2018

I think we avoid generic conditionals like this as much as possible. I’m worried that this could be abused too easily (of course something similar is already basically being used for a few things through boot attributes). I feel like the best thing to do is to provide very specific flags like enableSSL in curl, usePython in cmake, or onlyHeaders in Linux. This is easier to understand than something vague like isBootstrap. Then the stdenv would set these in an override or similar. It could also be reusable outside of the stdenv.

Maybe there are good uses of this though. @LnL7 do you have a specific place where this could be used? I guess it may be better than things there now like cmakeBoot, fetchurlBoot, etc.

/cc @Ericson2314

@Ericson2314
Copy link
Member

Ericson2314 commented Jul 13, 2018

Yeah I'm sort of thinking the same thing. I really want to redo the bootstrapping by 18.09 into just two changes, would would change everything anyways.

@LnL7
Copy link
Member Author

LnL7 commented Jul 13, 2018

I think there are 2 different cases for bootstrap overrides.

  1. Change the current stage to use dependencies from the previous stage for things that can't be built yet.
  2. Reduce optional dependencies for things that are enabled by default but not needed for the stdenv.

Do we want the stdenv stages to handle both? The first case is simple since it's a requirement, the second is not but we still want to avoid those to avoid rebuilding dependencies that are never used. The amount of packages the darwin stdenv depends on is significantly larger so it's easier for dependencies to sneak in there.

@LnL7
Copy link
Member Author

LnL7 commented Jul 21, 2018

Here's a good example 2badf9a, libffi is part of the stdenv so this pulls in a bunch of new dependencies.

@matthewbauer
Copy link
Member

matthewbauer commented Jul 21, 2018

Here's a good example 2badf9a, libffi is part of the stdenv so this pulls in a bunch of new dependencies.

Ok good catch. Can we just disable libffi tests in pkgs/stdenv/darwin overrides? We could even break all of these into an isBootstrap overlay.

My thinking on this is basically that we don't want packages to know anything about the bootstrapping process. They can have flags that are only turned on in bootstrapping but they should only be about things the package would already know about (doCheck, disable ncurses, use this package version, etc.)

@LnL7
Copy link
Member Author

LnL7 commented Jul 21, 2018

An alternative I've been thinking about is making the allowed set of packages strict somehow, to make introducing dependencies explicit.

@mmahut
Copy link
Member

mmahut commented Aug 3, 2019

What is the status of this pull request?

@LnL7 LnL7 mentioned this pull request Jan 31, 2020
10 tasks
@stale
Copy link

stale bot commented Jun 1, 2020

Thank you for your contributions.
This has been automatically marked as stale because it has had no activity for 180 days.
If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.
Here are suggestions that might help resolve this more quickly:

  1. Search for maintainers and people that previously touched the
    related code and @ mention them in a comment.
  2. Ask on the NixOS Discourse. 3. Ask on the #nixos channel on
    irc.freenode.net.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 1, 2020
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Oct 3, 2020
@Mindavi
Copy link
Contributor

Mindavi commented May 19, 2021

If I read this correctly there's not much consensus on this, and it doesn't look like anything's going to happen with this. Please re-open or make a new PR if you still want to pursue this.

@Mindavi Mindavi closed this May 19, 2021
@LnL7 LnL7 deleted the stdenv-is-bootstrap branch May 19, 2021 21:16
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.

None yet

7 participants