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.

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

go_1_17: init at 1.17.1 #127519

Merged
merged 2 commits into from Sep 11, 2021
Merged

go_1_17: init at 1.17.1 #127519

merged 2 commits into from Sep 11, 2021

Conversation

zowoq
Copy link
Contributor

@zowoq zowoq commented Jun 20, 2021

Motivation for this change
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/)
  • 21.11 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.

@kalbasit
Copy link
Member

kalbasit commented Jun 21, 2021 •
edited

Except from https://tip.golang.org/doc/go1.17

vendor contents

If the main module specifies go 1.17 or higher, go mod vendor now annotates vendor/modules.txt with the go version indicated by each vendored module in its own go.mod file. The annotated version is used when building the module's packages from vendored source code.
If the main module specifies go 1.17 or higher, go mod vendor now omits go.mod and go.sum files for vendored dependencies, which can otherwise interfere with the ability of the go command to identify the correct module root when invoked within the vendor tree.

This may change the hashes of vendorSha256 in existing packages that are using buildGoModule.

@zowoq
Copy link
Contributor Author

zowoq commented Jun 21, 2021

Yeah, this release isn't going to be easy ...

Go 1.17 requires macOS 10.13 High Sierra or later

aarch64-darwin is 11.0.0 but x86_64-darwin is 10.12.

If the main module specifies go 1.17 or higher

Updating to 1.17 and pinning darwin or even just x86_64-darwin to 1.16 seems like it's going to end up causing problems.

@endocrimes
Copy link
Member

this may change the hashes of vendorSha256 in existing packages that are using buildGoModule.

this feels like a pretty good reason to not force bump everything immediately - but instead give maintainers a chance to upgrade their modules as upstreams jump to 1.17

@zowoq zowoq changed the title go_1_17: init at 1.17beta1 go_1_17: init at 1.17rc1 Jul 16, 2021
@zowoq
Copy link
Contributor Author

zowoq commented Aug 5, 2021 •
edited

I'm keeping this in sync with the other go_*.nix files so I'm not addressing nits in this PR.

@zowoq zowoq changed the title go_1_17: init at 1.17rc1 go_1_17: init at 1.17rc2 Aug 5, 2021
@zowoq zowoq changed the title go_1_17: init at 1.17rc2 go_1_17: init at 1.17 Aug 16, 2021
@endocrimes
Copy link
Member

I’d still like to see #127519 (comment) at least partially resolved before this lands or soon after - it’s becoming difficult to track their history.

this explicitly is not a nitpick, but something important to longer term maintainability.

saschagrunert
saschagrunert approved these changes Aug 24, 2021
View reviewed changes
Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

LGTM

@c00w
Copy link
Contributor

c00w commented Sep 1, 2021

Any chance we can get this landed? It's been 2 weeks since go1.17 was released publicly and it would be great to use this without having to maintain a fork :).

@c00w
Copy link
Contributor

c00w commented Sep 1, 2021

re: updating the hashes - I still have my code (and it's somewhere on either github or git.sr.ht), which will autoregen all the hashes for go packages, so I can probably get most packages that are compatible bulk migrated once this lands.

@zowoq
Copy link
Contributor Author

zowoq commented Sep 1, 2021

We can merge this go make go_1_17 available but with of the go module and platform changes in this version I don't want to allow use of buildGo117{Module,Package} in nixpkgs for now.

@NixOS/golang Are we okay with that and the comment I've added in all-packages.nix?

@danielphan2003 danielphan2003 mentioned this pull request Sep 3, 2021
Open
@zowoq zowoq changed the title go_1_17: init at 1.17 go_1_17: init at 1.17.1 Sep 9, 2021
@ofborg ofborg bot requested a review from c00w September 9, 2021 20:55
@zowoq zowoq marked this pull request as ready for review September 9, 2021 22:53
@zowoq zowoq mentioned this pull request Sep 11, 2021
Closed
12 tasks
@zowoq zowoq merged commit 9675a86 into NixOS:master Sep 11, 2021
@zowoq zowoq deleted the go117 branch September 11, 2021 00:44
@terinjokes
Copy link
Contributor

@zowoq Is this a ticket to track work needed to begin considering allow buildGo117{Module,Package}?

@twpayne twpayne mentioned this pull request Oct 3, 2021
Merged
@risicle risicle mentioned this pull request Oct 17, 2021
Merged
12 tasks
Ma27
Ma27 reviewed Oct 25, 2021
View reviewed changes
pkgs/top-level/all-packages.nix
@@ -19158,6 +19174,15 @@ in
buildGo116Module = callPackage ../development/go-modules/generic {
go = buildPackages.go_1_16;
};
# go_1_17 has go module changes which may not be portable
# across different go versions and/or platforms,
Copy link
Member

Choose a reason for hiding this comment

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

Can you elaborate what implications this has on things we're trying to build with buildGo117Module?

pkgs/top-level/all-packages.nix
# go_1_17 has go module changes which may not be portable
# across different go versions and/or platforms,
# it also requires >=10.13 stdenv on darwin which
# is not currently available for x86_64-darwin
Copy link
Member

Choose a reason for hiding this comment

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

Can't we just mark each derivation built with buildGo117Package on x86_64-darwin as broken?

@r-burns r-burns mentioned this pull request Nov 4, 2021
Merged
12 tasks
@kalbasit kalbasit mentioned this pull request Nov 4, 2021
Closed
@Artturin
Copy link
Member

Artturin commented Nov 5, 2021 •
edited

re: updating the hashes - I still have my code (and it's somewhere on either github or git.sr.ht), which will autoregen all the hashes for go packages, so I can probably get most packages that are compatible bulk migrated once this lands.

hey, could you send the script if you still have it?

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/missing-go-1-17-on-x86-64-darwin/15032/3

@bryanasdev000 bryanasdev000 mentioned this pull request Mar 23, 2022
Merged
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Xe Xe Xe left review comments

@endocrimes endocrimes endocrimes left review comments

@Ma27 Ma27 Ma27 left review comments

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@saschagrunert saschagrunert saschagrunert approved these changes

@kalbasit kalbasit Awaiting requested review from kalbasit kalbasit is a code owner

@rvolosatovs rvolosatovs Awaiting requested review from rvolosatovs

@Frostman Frostman Awaiting requested review from Frostman

@mdlayher mdlayher Awaiting requested review from mdlayher

@cstrahan cstrahan Awaiting requested review from cstrahan

@Mic92 Mic92 Awaiting requested review from Mic92 Mic92 is a code owner

@c00w c00w Awaiting requested review from c00w

Assignees
No one assigned
Labels
6.topic: golang 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet