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

Projects
None yet
3 participants
@jtojnar
Copy link
Contributor

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.

@FRidh

This comment has been minimized.

Copy link
Member

FRidh commented Nov 24, 2018

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

@FRidh

This comment has been minimized.

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)

This comment has been minimized.

@FRidh

FRidh Nov 24, 2018

Member
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.

This comment has been minimized.

@FRidh

FRidh Nov 24, 2018

Member

Note this solved the issue I had.

This comment has been minimized.

@jtojnar

jtojnar Nov 24, 2018

Author Contributor

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

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

This comment has been minimized.

@FRidh

FRidh Nov 24, 2018

Member

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

This comment has been minimized.

@FRidh

FRidh Nov 25, 2018

Member

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.

This comment has been minimized.

@jtojnar

jtojnar Nov 25, 2018

Author Contributor

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

@jtojnar

This comment has been minimized.

Copy link
Contributor Author

jtojnar commented Nov 25, 2018

Added the docs and extra args to update.nix.

@jtojnar jtojnar force-pushed the jtojnar:parallel-update.nix branch 2 times, most recently Nov 25, 2018

@jtojnar

This comment has been minimized.

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 approved these changes Nov 30, 2018

@FRidh
Copy link
Member

FRidh left a comment

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

@jtojnar jtojnar force-pushed the jtojnar:parallel-update.nix branch Dec 1, 2018

update.nix: Run update scripts in parallel
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 force-pushed the jtojnar:parallel-update.nix branch to 59a94b5 Dec 1, 2018

commit message updated

@FRidh

FRidh approved these changes Dec 1, 2018

@jtojnar jtojnar merged commit 4920f0c into NixOS:master Dec 2, 2018

9 checks passed

grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details

@jtojnar jtojnar deleted the jtojnar:parallel-update.nix branch Dec 2, 2018

@jtojnar jtojnar referenced this pull request Jan 17, 2019

Merged

pantheon: init at 5.0 #48637

6 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.