Add preliminary Go 1.21 support #978
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We're importing both the whole namespace as well as individual symbols. The list of individual symbols from gomod.py is getting fairly large so let's keep things simple by importing the whole gomod module and keep it properly namespaced despite the small inconvenience. Signed-off-by: Erik Skultety <[email protected]>
eskultety
requested review from
brunoapimentel,
taylormadore,
ben-alkov and
ejegrova
This patch introduces a skeleton interface to what will ultimately become the go CLI command wrapping class for all of the module logic. Signed-off-by: Erik Skultety <[email protected]>
This is essentially a copy-paste of the existing _run_gomod_cmd
function which it should ultimately replace when we switch the whole
module to the Go instance interface.
There are only 3 notable differences compared to the original:
- use of the more common **kwargs pattern to abstract any
subprocess.run arguments properly and make them transparent to
the caller
- forcing the type of 'cmd' from a Sequence to list (it's going to
be more handy with future patches)
- making the error message more generic tied towards the actual Go
command execution rather than tying it strictly to downloading
dependencies
Signed-off-by: Erik Skultety <[email protected]>
This is essentially a copy-paste of the existing _run_download_cmd
function which it should ultimately replace when we switch the whole
module to the Go instance interface. There are only 3 notable
differences compared to the original:
- use of the more common **kwargs pattern to abstract any
subprocess.run arguments properly and make them transparent to
the caller
- forcing the type of 'cmd' from a Sequence to list (it's going to
be more handy with future patches)
- making the error message more generic tied towards the actual Go
command execution rather than tying it strictly to downloading
dependencies
Signed-off-by: Erik Skultety <[email protected]>
eskultety
force-pushed
the
go-121-preliminary
branch
from
0a2bf8f
to
ae47d2c
Compare
|
All of the errors in the integration tests look related to the pathlib changes in python 3.12, which is included with fedora 39 🥲 |
Sigh. What if we install and fallback to an older version of Python in the container image for now and fix it properly in a standalone PR? |
eskultety
force-pushed
the
go-121-preliminary
branch
2 times, most recently
from
75bdd30
to
1e83470
Compare
|
This all LGTM 🚀 I've run a few tests locally and was able to build projects from the output with both go < 1.21 and 1.21. I think updating the README is the only thing left to do |
taylormadore
approved these changes
Feb 13, 2024
FWIW ^this turned out to cause more problems we'll have to figure out in the foreseeable future, but for the purposes of this PR decided to drop bumping to F39 just yet, so this comment can be considered resolved. |
|
Just finished my first read, approach and execution look amazing, Erik 🚀🚀🚀 I will dive deeper and hopefully I can give you my final ack today. |
|
@taylormadore @brunoapimentel do we need something specific about Go 1.21.0 to the README apart from adding it to the versions of tools? I mean the Go contents is already pretty generic and adding some 1.21 specific bits doesn't seem like it would fit. |
I think that's fine 👍 |
brunoapimentel
approved these changes
Feb 15, 2024
To make the whole Go CLI wrapper interface more convenient, make
instances of the class callables, so that one can just write
something like:
go(["foo", "bar"], env={}, capture_output=False)
So in the end it's supposed to make the usage more intuitive by:
- passing any CLI arguments to a callable named "go"
- passing subprocess arguments directly with the base signature
Signed-off-by: Erik Skultety <[email protected]>
The release property is essentially just a gateway to extracting the version number. It uses and parses 'go version' to get the information. Signed-off-by: Erik Skultety <[email protected]>
This property is going to be used for comparisons with the recommended/required toolchain versions coming from go.mod file, so it returns packaging.version.Version instead of plain string. Signed-off-by: Erik Skultety <[email protected]>
This helper is crucial in that it determines our behaviour based on the
Go version specified in the package's go.mod file. Unfortunately for us
we cannot utilize Go's native functionality of parsing the go.mod file
for us as older versions of Go will have issues with go.mod files based
on newer version of the language. Therefore, let's extract the
'go <ver>' line manually.
Signed-off-by: Erik Skultety <[email protected]>
This helper's job is to, given the repo source dir, figure out the right version of Go toolchain to use to process the request based on the 'go' directive in the go.mod file and return an instance of such Go toolchain. This handling is necessary since starting with Go 1.21 the meaning of the 'go' mod directive has changed slightly, now denoting the *minimum required* Go version which on its may not sound like much of a problem except for the undesired side effect of bumping the project's Go version coupling based on its dependencies, hence dirtying the input source dir; in other words, if a project's dependency mandates usage of Go >=1.21 and the project itself recommends 1.20, a platform bundled 1.21 toolchain would bump the project's Go version accordingly. To avoid dirtying of the input repo we select the processing toolchain based on the description above. Note that installation of the alternative versions of Go toolchain is out of scope of this patch and is handled separately. Signed-off-by: Erik Skultety <[email protected]>
This patch wires everything up by routing all 'go' calls through the wrapper class. Signed-off-by: Erik Skultety <[email protected]>
eskultety
force-pushed
the
go-121-preliminary
branch
from
181e3ff
to
060aa20
Compare
|
@brunoapimentel do you agree with the changes? |
We used to log the version of Go used for pre-fetching of the deps. We can still do that, but not outside of a Go instance since we may need alternate version of SDK to be installed, so that information would then be misleading. Since we already log this (and much more) as part of the Go class, we don't need the dropped hunk anymore. Signed-off-by: Erik Skultety <[email protected]>
The logic of these helpers has been extracted into the Go class and so these have become unused and can be dropped. Signed-off-by: Erik Skultety <[email protected]>
This patch is based on the [1] cachi2 counterpart. However, unlike with cachi2 which installs an older 1.20 toolchain as the alternative to the default 1.21 due to the reason explained in [2] we install a newer 1.21.0 alternative Go toolchain as a pairing to the default distro-bundled 1.20. The reason for that in cachito is that we're facing a plethora of other issues by simply bumping the Fedora base image to F39, so better tackle those separately rather than trying to fix all of the problems within the Go 1.21 patchset. [1] containerbuildsystem/cachi2@9e88c94 [2] Projects suggesting go toolchain <1.21 compiled with 1.21+ may actually get their version bumped by the new Go toolchain automatically which creates an undesirable side effect of dirtying the source tree. To prevent that, cachi2 pre-installs an older Go toolchain (1.20) as well and lets it handle such scenarios, using 1.21 only really for projects requiring 1.21+. Signed-off-by: Erik Skultety <[email protected]>
One of Go 1.21's main features is specifying the suggested version of toolchain to be used for compiling modules as a means of forwards compatibility. However, this patch only adds preliminary support for Go 1.21 in that it essentially explicitly denies usage of toolchains (which is fine, because those versions are only suggestive, Go always prefers building with the bundled toolchain anyway) by the means of explicitly setting the GOTOOLCHAIN env variable to 'local' which instructs Go to always use it bundled toolchain (provided it's new enough to conform to the minimum required version of Go indicated by the 'go' line). This patch makes sure we set GOTOOLCHAIN=local during fetching of the dependencies as well as for user container builds. STONEBLD-2047 Signed-off-by: Erik Skultety <[email protected]>
It's not a replacement of 1.20.7, because that still remains the default and 1.21.0 is downloaded only as an alternative SDK for projects that require it. Signed-off-by: Erik Skultety <[email protected]>
eskultety
force-pushed
the
go-121-preliminary
branch
from
060aa20
to
37285e1
Compare
Looks great! Thanks Erik. |
Merged
via the queue into
containerbuildsystem:master
with commit Feb 15, 2024
b115beb
14 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR just deals with the necessary bits to get projects building with 1.21. Proper 1.21 support including support for the toolchain will be proposed in the future.
This PR is based on containerbuildsystem/cachi2#427 from which much of the code was ported with one notable exception - as proposed cachito won't download the 1.20 toolchain locally like cachi2 does in case it's missing. The reason for that is that only the container environment use case is considered and hence the worker images will have 1.20 baked into them.
Maintainers will complete the following section