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

go_1_21: init at 1.21rc3 #238100

Merged
merged 4 commits into from
Aug 1, 2023
Merged

go_1_21: init at 1.21rc3 #238100

merged 4 commits into from
Aug 1, 2023

Conversation

zowoq
Copy link
Contributor

@zowoq zowoq commented Jun 16, 2023

Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@zowoq zowoq requested review from kalbasit and Mic92 as code owners June 16, 2023 15:00
@zowoq zowoq marked this pull request as draft June 16, 2023 15:00
@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Jun 16, 2023
@ofborg ofborg bot requested a review from qbit June 16, 2023 15:39
@zowoq zowoq changed the title go_1_21: init at 1.21rc1 go_1_21: init at 1.21rc2 Jun 21, 2023
@zowoq zowoq marked this pull request as ready for review June 21, 2023 22:56
Copy link
Contributor

@qbit qbit left a comment

Choose a reason for hiding this comment

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

LGTM, and it builds fine too!

@zowoq zowoq marked this pull request as draft June 22, 2023 02:18
@zowoq
Copy link
Contributor Author

zowoq commented Jun 22, 2023

Seems 1.21 breaks the buildGoModule FOD fetcher:

GOPROXY list is not the empty string, but contains no entries

Looks like we just need to fix buildGoModule in a backwards compatible way, the FOD hashes are still the same.

@zowoq zowoq added the 2.status: work-in-progress This PR isn't done label Jun 22, 2023
@ThinkChaos
Copy link
Contributor

Go 1.21 will download and run any Go toolchain built code references by default. I think this is something NixOS should disable, or at minimum communicate about. I'm not even convinced this behavior will work out of the box on NixOS since the go downloaded toolchains will presumably be pre-build and dynamically linked (this is conjecture).
See Go Toolchains. The Go toolchain selection section should be enough for a quick overview.

Has this been discussed? Is there a NixOS policy that applies here?
Has this feature been tested?

@zowoq
Copy link
Contributor Author

zowoq commented Jul 1, 2023

Downloading a toolchain isn't something we'd want to allow, I'll disable it. Very likely that it wouldn't work anyway.

@zowoq zowoq removed the 2.status: work-in-progress This PR isn't done label Jul 12, 2023
@zowoq
Copy link
Contributor Author

zowoq commented Jul 12, 2023

@ofborg eval

@zowoq zowoq changed the title go_1_21: init at 1.21rc2 go_1_21: init at 1.21rc3 Jul 15, 2023
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 15, 2023
@zowoq zowoq marked this pull request as draft July 15, 2023 00:48
@zowoq zowoq marked this pull request as ready for review July 15, 2023 00:53
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 15, 2023
@@ -85,6 +87,7 @@ let
runHook preConfigure
export GOCACHE=$TMPDIR/go-cache
export GOPATH="$TMPDIR/go"
export GOPROXY="''${GOPROXY:-direct}" # respect impureEnvVars
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I think keeping it default (and making it overridable is better. Having sumdb backed checking seems like a win to me.

Copy link
Contributor Author

@zowoq zowoq Jul 22, 2023

Choose a reason for hiding this comment

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

Personally I think keeping it default

Do you mean the leaving it unset or setting https://proxy.golang.org,direct?

I'd prefer not to set it at all but but need to do something to address #238100 (comment).

and making it overridable is better

Besides setting it via impureEnvVars?

Copy link
Contributor

Choose a reason for hiding this comment

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

From my understanding of:

When GOTOOLCHAIN is set to local, the go command always runs the bundled Go toolchain.

We only need to set GOTOOLCHAIN to address the toolchain downloading concern.. Setting GOPROXY will impact module fetching as well as (if we didn't already have it as local toolchain fetching)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What does GOTOOLCHAIN have to do with the GOPROXY error?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, my bad, I clicked it and read the comment below ;D

In that case, I think setting it to https://proxy.golang.org,direct is the best approach (assuming it fixes the error).

Copy link
Contributor

Choose a reason for hiding this comment

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

The '' in the var value seems like a typo to me?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case, I think setting it to https://proxy.golang.org,direct is the best approach (assuming it fixes the error).

I expect people probably won't like that proxy.golang.org is set here even although it is the default that we've been using anyway.

The '' in the var value seems like a typo to me?

It isn't a typo, need to escape ${}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The GOPROXY issue was fixed in cbc976a so I'll revert this in #250492.

@colemickens
Copy link
Member

I have a version of this I'm maintaining as well. A few diffs, I didn't have to set as many env vars as you did.

I also rewrote print-hashes to not have duplicates and also added a 121 bootstrap.

This means I have Go cross-compiling working for my riscv64 targets, which is nice, since they have published bootstrap binary builds for riscv64.

Please feel free to steal commits: https://github.com/colemickens/nixpkgs/commits/go121

@zowoq
Copy link
Contributor Author

zowoq commented Jul 28, 2023

I also rewrote print-hashes to not have duplicates

I don't know what this means? With print-hashes.sh I get the same as you have for 1.21rc3?

and also added a 121 bootstrap.

This means I have Go cross-compiling working for my riscv64 targets, which is nice, since they have published bootstrap binary builds for riscv64.

Will deal with stuff like this in a follow up, primary concern with this PR is aarch64/x86_64 on darwin/linux.

@zowoq zowoq merged commit 3392d56 into NixOS:staging Aug 1, 2023
@zowoq zowoq deleted the go121 branch August 1, 2023 15:08
@zowoq
Copy link
Contributor Author

zowoq commented Aug 3, 2023

Will deal with stuff like this in a follow up

I'll wait for go_1_21 to land in master and continue in #246935.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants