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: Add support for auto-commiting changes #98304

Merged
merged 14 commits into from Oct 2, 2020

Conversation

@jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Sep 19, 2020

This replaces #59372 and fixes #57609.

I decided to abandon the asyncio_pool library since it seemed buggy and just went with plain asyncio from standard library.
I also managed to figure out how to make the updates work without any changes to passthru.updateScript necessary.

How to best use this to update GNOME

  1. Choose a base commit, for example channels/nixos-unstable branch.
  2. Run the tool nix-shell maintainers/scripts/update.nix --argstr maintainer jtojnar --argstr commit true
  3. Read the changelogs spit out by nix run github:jtojnar/what-changed between-commits <base-commit> (currently only support projects hosted on GNOME servers)
  4. Fix expressions that do not build and then use git commit --fixup <package>Tab to create a fixup commit. (Or --squash to be able to add extra content to the old commit message.)
  5. Push to gnome-3.38 branch
  6. Before merging, run git rebase --interactive --autosquash <base-commit>
jtojnar added 2 commits Apr 12, 2019
Not sure why I chose ProcessPoolExecutor in the first place.
Printing the changed file and new version can be used to commit the changes to git.
@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Sep 20, 2020

@jtojnar Awesome, will test 👍

@worldofpeace worldofpeace requested a review from FRidh Sep 20, 2020
@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Sep 20, 2020

Just for giggles I wanted to see what would happen if I tried to update a single package that I know has no upstream changes. This produced an interesting behavior:

nix-shell maintainers/scripts/update.nix --argstr path pantheon.elementary-files --argstr commit true
this derivation will be built:
  /nix/store/xqjpibcfa3gz2rgkgp87z76rxfyb4vys-packages.json.drv
these 5 paths will be fetched (1.86 MiB download, 7.80 MiB unpacked):
  /nix/store/5kgdnx38svikwlb50w1fr5187wmikisj-nix-update-0.1
  /nix/store/8p701nmi1papb2wj2z0d9kcgfk6pxsi3-nix-2.3.7
  /nix/store/g2hvjwqbii89p4c23r8g85bq8nh2lpxv-nix-2.3.7-man
  /nix/store/j6bj9lrn1gzn9kznv1x1dzbpz0bbsgfm-nix-prefetch-0.3.1
  /nix/store/xxyc21hh94nsr5rzmbhv29bz23zqah34-busybox-1.31.1-x86_64-unknown-linux-musl
copying path '/nix/store/xxyc21hh94nsr5rzmbhv29bz23zqah34-busybox-1.31.1-x86_64-unknown-linux-musl' from 'https://cache.nixos.org'...
copying path '/nix/store/g2hvjwqbii89p4c23r8g85bq8nh2lpxv-nix-2.3.7-man' from 'https://cache.nixos.org'...
copying path '/nix/store/8p701nmi1papb2wj2z0d9kcgfk6pxsi3-nix-2.3.7' from 'https://cache.nixos.org'...
copying path '/nix/store/j6bj9lrn1gzn9kznv1x1dzbpz0bbsgfm-nix-prefetch-0.3.1' from 'https://cache.nixos.org'...
copying path '/nix/store/5kgdnx38svikwlb50w1fr5187wmikisj-nix-update-0.1' from 'https://cache.nixos.org'...
building '/nix/store/xqjpibcfa3gz2rgkgp87z76rxfyb4vys-packages.json.drv'...

Going to be running update for following packages:
 - elementary-files-4.5.0

Press Enter key to continue...

Running update for:
Preparing worktree (new branch 'update-tmp1bx0ebhz')
HEAD is now at ebd6ab9b4a4 maintainers/scripts/update.nix: run update script with UPDATE_NIX_ATTR_PATH
Preparing worktree (new branch 'update-tmpjyt5h2dd')
HEAD is now at ebd6ab9b4a4 maintainers/scripts/update.nix: run update script with UPDATE_NIX_ATTR_PATH
Preparing worktree (new branch 'update-tmpkjzte7m8')
HEAD is now at ebd6ab9b4a4 maintainers/scripts/update.nix: run update script with UPDATE_NIX_ATTR_PATH
Preparing worktree (new branch 'update-tmpd25vhhi2')
HEAD is now at ebd6ab9b4a4 maintainers/scripts/update.nix: run update script with UPDATE_NIX_ATTR_PATH
 - elementary-files-4.5.0: UPDATING ...
 - elementary-files-4.5.0: DONE, no changes.
...

(I should also be wondering how that depended on busybox-1.31.1-x86_64-unknown-linux-musl ???)

  • It appears to have created 4 worktrees?
    I did check to see if the package was updated in all 4 worktrees and it was in only one.

  • It wouldn't exit, I had to ctrl+c for it stop and cleanup worktrees

