Skip to content

Conversation

@cardil
Copy link
Contributor

@cardil cardil commented Oct 3, 2025

Changes

  • 🐛 Set GOTOOLCHAIN=auto for knative.dev/toolbox tools

This allows Go to automatically fetch a remote go run dep with a newer version of golang, if needed. This should be okay, as those tools are development only, and not used in production code.

/kind bug

Fixes #426

Release Note

Automatically sets the GOTOOLCHAIN=auto for knative.dev/toolbox tools

This commit introduces a new shell helper function, `resolve_dep_version`,
which retrieves the version of a specified Go module from the project's
`go.mod` file.

As part of this change:
- All usages of `knative.dev/toolbox/...@latest` have been pinned to a
  specific commit (`@bc7e152`) to ensure reproducible builds and prevent
  unexpected failures due to upstream changes.
- The shell script test harness has been improved to more reliably locate
  the `go` executable, making the tests more robust across different
  environments.
@knative-prow knative-prow bot added kind/bug Categorizes issue or PR as related to a bug. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Oct 3, 2025
@knative-prow
Copy link

knative-prow bot commented Oct 3, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cardil

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow knative-prow bot requested review from mgencur and upodroid October 3, 2025 13:37
@knative-prow knative-prow bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 3, 2025
@cardil cardil changed the title [WIP] Fix knative.dev/toolbox for older versions of Golang 🐛 Fix knative.dev/toolbox for older versions of Golang Oct 6, 2025
@knative-prow knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 6, 2025
The `go_run` helper function is updated to set `GOTOOLCHAIN=auto`
when executing toolbox tools.
@cardil cardil force-pushed the bugfix/resolve-toolbox-version branch from 33d9abf to e9d3c22 Compare October 6, 2025 16:35
@cardil
Copy link
Contributor Author

cardil commented Oct 6, 2025

/assign @maschmid

@cardil
Copy link
Contributor Author

cardil commented Oct 6, 2025

/cc @maschmid

@knative-prow knative-prow bot requested a review from maschmid October 6, 2025 16:36
@knative-prow knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 6, 2025
This change corrects the logic to set `GOTOOLCHAIN=auto` for any package within the `knative.dev/toolbox` module.
@cardil cardil requested a review from Copilot October 6, 2025 19:46
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes compatibility issues with older versions of Golang by automatically setting GOTOOLCHAIN=auto for knative.dev/toolbox tools. This allows Go to fetch remote dependencies with newer Go versions when needed.

  • Adds automatic GOTOOLCHAIN=auto environment variable for knative.dev/toolbox tools
  • Updates test infrastructure to handle Go toolchain switching messages and improve binary detection
  • Refactors test loops to use modern Go range syntax

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
library.sh Adds GOTOOLCHAIN=auto setting for knative.dev/toolbox packages in go_run function
presubmit-tests.sh Updates to use go_run wrapper instead of direct go run
test/unit/sharedlib_test.go Enhances test infrastructure with better executable finding and Go toolchain message filtering
test/unit/update_deps_test.go Modernizes for loop syntax from index-based to range-based
test/unit/report_go_test.go Modernizes for loop syntax from index-based to range-based
test/unit/release_test.go Modernizes for loop syntax from index-based to range-based
test/unit/presubmit_test.go Modernizes for loop syntax from index-based to range-based
test/unit/codegen_test.go Modernizes for loop syntax from index-based to range-based

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@cardil cardil requested a review from Copilot October 6, 2025 20:38
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

This commit makes two corrections to the test suite in `sharedlib_test.go`:

1.  In the main test runner, the `errOut` variable (which captures stderr) was incorrectly being checked against the `outputTypeStdout` constant. This has been corrected to `outputTypeStderr` to ensure test failure messages accurately reflect the stream being checked.

2.  Potential errors from `io.ReadAll` when capturing stdout and stderr in the `goRunHelpPrefetcher` were being ignored. Assertions have been added to ensure that any errors during these reads are caught, making the test more robust.
@cardil cardil force-pushed the bugfix/resolve-toolbox-version branch from 4b16d14 to ce37357 Compare October 6, 2025 20:41
@cardil cardil requested a review from Copilot October 7, 2025 09:10
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

if m.IsDir() {
return syscall.EISDIR
}
if m&0111 != 0 {
Copy link

Copilot AI Oct 7, 2025

Choose a reason for hiding this comment

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

The magic number 0111 (octal for executable permission bits) should be defined as a named constant like executablePermMask = 0111 to improve code readability and maintainability.

Copilot uses AI. Check for mistakes.
@maschmid
Copy link

/lgtm

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Oct 13, 2025
@knative-prow knative-prow bot merged commit 49bc1be into knative:main Oct 13, 2025
6 checks passed
@cardil cardil deleted the bugfix/resolve-toolbox-version branch October 13, 2025 11:19
@cardil
Copy link
Contributor Author

cardil commented Oct 15, 2025

/cherrypick release-1.15
/cherrypick release-1.16
/cherrypick release-1.17
/cherrypick release-1.18
/cherrypick release-1.19

@knative-prow-robot
Copy link

@cardil: #436 failed to apply on top of branch "release-1.15":

Applying: Add resolve_dep_version function
Using index info to reconstruct a base tree...
M	codegen-library.sh
M	library.sh
M	presubmit-tests.sh
M	test/unit/library_test.go
M	test/unit/sharedlib_test.go
M	test/unit/update_deps_test.go
Falling back to patching base and 3-way merge...
Auto-merging test/unit/update_deps_test.go
CONFLICT (content): Merge conflict in test/unit/update_deps_test.go
Auto-merging test/unit/sharedlib_test.go
CONFLICT (content): Merge conflict in test/unit/sharedlib_test.go
Auto-merging test/unit/library_test.go
Auto-merging presubmit-tests.sh
Auto-merging library.sh
Auto-merging codegen-library.sh
CONFLICT (content): Merge conflict in codegen-library.sh
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 Add resolve_dep_version function

Details

In response to this:

/cherrypick release-1.15
/cherrypick release-1.16
/cherrypick release-1.17
/cherrypick release-1.18
/cherrypick release-1.19

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@cardil
Copy link
Contributor Author

cardil commented Oct 16, 2025

/cherrypick release-1.15

@knative-prow-robot
Copy link

@cardil: new pull request created: #440

Details

In response to this:

/cherrypick release-1.15

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@cardil
Copy link
Contributor Author

cardil commented Oct 16, 2025

/cherrypick release-1.16
/cherrypick release-1.17
/cherrypick release-1.18
/cherrypick release-1.19

@knative-prow-robot
Copy link

@cardil: new pull request created: #441

Details

In response to this:

/cherrypick release-1.16
/cherrypick release-1.17
/cherrypick release-1.18
/cherrypick release-1.19

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@knative-prow-robot
Copy link

@cardil: new pull request created: #442

Details

In response to this:

/cherrypick release-1.16
/cherrypick release-1.17
/cherrypick release-1.18
/cherrypick release-1.19

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@knative-prow-robot
Copy link

@cardil: new pull request created: #443

Details

In response to this:

/cherrypick release-1.16
/cherrypick release-1.17
/cherrypick release-1.18
/cherrypick release-1.19

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@knative-prow-robot
Copy link

@cardil: new pull request created: #444

Details

In response to this:

/cherrypick release-1.16
/cherrypick release-1.17
/cherrypick release-1.18
/cherrypick release-1.19

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Using knative.dev/toolbox for older versions of Golang is broken

3 participants