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

Bump golang version to 1.21.6-1 and switch to Microsoft Go #7446

Merged
merged 3 commits into from
Feb 21, 2024

Conversation

dagood
Copy link
Member

@dagood dagood commented Jan 24, 2024

Merge Checklist

All boxes should be checked before merging the PR (just tick any boxes which don't apply to this PR)

  • The toolchain has been rebuilt successfully (or no changes were made to it)
  • The toolchain/worker package manifests are up-to-date
  • Any updated packages successfully build (or no packages were changed)
  • Packages depending on static components modified in this PR (Golang, *-static subpackages, etc.) have had their Release tag incremented.
  • Package tests (%check section) have been verified with RUN_CHECK=y for existing SPEC files, or added to new SPEC files
  • All package sources are available
  • cgmanifest files are up-to-date and sorted (./cgmanifest.json, ./toolkit/scripts/toolchain/cgmanifest.json, .github/workflows/cgmanifest.json)
  • LICENSE-MAP files are up-to-date (./SPECS/LICENSES-AND-NOTICES/data/licenses.json, ./SPECS/LICENSES-AND-NOTICES/LICENSES-MAP.md, ./SPECS/LICENSES-AND-NOTICES/LICENSE-EXCEPTIONS.PHOTON)
  • All source files have up-to-date hashes in the *.signatures.json files
  • sudo make go-tidy-all and sudo make go-test-coverage pass
  • Documentation has been updated to match any changes to the build system
  • Ready to merge

Summary

Update golang package in 3.0 to use Microsoft Go 1.21.6-1, remove msft-golang package.

Change Log
  • Bump version to 1.21.6-1
  • Switch from upstream Go to the Microsoft build of Go
  • Clean up Go 1.17, 1.18 spec files
Does this affect the toolchain?

Unknown

Test Methodology
  • PR tests

@gdams
Copy link
Member

gdams commented Jan 24, 2024

@jslobodzian this is the progress we've made so far. Once we've got trigger access in AzDo we'll continue testing.

@mfrw mfrw added the 3.0-dev PRs Destined for AzureLinux 3.0 label Jan 25, 2024
@dagood
Copy link
Member Author

dagood commented Jan 26, 2024

We're having trouble with the buddy build, seems to be getting 404s very early on:
https://dev.azure.com/mariner-org/mariner/_build/results?buildId=491743
https://dev.azure.com/mariner-org/mariner/_build/results?buildId=491808

@dagood
Copy link
Member Author

dagood commented Jan 27, 2024

After rebasing on 4faeb04, a new buddy build hit a different error: https://dev.azure.com/mariner-org/mariner/_build/results?buildId=491974&view=results

I'm taking a look.

SPECS/golang/golang-1.17.spec Outdated Show resolved Hide resolved
@dagood dagood force-pushed the dev/dagood/update-to-msft branch 2 times, most recently from 52c4582 to 3bf4214 Compare January 31, 2024 22:07
@dagood
Copy link
Member Author

dagood commented Jan 31, 2024

Rebased again (because there was already a conflicting change merged since the latest build) and kicked off https://dev.azure.com/mariner-org/mariner/_build/results?buildId=495369.

@dagood dagood marked this pull request as ready for review January 31, 2024 22:10
@dagood dagood requested a review from a team as a code owner January 31, 2024 22:10
Copy link
Member

@gdams gdams left a comment

Choose a reason for hiding this comment

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

LGTM

Source1: https://dl.google.com/go/go1.4-bootstrap-20171003.tar.gz
Source2: https://dl.google.com/go/go%{bootstrap_compiler_version}.src.tar.gz
Source2: https://github.com/microsoft/go/releases/download/%{bootstrap_compiler_version}/go.20230802.5.src.tar.gz
Patch0: go14_bootstrap_aarch64.patch
Obsoletes: %{name} < %{version}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we might want to remove the obsoletes.. If we are ultimately supporting the n and n-1 versions concurrently. It might be that conflicts is more appropriate because we cannot have both installed at the same time....unless there is a way to do that.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's definitely possible to install multiple versions at the same time--the GOROOT itself can go in arbitrary locations, and symlinks seem to work. E.g. Ubuntu makes golang-1.20 that installs /usr/lib/go-1.20/bin/go--but it doesn't put anything in PATH, and I haven't seen any precedent for that, which I think people would expect here.

A setup that's been working for me: I have 20-some Go versions in ~/sdk and I keep a go symlink that points at a secure one. That's layered on top of https://github.com/golang/dl/blob/master/go1.21.6/main.go, where it puts a go1.21.6 binary in PATH after installing that official build through that tool in particular.

The packages here could maybe install PATH symlinks for go1.20.13 and go1.21.6 and another more independent package that follows the latest could install a go symlink. This could also work with the path toolset detection of https://go.dev/doc/toolchain.

Realistically, I don't know if anyone would take advantage of all that vs. simply installing one at a time. 😄 Especially because it's probably a CI scenario, not a heavyweight dev machine that's used to work on various repos.

/cc @qmuntal

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'm not super familiar with the intricacies of Obsoletes vs. Conflicts, but I'm not sure if either is needed here. From what I can see, the package name has always been golang, so installing a specific version will either upgrade or downgrade an existing installed version. It seems pretty natural that a package will conflict with another version of itself so I guess it doesn't need to be stated in the spec. 😄

Obsoletes seems to be more about handling one package(name) that no longer depends on another one, making it so that when installing the new version, the removed dependency will get cleaned up. Looking at all the Obsoleteses in https://src.fedoraproject.org/rpms/golang/blob/rawhide/f/golang.spec is interesting.

Conflicts seems like it's about conflicts between two different package(name)s. Although the intricacies look complicated and it seems like it should be treated as a last resort.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the tooling properly allows installing them side-by-side and the package is installing the contents properly, then yeah, remove the Obsoletes and the conflicts.

Copy link
Member Author

@dagood dagood Feb 5, 2024

Choose a reason for hiding this comment

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

The Go tools work side by side, but this spec file doesn't define the package or install the files in a way that allows side-by-side installs.

Even so, the Obsoletes: %{name} < %{version} is not needed (nor any Conflicts), because this is already being treated as an upgrade. For example, https://github.com/microsoft/CBL-Mariner/blob/2.0/SPECS/cmake/cmake.spec doesn't include Obsoletes, and it installs some non-side-by-side files like /usr/bin/cmake. When I run tdnf install -y cmake-3.21.4-3.cm2.x86_64 (old) then run tdnf install cmake, it asks if I want to upgrade:

root [ / ]# tdnf install cmake
Loaded plugin: tdnfrepogpgcheck

Upgrading:
cmake              x86_64        3.21.4-10.cm2      mariner-official-base  46.54M      17.51M  
Total installed size:  46.54M
Total download size:  17.51M
Is this ok [y/N]:

As far as I can tell, it's theoretically possible to force dnf to install two versions on the same machine, but it's involved enough that I don't think anyone would do it (to any package, let alone golang) without either explicitly installing it to a different location or expecting some bad results.

Creating a set of RPM package(name)s that work side by side seems doable to me, but not in the scope of this PR.

(I pushed a change to remove Obsoletes: %{name} < %{version}.)

@@ -68,7 +71,7 @@ popd
rm -rf %{_libdir}/golang

# Make go%{bootstrap_compiler_version} as the new bootstrapper
mv -v %{_topdir}/BUILD/go1.19.12 %{_libdir}/golang
mv -v %{_topdir}/BUILD/go%{bootstrap_compiler_version} %{_libdir}/golang

# Build current go version
export GOHOSTOS=linux
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 verify that these exports are correct? Esepcially the "GOROOT" one. I have a bug against Mariner 2.0 (#6363) that suggests GOROOT is pointing to the wrong place.

 It seems to be setting GOROOT to /usr/bin/go.
 But /usr/bin/go is a binary.
 Actual GOROOT should be /usr/lib/golang in this case.

Copy link
Member

Choose a reason for hiding this comment

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

The GOROOT_FINAL support has been dropped in go1.21.x
golang/go#62047

Copy link
Member Author

@dagood dagood Feb 1, 2024

Choose a reason for hiding this comment

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

Yeah, that's the wrong place. It should be possible to simply remove it--since 1.9, GOROOT is automatic. Although there might be something odd necessary due to bootstrapping, I don't have practical experience with it.

Also, the only part of /etc/profile.d/go-exports.sh that I think makes a difference nowadays is GOPATH, but I don't know whether it's actually good or not to move it from a user-specific path (default) to a systemwide path. For a container I suppose it doesn't matter.

I tried go env before and after . /etc/profile.d/go-exports.sh (with the 2.0 golang 1.20 package), and the difference was:

-GOMODCACHE="/root/go/pkg/mod"
+GOMODCACHE="/usr/share/gocode/pkg/mod"
-GOPATH="/root/go"
+GOPATH="/usr/share/gocode"
GOROOT="/usr/lib/golang"

(I don't know in what circumstances a bad GOROOT_FINAL results in an error, this isn't something I've seen before.)

I wasn't able to get https://github.com/microsoft/CBL-MarinerTutorials/tree/main/build-in-container to work, so I don't have a good dev loop to try things out. (We decided to go ahead with this PR because if URL substitution fails, it probably won't be a subtle problem. 😄) I might have been hitting the same issues as our buddy builds have been hitting. This is something I'll have to look into later.

URL: https://golang.org
Source0: https://golang.org/dl/go%{version}.src.tar.gz
URL: https://github.com/microsoft/go
Source0: https://github.com/microsoft/go/releases/download/v%{version}-%{ms_go_revision}/go.%{ms_go_buildid}.src.tar.gz
Source1: https://dl.google.com/go/go1.4-bootstrap-20171003.tar.gz
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe worth mentioning that we are also hosting this at https://github.com/microsoft/go/releases/tag/v1.4.0-1, but I don't see any pressing reason to change the URL in this spec. The checksum is the same, and a download cache is used to actually build the package.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's the same, I think it's still worth updating the URL so that there's no confusion about why we're using partially Microsoft and partially Google's sources. And if the name of the file is different in Microsoft's URL, adding #<cached_file_name> at the end of the URL will make the tool still pick the right file from our cache.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, pushed. Thanks for the note about #, that might be handy if we end up hitting some wall in our automation.

In theory, it might be useful to keep upstream's URL to show people that this package does use the same bootstrap sources as ordinary Go distributions. So, added a comment, too.

Copy link
Member

@mfrw mfrw left a comment

Choose a reason for hiding this comment

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

I have triggered a test build:

We might need one more rebase because of merge-conflict though :)
@dagood

Changes LGTM :)

@eiffel-fl eiffel-fl mentioned this pull request Feb 8, 2024
12 tasks
Remove "Obsoletes" clause pointing to self.
@dagood
Copy link
Member Author

dagood commented Feb 8, 2024

Thanks! Rebased, and for good measure, kicked off https://dev.azure.com/mariner-org/mariner/_build/results?buildId=502010.

@@ -141,6 +143,10 @@ fi
%{_bindir}/*

%changelog
* Wed Jan 24 2024 Davis Goodin <dagood@microsoft.com> - 1.21.6-1
Copy link
Member

Choose a reason for hiding this comment

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

A final review comment from me:
Since go builds static binaries and the way our artifact publish works is; we only publish new packages if there was a change from previous.

Although, 3.0 is not released yet, but we should bump the release of all packages that depend on go so that; come publish time, they are one release ahead of what is already present there and we publish the ones that are actually compiled with our new compiler rather than being compiled by the older version of go before this PR :).

A representative PR (although from 2.0) but the idea is the same:

There is a helper script (best-effort): scripts/update_spec.sh to mass release-bump specs and add a changelog entry.

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're aware of the need for this, but it looks like scripts/update_spec.sh still needs a list of packages to bump, and we're expecting one step beyond that: a script that bumps all the dependent packages automatically.

Copy link
Member

@mfrw mfrw left a comment

Choose a reason for hiding this comment

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

From my local testing of the PR, I think podman would need an update for gvproxy.
as per pr:

@dagood

SPECS/golang/golang.spec Outdated Show resolved Hide resolved
dagood and others added 2 commits February 15, 2024 16:53
Co-authored-by: Pawel Winogrodzki <pawelwi@microsoft.com>
@dagood
Copy link
Member Author

dagood commented Feb 16, 2024

Kicked off https://dev.azure.com/mariner-org/mariner/_build/results?buildId=507600 for the latest changes.

From my local testing of the PR, I think podman would need an update for gvproxy.

@mfrw It would be interesting to see the errors/logs from that. Normally Go should be backward compatible, although that spec does seem to be building in a more complicated way than I'm used to. (Maybe as a new issue or over email to avoid clutter in this PR, which I don't think is blocked by this?)

@dagood dagood merged commit 1fa5394 into microsoft:3.0-dev Feb 21, 2024
8 checks passed
@dagood dagood deleted the dev/dagood/update-to-msft branch February 21, 2024 19:18
@dagood
Copy link
Member Author

dagood commented Feb 21, 2024

In the meantime, 1.21.7-1 has been released. I'm planning to submit an update PR to that version, which will also make sure the list of spec+other changes for a patch is what we expect and we can set up automation around it.

1.22.0-1 is also out now, but it requires inserting another bootstrapping step, so that's something we'll work on separately from the 1.21.7-1 update.

eiffel-fl pushed a commit to eiffel-fl/CBL-Mariner that referenced this pull request Feb 23, 2024
…#7446)

Co-authored-by: Pawel Winogrodzki <pawelwi@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.0-dev PRs Destined for AzureLinux 3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants