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

maintainers/scripts/update.nix: various fixes #62478

Merged
merged 9 commits into from Jun 4, 2019

Conversation

jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Jun 2, 2019

Motivation for this change

For update script parallelization, we have started calling builtins.toJSON on updateScripts, which triggers evaluation of paths and therefore their copying
to Nix store. This breaks update scripts that assume that they exist in nixpkgs
like dwarf-fortress.

#61935

Let’s stringify the paths before JSONification to prevent the evaluation.

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 nix-review --run "nix-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

For update script parallelization, we have started calling builtins.toJSON
on updateScripts, which triggers evaluation of paths and therefore their copying
to Nix store. This breaks update scripts that assume that they exist in nixpkgs
like dwarf-fortress.

NixOS#61935

Let’s stringify the paths before JSONification to prevent the evaluation.
It makes little sense for update.nix to try to update overlays; for most people,
they will point to a read-only repository most of the cases.
Previously we did not handle non-existant paths making the program crash.
Let’s show a proper error.
Make calling update.py a little nicer
@jtojnar
Copy link
Contributor Author

jtojnar commented Jun 2, 2019

The clean up is extracted from #59372

@jtojnar jtojnar added the 6.topic: updaters Tooling for (semi-)automated updating of packages label Jun 2, 2019
Previously, we escaped the old version in place for use in sed commands,
and then had to use that in error messages. We can do better.
No need to try evaluate the URL twice as introduced in the original fix
NixOS@cce4868
grep was not actually properly included by common-updater-scripts before
0105058 but I am not sure why would we ever
need to add coreutils to PATH.
@jtojnar jtojnar changed the title maintainers/scripts/update.nix: Do not store updateScripts maintainers/scripts/update.nix: various fixes Jun 2, 2019
@ofborg ofborg bot added 6.topic: GNOME GNOME desktop environment and its underlying platform 10.rebuild-linux: 1-10 and removed 10.rebuild-linux: 0 labels Jun 2, 2019
Copy link
Member

@lilyball lilyball left a comment

Choose a reason for hiding this comment

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

This all looks pretty good to me.

Make it clear that PWD is not fixed
@jtojnar
Copy link
Contributor Author

jtojnar commented Jun 2, 2019

Added info about PWD.

@jtojnar jtojnar merged commit ea59f09 into NixOS:master Jun 4, 2019
@jtojnar jtojnar deleted the dont-store-updateScripts branch June 4, 2019 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: updaters Tooling for (semi-)automated updating of packages 8.has: documentation 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants