Skip to content

chore: add ability to include custom protoc-gen-go dependency in nix flake #14728

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
Merged
merged 6 commits into from
Sep 24, 2024
Merged

chore: add ability to include custom protoc-gen-go dependency in nix flake #14728

merged 6 commits into from
Sep 24, 2024

Conversation

joobisb
Copy link
Contributor

@joobisb joobisb commented Sep 19, 2024

This PR can be a starting point to fix #14343

@cdr-bot cdr-bot bot added the community Pull Requests and issues created by the community. label Sep 19, 2024
@joobisb joobisb changed the title feat: ability to add custom protoc-gen-go dependency to nix flake feat: add ability to include custom protoc-gen-go dependency in nix flake Sep 19, 2024
dannykopping
dannykopping reviewed Sep 19, 2024
View reviewed changes
Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

flake.nix Outdated
repo = "protobuf-go";
rev = rev;
# This should be updated whenever rev changes!
# To update, set to "", run nix-shell, insert new hash
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does the new hash come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are a couple of ways to generate the hash, can use either of them

  1. nix-prefetch-git https://github.com/protocolbuffers/protobuf-go --rev v1.30.0
  2. Run nix-shell with sha256 = ""; this will fail to load the shell and output the required hash in the log as seen in the image, use that
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you rather integrate this into scripts/update-flake.sh pls?
The less we make folks think, the more likely they are to do the right thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah that would be better, I will do that

@joobisb
Copy link
Contributor Author

joobisb commented Sep 19, 2024

@dannykopping i've addressed the comments, please have a look

@matifali
Copy link
Member

I agree with the resolution to align nix and CI. But I am afraid in long run this will become a resistance to easily upgrade dev tools and packages.

Why we can't update the version in CI in to match with nix?

Or best use nix to run our CI.

@dannykopping
Copy link
Contributor

Why we can't update the version in CI in to match with nix?

CI is the only one with a pinned version; either way we'd have to do what @joobisb has implemented.

Or best use nix to run our CI.

That's the dream, but it'll take some time. In the interim, we can start by aligning versions IMHO.

@dannykopping
Copy link
Contributor

@joobisb please avoid rebasing once a review has begun. There's no need to maintain a clean history, since we squash-merge anyway.

@joobisb
Copy link
Contributor Author

joobisb commented Sep 20, 2024

@dannykopping I've updated the script

dannykopping
dannykopping reviewed Sep 20, 2024
View reviewed changes
Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

Thanks!

scripts/update-flake.sh Outdated

# Update protoc-gen-go sha256
echo "Updating protoc-gen-go sha256..."
PROTOC_GEN_GO_REV=$(grep -A 20 'pkgs.buildGoModule rec' flake.nix | grep -A 10 'repo = "protobuf-go"' | grep 'rev = "v' | sed 's/.*rev = "\(.*\)".*/\1/')
Copy link
Contributor

@dannykopping dannykopping Sep 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is flimsy (and possibly not portable across Linux/Mac/Windows); is there a more robust way we can get this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dannykopping
found a way to do this using nix eval , please have a look

Refer: https://discourse.nixos.org/t/how-to-evaluate-a-nix-value-from-a-shell-script/24296/3

@joobisb
Copy link
Contributor Author

joobisb commented Sep 24, 2024

@dannykopping I've addressed the comments, could you please have a look

@dannykopping
Copy link
Contributor

@joobisb sure thing, I'm on PTO since yesterday but back tomorrow.
Please address the CI failures as well.
We'll get this merged soon 👍 thanks for your patience!

@matifali matifali changed the title feat: add ability to include custom protoc-gen-go dependency in nix flake chore: add ability to include custom protoc-gen-go dependency in nix flake Sep 24, 2024
@matifali
Copy link
Member

It should be called a chore as it's not a user-facing feature. Also, please run make fmt to fix the formatting issue.

@dannykopping
make fmt
3335644 
Signed-off-by: Danny Kopping <danny@coder.com>
@dannykopping
Update hash
c86ca86 
Signed-off-by: Danny Kopping <danny@coder.com>
dannykopping
dannykopping approved these changes Sep 24, 2024
View reviewed changes
Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

Thanks for much for your contribution @joobisb!

@dannykopping dannykopping enabled auto-merge (squash) September 24, 2024 12:56
@dannykopping dannykopping merged commit c127d90 into coder:main Sep 24, 2024
27 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 24, 2024
@joobisb joobisb deleted the fix/flake-dep branch October 4, 2024 14:31
@coadler
Copy link
Contributor

coadler commented Oct 16, 2024

@joobisb I'm seeing the following failure on my NixOS machine

$ nix develop
warning: Git tree '/home/colin/Projects/coder/coder' is dirty
error: builder for '/nix/store/wg14v4ylmjrf2krlxbk6a049nvc23zad-protoc-gen-go.drv' failed with exit code 1;
       last 10 log lines:
       > calling 'postUnpack' function hook '_updateSourceDateEpochFromSourceRoot'
       > Running phase: patchPhase
       > Running phase: updateAutotoolsGnuConfigScriptsPhase
       > Running phase: configurePhase
       > calling 'preConfigure' function hook '_multioutConfig'
       > Running phase: buildPhase
       > evaling implicit 'preBuild' string hook
       > go: github.com/golang/protobuf@v1.5.0: Get "https://proxy.golang.org/github.com/golang/protobuf/@v/v1.5.0.info": dial tcp: lookup proxy.golang.org on [::1]:53: read udp [::1]:33535->[::1]:53: read: connection refused
       > go: github.com/google/go-cmp@v0.5.5: Get "https://proxy.golang.org/github.com/google/go-cmp/@v/v0.5.5.info": dial tcp: lookup proxy.golang.org on [::1]:53: read udp [::1]:54889->[::1]:53: read: connection refused
       > /nix/store/5r0df66ikad3xw06azlqvswcvncll8wa-stdenv-linux/setup: line 193: pop_var_context: head of shell_variables not a function context
       For full logs, run 'nix log /nix/store/wg14v4ylmjrf2krlxbk6a049nvc23zad-protoc-gen-go.drv'.
error: 1 dependencies of derivation '/nix/store/z9ik49f82idmhi99ra9zvd3q38cqsg9s-nix-shell-env.drv' failed to build

On Linux, nix builds restrict network access. I was able to fix by deleting the following lines:

           subPackages = [ "cmd/protoc-gen-go" ];
           vendorHash = null;
-          proxyVendor = true;
-          preBuild = ''
-            export GOPROXY=https://proxy.golang.org,direct
-            go mod download
-          '';
-

Is there a reason this preBuild hook is necessary?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dannykopping dannykopping dannykopping approved these changes

Assignees

@joobisb joobisb

Labels
community Pull Requests and issues created by the community.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ensure all dependencies defined in flake.nix and our GitHub Actions are aligned
4 participants
@joobisb @matifali @dannykopping @coadler