-
Notifications
You must be signed in to change notification settings - Fork 27
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
Add preliminary Go 1.21 support #427
Conversation
We need to update the go version in the docs: https://github.com/containerbuildsystem/cachi2/blob/main/README.md?plain=1#L311. Besides this, LGTM 🚀 |
Converting to draft as due to #419 (comment) this should be proposed as POC first. |
39758a3
to
7ea7d38
Compare
Dropping the draft flag, since we can assume the latest revision to act as v1. Note that unit/integration tests implementation was skipped for v1 (apart from the necessary changes to keep the existing suite passing) to get review on the overall design. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach LGTM 👍
Didn't go into the details
tests/integration/test_data/gomod_e2e_multiple_modules/bom.json
Outdated
Show resolved
Hide resolved
So, I'm having a small issue running this locally when I target a Go 1.18 repo:
I could work around this by passing my local $HOME to the |
Doh! I knew about this, but forgot to address it amongst other things that had to be done here. So Go's SDK download and install requires |
One more small detail: apparently, very old versions of I think we can easily leave this as unsupported... but maybe it's just as easy to assume this should be handled by go 1.20. |
LOL, that must be veeery old. Well, my original unposted version of the code handling this situation looked roughly like this:
But then I figured versions of Go that wouldn't pass parsing with the regex have already been long EOL'd.
Yeah, good point, I can set the parsed version using the regex to a fixed value of the bundled Go if the regex doesn't find a match (so that we get past the version comparisons) and let the bundled Go installation (since it claims to be backwards compatible) handle it, that's a trivial fix. |
a8d2214
to
9ce522a
Compare
since v1:
|
Maybe it'd be good to add some unit tests to the new I think we also should bump some of our integration test-repos to Go 1.21 (or add new branches/scenarios). But we can do this in a follow up story. |
Sure, I will add them provided I have a green light on this approach, see #427 (comment) |
since v2:
|
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]>
This helper's purpose is to locate an alternative toolchain installation if requested. It scans 2 locations: 1) system /usr/local/go/go<ver> - this one will be pre-installed in the container image to save time during runtime 2) $XDG_CACHE_HOME/cachi2/go - this one is targeted at local cachi2 use cases, i.e. without using the container image 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 patch wires everything up by routing all 'go' calls through the wrapper class. Signed-off-by: Erik Skultety <[email protected]>
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]>
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 the 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 both pre-fetch as well as for user container builds. Future patches will add proper support for 1.21 dealing with alternative toolchain versions that will need to be prefetched. STONEBLD-2046 Signed-off-by: Erik Skultety <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Since v3:
|
The main motivation behind this change is the Go 1.21 version which isn't available on Fedora 38 but is on Fedora 39. Resolves: containerbuildsystem#387 Signed-off-by: Erik Skultety <[email protected]>
Since we bumped the container image to Fedora 39 shipping with 1.21, we need to update our Go mock data with 1.21. Signed-off-by: Erik Skultety <[email protected]>
since v4:
|
@taylormadore @ben-alkov please give this one more quick look and if it looks good I'll proceed with the merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still LGTM 👍
3a916a0
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.Maintainers will complete the following section
Docs links in the code are still valid (if docs were updated)Note: if the contribution is external (not from an organization member), the CI
pipeline will not run automatically. After verifying that the CI is safe to run:
/ok-to-test
(as is the standard for Pipelines as Code)