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

update.nix: Run update scripts in parallel #50977

Merged
merged 2 commits into from
Dec 2, 2018
Merged

update.nix: Run update scripts in parallel #50977

merged 2 commits into from
Dec 2, 2018
+169 −50

Conversation

jtojnar
Copy link
Member

@jtojnar jtojnar commented Nov 24, 2018 •
edited
Loading

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.

Sorry, something went wrong.

@GrahamcOfBorg GrahamcOfBorg added 6.topic: GNOME GNOME desktop environment and its underlying platform 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux 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'

FRidh
FRidh reviewed Nov 24, 2018
View reviewed changes
maintainers/scripts/update.py
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
Member Author

@jtojnar jtojnar Nov 24, 2018 •
edited
Loading

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

This was referenced Nov 24, 2018
Merged
Closed
@jtojnar jtojnar force-pushed the parallel-update.nix branch 2 times, most recently from e48d4c0 to 8f56e35 Compare November 25, 2018 20:08
@GrahamcOfBorg GrahamcOfBorg added the 8.has: documentation This PR adds or changes documentation label Nov 25, 2018
@jtojnar
Copy link
Member Author

jtojnar commented Nov 25, 2018

Added the docs and extra args to update.nix.

@jtojnar jtojnar force-pushed the parallel-update.nix branch 2 times, most recently from 4135a29 to d8c572d Compare November 25, 2018 20:59
@GrahamcOfBorg GrahamcOfBorg added 10.rebuild-linux: 1-10 and removed 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Nov 25, 2018
@jtojnar
Copy link
Member 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
View reviewed changes
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

@jtojnar jtojnar force-pushed the parallel-update.nix branch from d8c572d to 6749e10 Compare December 1, 2018 13:55
@jtojnar
update.nix: Run update scripts in parallel
59a94b5 
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 parallel-update.nix branch from 6749e10 to 59a94b5 Compare December 1, 2018 18:18
@jtojnar jtojnar dismissed FRidh’s stale review December 1, 2018 18:19

commit message updated

@jtojnar jtojnar merged commit 4920f0c into NixOS:master Dec 2, 2018
@jtojnar jtojnar deleted the parallel-update.nix branch December 2, 2018 00:23
@pbogdan pbogdan mentioned this pull request Dec 3, 2018
Merged
11 tasks
@jtojnar jtojnar mentioned this pull request Jan 17, 2019
Merged
6 tasks
@jtojnar jtojnar mentioned this pull request Mar 13, 2019
Closed
@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
Reviewers

@FRidh FRidh

Assignees
No one assigned
Labels
6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: updaters Tooling for (semi-)automated updating of packages 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jtojnar @FRidh @GrahamcOfBorg