Skip to content

Revert "buildenv: read propagated-user-env-packages line-by-line"#27859

Merged
Ericson2314 merged 1 commit intoNixOS:stagingfrom
adisbladis:revert_27427
Aug 3, 2017
Merged

Revert "buildenv: read propagated-user-env-packages line-by-line"#27859
Ericson2314 merged 1 commit intoNixOS:stagingfrom
adisbladis:revert_27427

Conversation

@adisbladis
Copy link
Member

@adisbladis adisbladis commented Aug 2, 2017 •
edited
Loading

This reverts commit dce958a.

Motivation for this change

This change was originally a fix for 3cb745d which was reverted in b087618

Since the revert dce958a breaks Qt applications.

I don't know if this is the correct fix or if 3cb745d should be reinstated, the commit message in the revert made me lean towards just restoring the old behaviour.
Either solution would work fine, I have tested both.
If that solution is preferable I can close this one and create a new PR.

Things done

Please check what applies. Note that these are not hard requirements but merely serve as information for reviewers.

  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • 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 @ttuegel

@FRidh
Copy link
Member

FRidh commented Aug 2, 2017

@adisbladis I think you are correct that this should be reverted. Note that this needs to go in staging instead.

@FRidh FRidh changed the base branch from master to staging August 2, 2017 08:21
@adisbladis adisbladis mentioned this pull request Aug 2, 2017
Merged
8 tasks
@globin
Copy link
Member

globin commented Aug 2, 2017

ping @Ericson2314

@globin
Copy link
Member

globin commented Aug 2, 2017

Please do NOT merge and wait for @Ericson2314's reply :)

@Ericson2314
Copy link
Member

Ericson2314 commented Aug 2, 2017 •
edited
Loading

Ah thanks. I forgot that @ttuegel fixed my earlier change here when I rolled it back.

I think it would be better to continue reading the entire file while splitting on newline and space alike. See NixOS/nix#1482 for how that's done in Perl. The reason for this is other code (e.g. https://github.com/NixOS/nixpkgs/blob/master/pkgs/stdenv/generic/setup.sh#L309) reads the entire file (even before any of my changes).

[In https://github.com//pull/27427#issuecomment-318737198, @oxij proposes that we switch to \0 delimiting, which is indeed most robust. But we can still do with @oxij's XOR transition strategy: if the file contains null bytes, splt on those, else split on all white-space as I describe above.]

@edolstra Any opinions? Not quite sure where you stand from the Nix PRs.

@FRidh
Copy link
Member

FRidh commented Aug 3, 2017

It's good to come up with a long-term solution, but if that takes time I prefer we roll back as soon as possible and fix issues for our users. We can come up with a long-term solution later.

@Ericson2314
Copy link
Member

Ericson2314 commented Aug 3, 2017 •
edited
Loading

I just pinged @edolstra on IRC. Happy to merge this for now if I don't get a response today. He's on holiday, I'm told.

@Ericson2314 Ericson2314 merged commit f1d48ca into NixOS:staging Aug 3, 2017
@Ericson2314 Ericson2314 mentioned this pull request Aug 3, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@adisbladis @FRidh @globin @Ericson2314