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

Jump to bottom

nixUnstable: 2.4pre20201102_550e11f -> 2.4pre20201105_387f824 #102932

Closed
wants to merge 4 commits into from
Closed

nixUnstable: 2.4pre20201102_550e11f -> 2.4pre20201105_387f824 #102932

wants to merge 4 commits into from

Conversation

roberth
Copy link
Member

@roberth roberth commented Nov 5, 2020 •
edited

Motivation for this change

Most notably

  • Fix a memory corruption and deadlock caused by new constant-memory source filtering implementation (+ fix implementation error)
  • Fix nix-copy-closure with drvs
  • Fix a varargs error in an error message (also a crash?)
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@roberth roberth requested a review from grahamc November 5, 2020 14:29
@ofborg ofborg bot requested a review from edolstra November 5, 2020 14:47
@Ma27 Ma27 added the 9.needs: port to stable A PR needs a backport to the stable release. label Nov 6, 2020
@Ma27 Ma27 self-requested a review November 6, 2020 19:51
Ma27
Ma27 reviewed Nov 7, 2020
View reviewed changes
Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

While the change itself seems fine, I'd like to mention that Hydra doesn't build stuff anymore in hydra-queue-runner which can be reproduced by running nixos/tests/hydra on master and on this branch.

Unfortunately I don't have the time right now to look at this, but since Hydra requires nixUnstable these days, this is something that should be taken care of (cc @edolstra).

After a brief check I identified NixOS/nix@c4d903d and NixOS/nix@b43c13a to be the cause, however I acknowledge that those are relevant fixes and I don't know enough about that to patch Hydra accordingly.

@Ma27
Copy link
Member

Ma27 commented Nov 8, 2020

So just realized that with a nix-daemon on latest nix master (i.e. this revision) all started builds stall, even without Hydra making a rollback to a previous configuration necessary.

Hence 👎 on merging this.

@mohe2015
Copy link
Contributor

mohe2015 commented Nov 8, 2020

I agree with the statement from @Ma27

@leotaku leotaku mentioned this pull request Nov 9, 2020
Closed
@roberth
Copy link
Member Author

roberth commented Nov 10, 2020

NixOS/nix#4235 is currently the only remaining issue. I've added a patch for it to make this branch useful for testing.
I don't think we should be patching Nix here, but for now it's useful for those who want to give this PR a spin.

@roberth roberth mentioned this pull request Nov 10, 2020
Open
@roberth roberth mentioned this pull request Nov 11, 2020
Open
@lopsided98 lopsided98 mentioned this pull request Nov 16, 2020
Closed
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/to-flake-or-not-to-flake/10047/5

@roberth roberth mentioned this pull request Nov 19, 2020
Merged
10 tasks
@roberth
Copy link
Member Author

roberth commented Nov 19, 2020

Closing in favor of #104289, in order not to break any pins on this old branch.

@roberth roberth closed this Nov 19, 2020
@erictapen erictapen removed the 9.needs: port to stable A PR needs a backport to the stable release. label Jan 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ma27 Ma27 Ma27 left review comments

@grahamc grahamc Awaiting requested review from grahamc

@edolstra edolstra Awaiting requested review from edolstra

Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@roberth @Ma27 @mohe2015 @nixos-discourse @erictapen