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

Jump to bottom

Add --rev to update-source-version, add a generic unstable updater #101083

Merged
merged 3 commits into from Nov 21, 2020
Merged

Add --rev to update-source-version, add a generic unstable updater #101083

merged 3 commits into from Nov 21, 2020

Conversation

fgaz
Copy link
Member

@fgaz fgaz commented Oct 19, 2020 •
edited

Motivation for this change

Unstable packages are tedious to keep up to date. You have to manually check the repository for new commits, update the version to HEAD's commit date, and update the revision to HEAD's sha.

This patch adds a --rev parameter to update-source-version (necessary since in unstable packages version and rev are independent), and a generic unstable updater to handle the case described above.

Finally, in the last commit, the updater is applied to a package, qbe, that benefits from it, and on which you can test the updater.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@fgaz fgaz requested a review from jtojnar as a code owner October 19, 2020 18:04
@fgaz fgaz added the 6.topic: updaters Tooling for (semi-)automated updating of packages label Oct 19, 2020
jtojnar
jtojnar reviewed Oct 19, 2020
View reviewed changes
Copy link
Contributor

@jtojnar jtojnar left a comment •
edited

Choose a reason for hiding this comment

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

I like the u-s-v change.

The update script itself looks fine. But we might want to implement it using genericUpdater, cc @romildo. Though it might need changes before it is usable (e.g. make it more flexible, do not create fileForGitCommands now that update.nix supports auto-comitting).

pkgs/common-updater/unstable-updater.nix Outdated Show resolved Hide resolved
pkgs/common-updater/unstable-updater.nix Outdated Show resolved Hide resolved
pkgs/common-updater/unstable-updater.nix Outdated

# update the nix expression
${common-updater-scripts}/bin/update-source-version "$attr_path" \
"unstable-$commit_date" \
Copy link
Contributor

Choose a reason for hiding this comment

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

Relevant: #100833

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure whether to leave it as is until #100833 is in, or to make it configurable by taking a makeVersionString function as an argument (with default date → "unstable-$date")

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I'll leave it as is for now

pkgs/common-updater/unstable-updater.nix Outdated Show resolved Hide resolved
pkgs/top-level/all-packages.nix Outdated
@@ -94,6 +94,8 @@ in

genericUpdater = callPackage ../common-updater/generic-updater.nix { };

unstableUpdater = callPackage ../common-updater/unstable-updater.nix { };
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably should make the name signal that it is git specific.

Copy link
Member Author

Choose a reason for hiding this comment

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

I based the name on genericUpdater. Should I rename that one too or is it too late?

Copy link
Contributor

Choose a reason for hiding this comment

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

genericUpdater is quite fresh an undocumented so I do not see an issue with renaming it. We can leave it for a separate PR, though, if we decide to not use it now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Member Author

Choose a reason for hiding this comment

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

I named it "unstableGitUpdater", not sure if it's clear enough

@fgaz
Copy link
Member Author

fgaz commented Oct 20, 2020 •
edited

@jtojnar

we might want to implement it using genericUpdater

I considered that, but

  • it would require at least a versionSorter, a versionFilter, maybe a makeVersion parameters
  • and/or a way to trigger the --rev behavior
    • which is pretty much specific to unstable versions
  • unstableUpdater can make shallow clones, potentially using much less resources (edit: nevermind, we probably get a full clone with the later commands anyway)
  • unstableUpdater is quite simple on its own

@fgaz
Copy link
Member Author

fgaz commented Nov 20, 2020

Ping

@jtojnar
Copy link
Contributor

jtojnar commented Nov 20, 2020 •
edited

Could you autosquash the fixup commits and fix the evaluation?

@fgaz
common-updater-scripts: add --rev to update-source-version
4d8e0f9 
Adds a --rev=<revision> parameter to the script that makes it possible
to explicitly specify a new revision.
Useful to update unstable packages, where the version and revision may
be independent.
@fgaz
unstableUpdater: init
3f20319
@fgaz
qbe: use unstableUpdater
97e5fc3
@jtojnar jtojnar merged commit a0efbc1 into NixOS:master Nov 21, 2020
@jtojnar
Copy link
Contributor

jtojnar commented Nov 21, 2020

Looks good, thanks.

@fgaz fgaz deleted the unstable-updater branch November 21, 2020 14:51
@fgaz fgaz mentioned this pull request Dec 2, 2020
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtojnar jtojnar jtojnar left review comments

Assignees
No one assigned
Labels
6.topic: updaters Tooling for (semi-)automated updating of packages 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@fgaz @jtojnar