I then tried it with the gnome update script

nix-shell maintainers/scripts/update.nix --argstr path gnome3.nautilus --argstr commit true

It produced the same behavior where I had to ctrl+c, but when I did that I could see the update applied to the branch.

@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Sep 20, 2020

It appears to have created 4 worktrees?
I did check to see if the package was updated in all 4 worktrees and it was in only one.

Oh, I guess this is expected even if you update a single package

parser.add_argument('--max-workers', '-j', dest='max_workers', type=int, help='Number of updates to run concurrently', nargs='?', default=4)

and https://github.com/NixOS/nixpkgs/pull/98304/files#diff-ef694abe116c19d066d3c003b85eadc9R146.

@doronbehar
Copy link
Contributor

@doronbehar doronbehar commented Sep 20, 2020

These worktree related messages are certainly sort of confusing. Would it be possible to hide them?

I ran:

nix-shell maintainers/scripts/update.nix --argstr package gotify-server --argstr commit true

And currently the command hangs with this diff:

diff --git i/pkgs/servers/gotify/vendor-sha.nix w/pkgs/servers/gotify/vendor-sha.nix
index f9e648a957e..e16c76dff88 100644
--- i/pkgs/servers/gotify/vendor-sha.nix
+++ w/pkgs/servers/gotify/vendor-sha.nix
@@ -1 +1 @@
-"1in4gzmrgb6z7p5fnz33f88g5l0vki2xlxlllk5wy9icp4h3h9sd"
+""

And these messages by update.py:

 - gotify-server-2.0.18: UPDATING ...
 - gotify-server-2.0.18: DONE, no changes.
@jtojnar
Copy link
Contributor Author

@jtojnar jtojnar commented Sep 20, 2020

Thanks for testing.

  1. musl might be dependency of static Nix?
  2. I will adjust it to only create at most as many workers as there are packages.
  3. I was running it with gnome3 path and did not have the patience to run the updates till the end 😸 Will add termination condition.
  4. The issue with gotify is that update.nix will get passthru.updateScript in the main repo so $0 the script uses will point there as well. Ran a few tests and replacing the output of git rev-parse --show-toplevel should work:
$ mkcd /tmp
$ mkdir foo
$ ln -s foo bar
$ cd bar
$ git init
Initialized empty Git repository in /tmp/foo/.git/
$ git rev-parse --show-toplevel
/tmp/foo
$ echo '#!/usr/bin/env bash' > script.sh
$ echo 'echo "$(dirname "$0")"' >> script.sh
$ echo 'echo "$(dirname "${BASH_SOURCE[0]}")"' >> script.sh
$ echo 'echo "$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )"' >> script.sh # https://stackoverflow.com/a/246128/160386
$ chmod +x script.sh
$ /tmp/foo/script.sh
/tmp/foo
/tmp/foo
/tmp/foo
$ /tmp/bar/script.sh
/tmp/bar
/tmp/bar
/tmp/bar
$ ./script.sh
.
.
/tmp/bar
$ nix-instantiate --expr 'builtins.toString ./script.sh' --eval
"/tmp/foo/script.sh"
jtojnar added a commit to jtojnar/nixpkgs that referenced this pull request Sep 20, 2020
`update.nix` extracts `passthru.updateScript` attributes in the main repo
and when they are relative paths (e.g. `./update.sh`), Nix will resolve them
to absolute paths in the main repo.

Update scripts can use $(dirname $0) to get the location of files they
should update but that would point to the main repo.
We want them to modify the appropriate git worktree instead
so we replace the prefix accordingly.

`git rev-parse --show-toplevel` will resolve symlinks but, fortunately,
Nix will do that as well, so the path will match:

NixOS#98304 (comment)
@jtojnar jtojnar force-pushed the jtojnar:updateScript-commit3 branch from 7c00ae7 to 8071955 Sep 20, 2020
Copy link
Contributor

@doronbehar doronbehar left a comment

Other then that, code looks good and it worked for me after testing gotify-server and zoom-us.

supportedFeatures = package.updateScript.supportedFeatures or [];
attrPath = package.updateScript.attrPath or attrPath;
Comment on lines +151 to +152

This comment has been minimized.

@doronbehar

doronbehar Sep 20, 2020
Contributor

It'd be nice if this were documented.

This comment has been minimized.

@jtojnar

jtojnar Sep 20, 2020
Author Contributor

I intentionally removed the documentation in 4bc26cf so that people do not rely on it yet. I am weighting whether the added complexity is worth it and reserve the right to remove it.

