Stdenv setup: Process dependencies before dependents #31414

Ericson2314 · 2017-11-09T01:20:29Z

Motivation for this change

This is intended to be reviewed commit-by-commit.

This does 2 things

Change the dependency order. Before, we did the current node before children, now we do the children before current node. Were dependencies forming a tree, this would simply be the inverse traversal, but since they in fact form a DAG, it's a bit more subtle. The opposite of the new invariant "dependencies before dependents" wasn't true: dependents weren't always processed first because it was possible that a dependency was already visited via another dependent.

This is an observable change because the processing we are doing does not commute: arbitrary bash setup hooks certainly don't, and PATH concatenation doesn't either. I do think having this invariant is valuable, though, and rely on it to simplify cc-wrapper's setup hook.
Cyclic dependencies are now an error. Yes, they are only possible in absurd cases, but possible nonetheless:
```
└── /nix/store/asdfasdfasdf-foobar/
    ├── foo
    │   └── nix-support
    │       └── propagated-build-inputs = /nix/store/asdfasdfasdf-foobar/bar
    └── bar
        └── nix-support
            └── propagated-build-inputs = /nix/store/asdfasdfasdf-foobar/foo
```
(foo and bar are cyclic). The old algo would silently work with this; the new one would naively diverge, so I made it error out instead as such a thing should never be intended. But if we agree this shouldn't happen in practice so infinite recursion is a good enough error, I can simplify things

Things done

~~Hoping to merge this Monday 2017-11-13 at the latest :). Thanks!~~

Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
Built stdenv on platform(s)
- NixOS -- latest in progress, see nixborg build
- macOS -- latest in progress
- 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/)
Fits CONTRIBUTING.md.

CC @bgamari

copumpkin · 2017-11-09T01:37:16Z

This seems sensible in principle, but can we get a Hydra job showing what the implications might be? Subtly changing behavior (that arguably nobody should rely on the order of, but people probably do) makes me very uncomfortable even if it does make sense.

globin · 2017-11-09T08:43:59Z

@nixborg build

nixborg · 2017-11-09T08:44:15Z

Jobset created at https://hydra.mayflower.de/jobset/nixos/pr-31414

orivej · 2017-11-09T10:40:16Z

pkgs/stdenv/generic/setup.sh

