Full Go 1.21 support #996
Full Go 1.21 support #996
Conversation
|
/retest |
|
Overall, LGTM! We just need to fix the failing test. |
|
@taylormadore @brunoapimentel I bumped into another problem. So, after updating the go 1.21 deps bundle (in my own fork), the integration test again fails on content mismatch. And here's the same bit in the expected data that I generated yesterday: To make it more clearly visible, here's the diff: In other words, the hash for the toolchain module matches, but the tlog record [1] [2] from the database server is different, but still properly signed and apparently still identifying the correct object, my humble assumption is that there's a load balancer on the server side and/or the database is somehow replicated, so while the signature is correct, the subtree for providing the signature for the module is different. No idea how to proceed here as I expect this to continue breaking the test suite even after merging. FWIW you can double check my findings by simply running [1] https://pkg.go.dev/golang.org/x/mod/sumdb/tlog#FormatTree |
As far as I could tell, the tlog record is ever changing, so I don't see any other way to work around this besides somehow ignoring it during the tests. We can have some type of ignore list mechanism to handle such scenarios in |
I went ahead and created a ignore list for that test: master...brunoapimentel:cachito:go-121. See if you think this is a good solution, the test is passing when pointing to the file on your fork of cachito-testing/test_files. |
We report the go release once we query the bundled version of Go, but we don't actually report what version we end up using which is potentially misleading users into believing that what the bundled Go reports is what we'll use for the prefetch. That won't necessarily be true in the future where we'll base our efforts around the .0 toolchain releases of each minor version of Go along with the GOTOOLCHAIN=auto setting. Signed-off-by: Erik Skultety <eskultet@redhat.com>
- mock Go.__call__ instead of Go._run and version fetching - since we're mocking __call__ we can now make use of the existing 'go_mod_file' fixture and indirectly parametrize it Signed-off-by: Erik Skultety <eskultet@redhat.com>
There's no need to take path to the whole source repo directory, this is tiny self-contained helper that only needs path to a given go.mod file. Taken-over-from: containerbuildsystem/cachi2@b3b77ee Signed-off-by: Erik Skultety <eskultet@redhat.com>
Currently the fixture creates a go.mod file whilst being indirectly parametrized, but still returns None which prevents us from utilizing the power of indirect parametrization fully and so we still have to construct the same temporary path in our test case (yet still having to specify 'go_mod_file' as a pytest fixture dependency in the main test function signature). Return the written path and simplify the test code some more. Signed-off-by: Erik Skultety <eskultet@redhat.com>
Unfortunately, with Go 1.21 it's not enough to rely solely on what the mod file declares with the 'go' line, but also what it declares with the 'toolchain' line. While 'toolchain' is only suggestive, i.e. not usually enforced, the situation changes when combined with GOTOOLCHAIN=auto. Start parsing that keyword for future use. Inspired-by: containerbuildsystem/cachi2@2c83a58 Signed-off-by: Erik Skultety <eskultet@redhat.com>
Future patches will enable automatic toolchain handling with
GOTOOLCHAIN=auto meaning that Go will download a missing toolchain on
its own, however that also means that it could easily download a
version of Go which wasn't vetted by cachito with regards to feature
safety to avoid arbitrary code execution, etc.
Therefore, this patch sets an upper boundary on the supported Go
version. In this case, anything above 1.22 will automatically fail the
request.
Note that because we're going to set GOTOOLCHAIN=auto for our
dependency fetching in a future patch we must also check the version
coming from the 'toolchain' keyword. The reason for that is that
despite 'toolchain' being only suggestive, it becomes enforced with
GOTOOLCHAIN=auto and if someone writes the following go.mod file we'd
fetch and use a toolchain version we have not vetted for our project's
use:
go 1.21
toolchain 1.22.0
Insipired-by: containerbuildsystem/cachi2@52ad833
Signed-off-by: Erik Skultety <eskultet@redhat.com>
If the destination exists an error will be emitted and the container image build will fail. Use '-p' which besides creating parent directories makes sure that no error is emitted for existing directories in the destination path. Inspired-by: containerbuildsystem/cachi2@4b4435c Signed-off-by: Erik Skultety <eskultet@redhat.com>
Currently, we only pre install Go 1.20 as a fallback for projects building with older version of Go due to some specific behavioral semantic changes to 1.21 which our container image (Fedora 39) is based on. Starting Go 1.21, the go toolchain can fetch and use newer toolchains than the bundled one which is used to process a project from the command line. However, if the project's Go 1.21.X toolchain requirement is older than the bundled toolchain doing the processing, the older toolchain is never downloaded in such case. This design decision makes things for hermetic builds harder because not having any control over the container build environment it could potentially fail due a missing Go toolchain version if we had not prefetched it due to the Go's version handling. Inspired-by: containerbuildsystem/cachi2@9967a2e Signed-off-by: Erik Skultety <eskultet@redhat.com>
I applied both with one nitpick change and a slight adjustment to one of the commit messages, otherwise untouched. I confirm that with your commits the failing test finally passes. In the meantime I also pushed an update to the test data tarball, so hopefully the CI should finally be green. Thank you for suggesting the commits! |
I'm completely fine with the changes! The failure in the integration test now is something I had not noticed. It is probably because we're overwriting the ignore defaults: So we'll probably have to join those with what we pass in |
Yep, I already have that particular change locally, but something else is happening as well after I had to change the order of arguments to |
<facepalm> a typo in the Outside of scope of this PR, but the way integration tests are handled is way too fragile. |
Commit 9f08f8e added preliminary support for Go 1.21 only, meaning that we'd use a fixed toolchain version of 1.21.0 (or the bundled Go toolchain, whichever was bigger). Some projects do bump even the micro version of their required Go toolchain which means that we'd be forced to refresh the container image with every Go micro release to keep up with these bumps for the dependency prefetching (looking at you Openshift). Starting with Go 1.21, however, one is able to instruct Go to download a newer toolchain automatically if the installed one doesn't comply with the requirement in go.mod. That behaviour is controller by the GOTOOLCHAIN variable which will need to be set as 'GOTOOLCHAIN=auto' to override the default behaviour of Go (depending on the installation) which always prefers the installed toolchain. One thing to consider is the fact that a newer toolchain never downloads an older one, meaning that if the project's build image doesn't supply the desired version of Go (either with through the 'go' line or the 'toolchain' line) and the consumer overrides the GOTOOLCHAIN setting to 'auto', the build will fail in either case because the correct toolchain version won't be available. To mitigate this on our side we'll always base our processing on the 0th release of a minor Go version (i.e. 1.21.0, 1.22.0, etc.) which ensures that with GOTOOLCHAIN=auto Go will always download and cache any newer toolchain from any dependent module. Note that we're only going to set GOTOOLCHAIN=auto for the dependency fetching, i.e. in our own environment. Consumers are free to override the GOTOOLCHAIN at their will in their build environment. With that in mind, we're also dropping the GOTOOLCHAIN=local override we have been forcing upon the users so far for the very same reason. Inspired-by: containerbuildsystem/cachi2@e0732c7 Signed-off-by: Erik Skultety <eskultet@redhat.com>
This directory only contains records regarding the toolchain download. More importantly though, these tlog records change in time so while still providing accurate verification information on a toolchain download we must ensure determinism in the test execution, hence we have to ignore this directory. Signed-off-by: Bruno Pimentel <bpimente@redhat.com>
This allows the test to ignore certain files or directories when checking the contents of the request output. The parameter takes a map that has as key the directory that needs to be checked (as declared in the expected_files parameter) and a list of file/directory names to be ignored. Note that these names do not represent absolute paths, so any file or directory matching each name will be ignored. Signed-off-by: Bruno Pimentel <bpimente@redhat.com>
|
We had a face-to-face discussion about cachito setting GOSUMDB=off and that conflicting with when GOTOOLCHAIN=auto on the user build side: I'm going to approve and we'll document/resolve this in a follow-up. |
052a78a
Maintainers will complete the following section
OpenAPI schema is updated (if applicable)DB schema change has corresponding DB migration (if applicable)README updated (if worker configuration changed, or if applicable)