@ofborg ofborg bot removed the 6.topic: GNOME label Sep 20, 2020
jtojnar added 6 commits Apr 12, 2019
Update scripts can now declare features using

	passthru.updateScript = {
	  command = [ ../../update.sh pname ];
	  supportedFeatures = [ "commit" ];
	};

A `commit` feature means that when the update script finishes successfully,
it will print a JSON list like the following:

	[
	  {
	    "attrPath": "volume_key",
	    "oldVersion": "0.3.11",
	    "newVersion": "0.3.12",
	    "files": [
	      "/path/to/nixpkgs/pkgs/development/libraries/volume-key/default.nix"
	    ]
	  }
	]

and data from that will be used when update.nix is run with --argstr commit true
to create commits.

We will create a new git worktree for each thread in the pool and run the update
script there. Then we will commit the change and cherry pick it in the main repo,
releasing the worktree for a next change.
Get rid of some globals, split main into smaller functions, rename some variables, add typehints.
This will make it cleaner and also better respect SIGTERM.
…trPath

Instead of having the updateScript support returning JSON object,
it should be sufficient to specify attrPath in passthru.updateScript.
It is much easier to use.

The former is now considered experimental.
…utes

We can determine all of them when attrPath is present so we might jsut as well do it.
jtojnar added 5 commits Sep 19, 2020
…mmitted
We no longer need it for most use cases so I am making it experimental.

I have something in mind where it might be useful in the future (customizing commit messages)
but for now, it would only confuse people.
…R_PATH

The environment variable will contain the attribute path the script is supposed to update.
`update.nix` extracts `passthru.updateScript` attributes in the main repo
and when they are relative paths (e.g. `./update.sh`), Nix will resolve them
to absolute paths in the main repo.

Update scripts can use $(dirname $0) to get the location of files they
should update but that would point to the main repo.
We want them to modify the appropriate git worktree instead
so we replace the prefix accordingly.

`git rev-parse --show-toplevel` will resolve symlinks but, fortunately,
Nix will do that as well, so the path will match:

#98304 (comment)
@jtojnar jtojnar force-pushed the jtojnar:updateScript-commit3 branch from edda38d to 71c246c Sep 20, 2020
- Make some arguments more fitting (the path is actually full, not just relative to prefix…).
- Increase the purity of packages* functions (they now take pkgs from argument, not from scope).
- Add some documentation comments.
@jtojnar jtojnar force-pushed the jtojnar:updateScript-commit3 branch from 6247163 to 74a5bb4 Sep 20, 2020
@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Oct 2, 2020

I tried #98304 (comment) and everything seems to work well 👍

Copy link
Member

@worldofpeace worldofpeace left a comment

I'm at a strong disposition to merge this. I don't want to write commit messages for these anymore 😁

I updated the gnome-3.38 branch with no issues.

@jtojnar jtojnar merged commit 74c5472 into NixOS:master Oct 2, 2020
19 of 20 checks passed
19 of 20 checks passed
tests tests
Details
action
Details
common-updater-scripts, common-updater-scripts.passthru.tests on x86_64-darwin
Details
common-updater-scripts, common-updater-scripts.passthru.tests on aarch64-linux No attempt
Details
Evaluation Performance Report Evaluator Performance Report
Details
Wait for ofborg
Details
common-updater-scripts, common-updater-scripts.passthru.tests on x86_64-linux Success
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="74a5bb4"; rev="74a5bb4041bb4ef001875d5b6b26a2105dc24450"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="74a5bb4"; rev="74a5bb4041bb4ef001875d5b6b26a2105dc24450"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="74a5bb4"; rev="74a5bb4041bb4ef001875d5b6b26a2105dc24450"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="74a5bb4"; rev="74a5bb4041bb4ef001875d5b6b26a2105dc24450"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="74a5bb4"; rev="74a5bb4041bb4ef001875d5b6b26a2105dc24450"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="74a5bb4"; rev="74a5bb4041bb4ef001875d5b6b26a2105dc24450"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="74a5bb4"; rev="74a5bb4041bb4ef001875d5b6b26a2105dc24450"; } ./pkgs/t
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:updateScript-commit3 branch Oct 2, 2020
dawidsowa added a commit to dawidsowa/nixpkgs that referenced this pull request Oct 11, 2020
`update.nix` extracts `passthru.updateScript` attributes in the main repo
and when they are relative paths (e.g. `./update.sh`), Nix will resolve them
to absolute paths in the main repo.

Update scripts can use $(dirname $0) to get the location of files they
should update but that would point to the main repo.
We want them to modify the appropriate git worktree instead
so we replace the prefix accordingly.

`git rev-parse --show-toplevel` will resolve symlinks but, fortunately,
Nix will do that as well, so the path will match:

NixOS#98304 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.