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

build: automate nix package upgrades. Fixes #11691 #12520

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

isubasinghe
Copy link
Member

@isubasinghe isubasinghe commented Jan 14, 2024

Fixes #11691

Motivation

The nix hash is extremely out of date and needs manual updating.

Modifications

A nix-hash task was added to the makefile that patches the nix flake.

Verification

Tested it by mutating the hash and running the nix-hash command.

Caution

This PR will likely result in someone needing to run make codegen on dependabot PRs relating to golang dependencies.
I would be happy to do this.

@isubasinghe isubasinghe changed the title Upgrade nix pkgs fix: upgrade nix pkgs Jan 14, 2024
@isubasinghe isubasinghe changed the title fix: upgrade nix pkgs feat: automate nix package upgrades. Fixes #11691 Jan 21, 2024
@isubasinghe isubasinghe marked this pull request as ready for review January 21, 2024 01:27
@caelan-io
Copy link
Member

Are we okay to merge this one?

Copy link
Contributor

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

I think this makes more sense inside of the lint job, rather than codegen. We already have deps linting there and this does not generate code.
The patch-nix-hash script also needs to be added to changed-files in CI

Also made some small improvements to the script

This PR will likely result in someone needing to run make codegen on dependabot PRs relating to golang dependencies.
I would be happy to do this.

We can automate this similarly to #12234 . Maybe inside of the lint CI job? not sure what the best place might be

hack/patch-nix-hash.sh Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
.github/workflows/ci-build.yaml Outdated Show resolved Hide resolved
hack/patch-nix-hash.sh Show resolved Hide resolved
Copy link
Contributor

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

Can optimize the script a bit as well to be more DRY and not repeat some commands / calculations

hack/patch-nix-hash.sh Outdated Show resolved Hide resolved
echo OK
else
# this is reliant on the vendorHash being on line 195
sed -i '195s/vendorHash = \"\([^\"]*\)\"/vendorHash = ""/g' ./dev/nix/flake.nix
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
sed -i '195s/vendorHash = \"\([^\"]*\)\"/vendorHash = ""/g' ./dev/nix/flake.nix

we can just replace it in the next sed command; no need to run a similar command twice.

Copy link
Member Author

Choose a reason for hiding this comment

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

So this ordering actually matters, this is because the error message for a non present nix hash can be different to the error message for an empty hash.

This means that we can't really get rid of this nor store the output as a variable.

Copy link
Contributor

Choose a reason for hiding this comment

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

But do we actually care to differentiate the error? If there's an error, it needs a manual fix.
I would think we can just do the replace, and if that fails, print a nicer error message that it needs a manual fix and exit 1

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah we do actually care to differentiate the error, only a certain error msg contains the correct nix hash.
We force the correct error output via the first sed command.

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've reduced the if check, and forced this ordering by always doing the first sed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah we do actually care to differentiate the error, only a certain error msg contains the correct nix hash.

We can just print a nicer error ourselves though rather than relying on sed's error, which is not as user-friendly / specific to the script. Unless I'm misunderstanding something?

The current version seems to also unconditionally print the change even if nothing was changed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah we aren't relying on sed's error. Sed is used to force an ordering which gets the correct error output from the nix build command.

Yes the current version unconditionally changes the hash, this is the only way to reduce the number of "nix build" commands issued. We can't just store it unfortunately.

Copy link
Contributor

@agilgur5 agilgur5 Feb 11, 2024

Choose a reason for hiding this comment

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

OH you mean you're intentionally making the nix build command fail in order to pull the vendorHash from its output??
Oh that is wacky I did not expect that.

Yes the current version unconditionally changes the hash

I actually didn't mean the hash, I meant the print / echo. It always says "Changed" even if it didn't change, which is a bit confusing.

Are you not able to:

  1. grep current hash
  2. nix build
  3. grep new hash
  4. Check if it changed, if so, echo

Or is there a reason you can't?