+}
+
+# Add package to the future PATH and run setup hooks
+activatePackage() {


Who calls this function?

Ericson2314 · 2017-11-09T15:44:13Z

pkgs/stdenv/generic/setup.sh

@@ -359,15 +389,22 @@ if [ -z "${crossConfig:-}" ]; then
             ${propagatedNativeBuildInputs:-} ${propagatedBuildInputs:-}; do
        findInputs "$i" nativePkgs propagated-native-build-inputs
    done
+    for i in "${nativePkgs[@]}"; do
+        activatePackage "$i"


@orivej here

Ericson2314 · 2017-11-09T15:44:39Z

pkgs/stdenv/generic/setup.sh

    for i in ${nativeBuildInputs:-} ${defaultNativeBuildInputs:-} ${propagatedNativeBuildInputs:-}; do
        findInputs "$i" nativePkgs propagated-native-build-inputs
    done
+    for i in "${nativePkgs[@]}"; do
+        activatePackage "$i"


@orivej And here

Ericson2314 · 2017-11-09T17:47:55Z

@globin is there any good known-good jobsest to compare the nixborg one to?

Ericson2314 · 2017-11-09T23:58:37Z

Oh hmm, this breaks down when different dependencies are in fact the same derivation. I'm going to have to think about that.

orivej · 2017-11-10T00:23:13Z

Could you give an example in nixpkgs?

Ericson2314 · 2017-11-10T18:26:07Z

@orivej Nevermind. The already-visited logic is based on actual paths, I just mistakenly changed the iteration order. [I did have to make _PATH be prepended instead of appended, but that is expected.]

There's still a theoretical issue where "unrelated" deps could clobber each other on the PATH in different ways based on different topological sorts, and our depth-first one might not have the most intuitive behavior in that regard, but I won't worry about this unless it shows up in practice.

I added a cc-wrapper commit from my #29396 branch that actually leverages this, to make sure everything works alright.

orivej

Looks good. https://hydra.mayflower.de/jobset/nixos/pr-31414 did not pick your changes yet… And since AFAIK it does not contribute to the nixos cache, shouldn't this PR go to staging first?

globin · 2017-11-11T12:36:49Z

@nixborg build

nixborg · 2017-11-11T12:38:08Z

Jobset created at https://hydra.mayflower.de/jobset/nixos/pr-31414

This will be very useful for bootstrapping, eventually.

…d of strings This is generally cleaner: less eval, less worrying about separators, and probably also faster. I got the idea from that python wrapper script.

FRidh · 2019-07-20T11:43:04Z

I like the motivation behind this change but because nothing is happening I am closing this.

JohnAZoidberg · 2019-08-10T00:36:34Z

This issue is mentioned in nixpkgs stdenv: https://github.com/nixos/nixpkgs/blob/master/pkgs/stdenv/generic/setup.sh#L1167

Ericson2314 · 2019-08-10T01:08:20Z

Yes maybe a GitHub issue rather than open PR canbe made for that.

Ericson2314 · 2021-08-01T16:32:01Z

Reopning and marking as draft because I ought to finish it at some point.

Ericson2314 · 2021-08-02T16:02:48Z

Should be possible to finish after #132340 is the default and cc-wrapper is simplified.

stale · 2022-04-19T08:16:06Z

I marked this as stale due to inactivity. → More info

Ericson2314 requested review from copumpkin, dezgeg, LnL7 and edolstra November 9, 2017 01:20

Ericson2314 added 1.severity: mass-darwin-rebuild 1.severity: mass-rebuild labels Nov 9, 2017

GrahamcOfBorg added 10.rebuild-darwin: 501+ 10.rebuild-linux: 501+ 10.rebuild-darwin-stdenv 10.rebuild-linux-stdenv labels Nov 9, 2017

orivej reviewed Nov 9, 2017

View reviewed changes

Ericson2314 commented Nov 9, 2017

View reviewed changes

orivej approved these changes Nov 9, 2017

View reviewed changes

Ericson2314 added this to Needed for binutils-wrapper in Cross compilation Nov 9, 2017

Ericson2314 force-pushed the stdenv-toposort branch from 869bacc to 714b5f2 Compare November 10, 2017 18:15

orivej changed the base branch from master to staging November 10, 2017 19:49

orivej changed the base branch from staging to master November 10, 2017 19:51

orivej approved these changes Nov 10, 2017

View reviewed changes

Ericson2314 changed the base branch from master to staging November 10, 2017 20:58

Ericson2314 mentioned this pull request Nov 10, 2017

bfd, opcodes: Init separate derivations for binutils libraries #30484

Merged

8 tasks

Ericson2314 force-pushed the stdenv-toposort branch from 714b5f2 to 57e9312 Compare November 13, 2017 04:24

Ericson2314 changed the base branch from staging to master December 20, 2017 20:38

Ericson2314 changed the base branch from master to staging December 20, 2017 20:38

Ericson2314 moved this from After big PR to Perhaps not priority for 18.03 in Cross compilation Jan 2, 2018

7c6f434c added the 11.by: nixpkgs-member label Mar 10, 2018

Ericson2314 mentioned this pull request Apr 20, 2018

PATH should prioritize user defined packages #38657

Open

Ericson2314 mentioned this pull request May 7, 2018

treewide: Modular setup hooks #40139

Merged

8 tasks

Ericson2314 referenced this pull request Jul 27, 2018

libgcc: Init at 7.3.0

310cb76

This will be very useful for bootstrapping, eventually.

FRidh closed this Jul 20, 2019

Ericson2314 reopened this Aug 1, 2021

github-actions bot added 6.topic: emacs 6.topic: erlang 6.topic: golang 6.topic: ocaml 6.topic: python 6.topic: qt/kde 6.topic: ruby 6.topic: stdenv Standard environment 6.topic: TeX Issues regarding texlive and TeX in general labels Aug 1, 2021

Ericson2314 marked this pull request as draft August 1, 2021 16:31

ofborg bot added the 2.status: merge conflict label Aug 1, 2021

Ericson2314 mentioned this pull request Feb 17, 2022

cc-wrapper: pass always -B and -L together to avoid mixing parts of libc #158047

Merged

7 tasks

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Apr 19, 2022

infinisil mentioned this pull request Aug 30, 2022

Clean up C compilers and stdenv #295038

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stdenv setup: Process dependencies before dependents #31414

Stdenv setup: Process dependencies before dependents #31414

Ericson2314 commented Nov 9, 2017 •

edited

copumpkin commented Nov 9, 2017

globin commented Nov 9, 2017

nixborg commented Nov 9, 2017

orivej Nov 9, 2017

Ericson2314 Nov 9, 2017

Ericson2314 Nov 9, 2017

Ericson2314 commented Nov 9, 2017

Ericson2314 commented Nov 9, 2017

orivej commented Nov 10, 2017

Ericson2314 commented Nov 10, 2017

orivej left a comment

globin commented Nov 11, 2017

nixborg commented Nov 11, 2017

FRidh commented Jul 20, 2019

JohnAZoidberg commented Aug 10, 2019

Ericson2314 commented Aug 10, 2019

Ericson2314 commented Aug 1, 2021

Ericson2314 commented Aug 2, 2021

stale bot commented Apr 19, 2022

Stdenv setup: Process dependencies before dependents #31414

Are you sure you want to change the base?

Stdenv setup: Process dependencies before dependents #31414

Conversation

Ericson2314 commented Nov 9, 2017 • edited

Motivation for this change

Things done

copumpkin commented Nov 9, 2017

globin commented Nov 9, 2017

nixborg commented Nov 9, 2017

orivej Nov 9, 2017

Choose a reason for hiding this comment

Ericson2314 Nov 9, 2017

Choose a reason for hiding this comment

Ericson2314 Nov 9, 2017

Choose a reason for hiding this comment

Ericson2314 commented Nov 9, 2017

Ericson2314 commented Nov 9, 2017

orivej commented Nov 10, 2017

Ericson2314 commented Nov 10, 2017

orivej left a comment

Choose a reason for hiding this comment

globin commented Nov 11, 2017

nixborg commented Nov 11, 2017

FRidh commented Jul 20, 2019

JohnAZoidberg commented Aug 10, 2019

Ericson2314 commented Aug 10, 2019

Ericson2314 commented Aug 1, 2021

Ericson2314 commented Aug 2, 2021

stale bot commented Apr 19, 2022

Ericson2314 commented Nov 9, 2017 •

edited