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

update.nix: Run update scripts in parallel #50977

Merged
merged 2 commits into from Dec 2, 2018

Conversation

jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Nov 24, 2018

Motivation for this change

nix-shell maintainers/scripts/update.nix --argstr path gnome3 is just too slow the old way.

cc @garbas @matthewbauer @hedning

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

@GrahamcOfBorg GrahamcOfBorg added 6.topic: GNOME GNOME desktop environment and its underlying platform 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Nov 24, 2018
@FRidh
Copy link
Member

FRidh commented Nov 24, 2018

Somewhat related, in #50983 I implement support in case of Python packages.

@FRidh
Copy link
Member

FRidh commented Nov 24, 2018

If I combine this PR and my PR, and run the following:

$ nix-shell maintainers/scripts/update.nix --argstr package python3.pkgs.scikit-bio

Going to be running update for following packages:
 - python3.7-scikit-bio-0.5.4

Press Enter key to continue...

Running update for:
 - python3.7-scikit-bio-0.5.4: UPDATING ...
Traceback (most recent call last):
  File "/nix/store/wvs5gqzwfqgb0165b1yixpx02vq3r1kh-update.py", line 66, in <module>
    main(args.max_workers, args.keep_going, args.packages)
  File "/nix/store/wvs5gqzwfqgb0165b1yixpx02vq3r1kh-update.py", line 35, in main
    future.result()
  File "/nix/store/y78kh4c49qz3m8al5mmxqxanncfs4x0h-python3-3.7.1/lib/python3.7/concurrent/futures/_base.py", line 425, in result
    return self.__get_result()
  File "/nix/store/y78kh4c49qz3m8al5mmxqxanncfs4x0h-python3-3.7.1/lib/python3.7/concurrent/futures/_base.py", line 384, in __get_result
    raise self._exception
  File "/nix/store/y78kh4c49qz3m8al5mmxqxanncfs4x0h-python3-3.7.1/lib/python3.7/concurrent/futures/thread.py", line 57, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/nix/store/wvs5gqzwfqgb0165b1yixpx02vq3r1kh-update.py", line 11, in run_update_script
    subprocess.run(package['updateScript'], stdout=subprocess.PIPE, stderr=subprocess.STDOUT, check=True)
  File "/nix/store/y78kh4c49qz3m8al5mmxqxanncfs4x0h-python3-3.7.1/lib/python3.7/subprocess.py", line 466, in run
    with Popen(*popenargs, **kwargs) as process:
  File "/nix/store/y78kh4c49qz3m8al5mmxqxanncfs4x0h-python3-3.7.1/lib/python3.7/subprocess.py", line 769, in __init__
    restore_signals, start_new_session)
  File "/nix/store/y78kh4c49qz3m8al5mmxqxanncfs4x0h-python3-3.7.1/lib/python3.7/subprocess.py", line 1516, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
OSError: [Errno 8] Exec format error: '/nix/store/isf8fnzhhid3gm2nd6ajyjmgw2daj7j9-update-python'

def run_update_script(package):
print(f" - {package['name']}: UPDATING ...", file=sys.stderr)

subprocess.run(package['updateScript'], stdout=subprocess.PIPE, stderr=subprocess.STDOUT, check=True)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
subprocess.run(package['updateScript'], stdout=subprocess.PIPE, stderr=subprocess.STDOUT, check=True)
subprocess.run(package['updateScript'], stdout=subprocess.PIPE, stderr=subprocess.STDOUT, check=True, shell=True)

Currently it is possible to not just pass in a script, but also arguments.

Copy link
Member

Choose a reason for hiding this comment

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

Note this solved the issue I had.

Copy link
Contributor Author

@jtojnar jtojnar Nov 24, 2018

Choose a reason for hiding this comment

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

Would not it be better to add shebang to the update-python script:

https://github.com/NixOS/nixpkgs/pull/50983/files#r236044493

Copy link
Member

Choose a reason for hiding this comment

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

That's another way. In any case, the interface needs to be documented.

Copy link
Member

Choose a reason for hiding this comment

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

Just to be clear, I am not saying we should have it with arguments. If we need to create a derivation for every update we may as well have arguments in there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Allowing lists will allow us to avoid needing derivations per package.

@jtojnar
Copy link
Contributor Author

jtojnar commented Nov 25, 2018

Added the docs and extra args to update.nix.

@jtojnar
Copy link
Contributor Author

jtojnar commented Nov 25, 2018

One thing I do not like about this is that KeyboardInterrupt does not work very well with threads.

FRidh
FRidh previously requested changes Nov 30, 2018
Copy link
Member

@FRidh FRidh left a comment

Choose a reason for hiding this comment

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

Please update the commit message describing the relevant changes:

  • runs in parallel
    Change in semantics:
  • update script needs to be an executable
  • updateScript is a list

To make updating large attribute sets faster, the update scripts
are now run in parallel.

Please note the following changes in semantics:

- The string passed to updateScript needs to be a path to an executable file.
- The updateScript can also be a list: the tail elements will then be passed
  to the head as command line arguments.
@jtojnar jtojnar merged commit 4920f0c into NixOS:master Dec 2, 2018
@jtojnar jtojnar deleted the parallel-update.nix branch December 2, 2018 00:23
@jtojnar jtojnar mentioned this pull request Jan 17, 2019
6 tasks
@jtojnar jtojnar added the 6.topic: updaters Tooling for (semi-)automated updating of packages label Jun 2, 2019
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

3 participants