@agilgur5 agilgur5 added type/dependencies PRs and issues specific to updating dependencies area/build Build or GithubAction/CI issues labels Jan 31, 2024
@agilgur5
Copy link
Contributor

agilgur5 commented Feb 1, 2024

feat:

This probably makes more sense as a build: commit as well

@isubasinghe
Copy link
Member Author

isubasinghe commented Feb 3, 2024

This PR will likely result in someone needing to run make codegen on dependabot PRs relating to golang dependencies.
I would be happy to do this.

We can automate this similarly to #12234 . Maybe inside of the lint CI job? not sure what the best place might be

haha god, that works I suppose, not a fan but defs better than manual effort I suppose.

@isubasinghe isubasinghe force-pushed the upgrade-nix-pkgs branch 3 times, most recently from b3acbaa to 4ec5815 Compare February 4, 2024 09:07
@isubasinghe isubasinghe marked this pull request as draft February 4, 2024 09:35
@agilgur5 agilgur5 self-assigned this Feb 4, 2024
Copy link
Contributor

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

mostly nits, although the "check if Nix is installed before running" comment below is pretty important

This is back to draft now, but the commit prefix change and the dependabot automation is also still needed.

hack/patch-nix-hash.sh Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
.github/workflows/ci-build.yaml Outdated Show resolved Hide resolved
@agilgur5 agilgur5 changed the title feat: automate nix package upgrades. Fixes #11691 build: automate nix package upgrades. Fixes #11691 Feb 5, 2024
Signed-off-by: isubasinghe <isitha@pipekit.io>
Signed-off-by: isubasinghe <isitha@pipekit.io>
Signed-off-by: isubasinghe <isitha@pipekit.io>
Signed-off-by: isubasinghe <isitha@pipekit.io>
Signed-off-by: isubasinghe <isitha@pipekit.io>
Signed-off-by: isubasinghe <isitha@pipekit.io>
Signed-off-by: isubasinghe <isitha@pipekit.io>
Signed-off-by: isubasinghe <isitha@pipekit.io>
Signed-off-by: isubasinghe <isitha@pipekit.io>
Signed-off-by: Isitha Subasinghe <isitha@pipekit.io>
Signed-off-by: isubasinghe <isitha@pipekit.io>
Signed-off-by: isubasinghe <isitha@pipekit.io>
Signed-off-by: isubasinghe <isitha@pipekit.io>
Signed-off-by: isubasinghe <isitha@pipekit.io>
Signed-off-by: isubasinghe <isitha@pipekit.io>
Signed-off-by: isubasinghe <isitha@pipekit.io>
@@ -377,12 +379,15 @@ jobs:
timeout-minutes: 15 # must be strictly greater than the timeout in .golangci.yml
env:
GOPATH: /home/runner/go
USE_NIX: true
Copy link
Contributor

@agilgur5 agilgur5 Feb 11, 2024

Choose a reason for hiding this comment

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

mmm this changes if/how/when things get installed, I don't think it should be on during CI (especially if it's not the most common dev env).

In the Makefile you can just check if Nix exists before running ./hack/patch-nix-hash.sh. Like check if the nix command exists, e.g. command -v nix. Much simpler check 😅

steps:
- uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
- uses: actions/setup-go@0c52d547c9bc32b1aa3301fd7a9cb496313a4491 # v5.0.0
with:
go-version: "1.21"
cache: true
- uses: cachix/install-nix-action@6004951b182f8860210c8d6f0d808ec5b1a33d28 #v25 -> nix 2.19.2
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- uses: cachix/install-nix-action@6004951b182f8860210c8d6f0d808ec5b1a33d28 #v25 -> nix 2.19.2
- uses: cachix/install-nix-action@6004951b182f8860210c8d6f0d808ec5b1a33d28 # v25 -> nix 2.19.2

nit: Consistent space after #
(I'm also not sure how many different permutations of the comment dependabot handles)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build Build or GithubAction/CI issues type/dependencies PRs and issues specific to updating dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automate nix dependency hash update
4 participants