Update hack scripts to shim temp GOPATH as needed #4465
Conversation
|
/kind bug |
|
This uses the workaround that @vdemeester documented here: #3884 (comment) It is not pretty but it works around the problem. |
|
/test pull-tekton-pipeline-integration-tests |
hack/update-codegen.sh
Outdated
| @@ -82,3 +85,5 @@ ${REPO_ROOT_DIR}/hack/update-deps.sh | |||
|
|
|||
| # Make sure the OpenAPI specification and Swagger file are up-to-date | |||
| ${REPO_ROOT_DIR}/hack/update-openapigen.sh | |||
|
|
|||
| shim_gopath_clean | |||
There was a problem hiding this comment.
Maybe trap shim_gopath_clean EXIT and move this above, so this gets done even if the script is interrupted? Otherwise lgtm, and thanks for doing this!
|
The failure here was our old friend /test pull-tekton-pipeline-alpha-integration-tests |
|
/approve |
There was a problem hiding this comment.
One question around the path used. For reference, in docker, in the early days (before vendor was supported by go, etc..) we would use a .gopath with links in the root of the source (and have it in the .gitignore of course) for it to work no matter which system it would be on.
Seems like it's still there.
hack/setup-temporary-gopath.sh
Outdated
| # and openapigen to execute in. This is only done if | ||
| # the current repo directory is not within GOPATH. | ||
| function shim_gopath() { | ||
| local TEMP_GOPATH="/tmp/tekton-pipelines-codegen-gopath" |
There was a problem hiding this comment.
Any reason to use than instead of mktemp ?
One reason to use mktemp would be that not all systems use /tmp as temporary directory, and usually mktemp handle this 🙃.
There was a problem hiding this comment.
I initially avoided that because mktemp on Mac has non-standard behaviour, meaning more shell code to work around it. I don't feel strongly about it but in the moment writing it felt like just using /tmp was going to be clearer if / when it breaks on a system without a /tmp directory.
|
Something really strange is happening when the dependencies install to the temporary GOPATH: many packages end up created with |
|
OK the |
When we run our codegen and openapigen scripts from a directory that is not under GOPATH the generated output uses relative prefixes instead of absolute prefixes. The new hack/setup-temporary-gopath.sh script detects if the repo is not in GOPATH and creates a temporary one under .gopath if so. codegen and openapigen scripts now depend on setup-temporary-gopath.sh before generating anything.
|
In response to this:
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/test-infra repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: imjasonh, vdemeester The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
previously seen as a flake in #3878 (comment) /test pull-tekton-pipeline-unit-tests |
|
/test pull-tekton-pipeline-unit-tests |
|
/test tekton-pipeline-unit-tests |
|
I think this does not work if GOPATH isn't set: |
Changes
When we run our codegen and openapigen scripts from a directory that is
not under GOPATH the generated output uses relative prefixes instead of
absolute prefixes.
The new hack/setup-temporary-gopath.sh script detects if the repo is not in
GOPATH and creates a temporary one under ".gopath" if so. codegen and
openapigen scripts now depend on setup-temporary-gopath.sh before
generating anything.
Fixes #3884
Fixes #4430
... for some definition of "fixes" ...
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
Release Notes