Skip to content
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

fix(build): correct make target deps for Go binaries #13145

Merged
merged 5 commits into from
Aug 15, 2024

Conversation

Joibel
Copy link
Member

@Joibel Joibel commented Jun 5, 2024

Fixes the makefile when editing files under certain circumstances.

Motivation

@Adrien-D was editing the files using vscode on a Mac and finding that although kit was attempting a rebuild when his file was saved the result was not running.

This was tracked down to make dist/argo (he was working on the server component) reporting 'dist/argo' is up to date.

The binary targets only depend upon the timestamps of the package subdirectories, not the timestamps of the .go files within.

Modifications

This file changes the dependencies for the three golang binaries in the makefile to be the .go files under all the packages, rather than just the package directories

libc-dev doesn't make it into the final image, just the builder image. Because we now depend on the .go files we need to be able to rebuild pkg/apis/workflow/v1alpha1/openapi_generated.go, who's make target depends upon /go/bin/openapi-gen which needs libc-dev to build.

Verification

See the last make in this for evidence of the problem, and the second for proof it works.

vscode ➜ ~/go/src/github.com/argoproj/argo-workflows (01bf1cfc8) $ make dist/workflow-controller
GIT_COMMIT=01bf1cfc82834a5fc50bcba42cdacc0eb1fbcde1 GIT_BRANCH=HEAD GIT_TAG=untagged GIT_TREE_STATE=dirty RELEASE_TAG=false DEV_BRANCH=true VERSION=latest
KUBECTX=k3d-k3s-default DOCKER_DESKTOP=false K3D=true DOCKER_PUSH=false
RUN_MODE=local PROFILE=minimal AUTH_MODE=hybrid SECURE=false STATIC_FILES=false ALWAYS_OFFLOAD_NODE_STATUS=false UPPERIO_DB_DEBUG=0 LOG_LEVEL=debug NAMESPACED=true
make: 'dist/workflow-controller' is up to date.
vscode ➜ ~/go/src/github.com/argoproj/argo-workflows (01bf1cfc8) $ touch workflow/controller/controller.go
vscode ➜ ~/go/src/github.com/argoproj/argo-workflows (01bf1cfc8) $ make dist/workflow-controller
GIT_COMMIT=01bf1cfc82834a5fc50bcba42cdacc0eb1fbcde1 GIT_BRANCH=HEAD GIT_TAG=untagged GIT_TREE_STATE=dirty RELEASE_TAG=false DEV_BRANCH=true VERSION=latest
KUBECTX=k3d-k3s-default DOCKER_DESKTOP=false K3D=true DOCKER_PUSH=false
RUN_MODE=local PROFILE=minimal AUTH_MODE=hybrid SECURE=false STATIC_FILES=false ALWAYS_OFFLOAD_NODE_STATUS=false UPPERIO_DB_DEBUG=0 LOG_LEVEL=debug NAMESPACED=true
CGO_ENABLED=0 go build -gcflags '' -v -ldflags '-X github.com/argoproj/argo-workflows/v3.version=latest -X github.com/argoproj/argo-workflows/v3.buildDate=2024-06-05T13:10:23Z -X github.com/argoproj/argo-workflows/v3.gitCommit=01bf1cfc82834a5fc50bcba42cdacc0eb1fbcde1 -X github.com/argoproj/argo-workflows/v3.gitTreeState=dirty -X github.com/argoproj/argo-workflows/v3.gitTag=untagged -extldflags -static' -o dist/workflow-controller ./cmd/workflow-controller
github.com/argoproj/argo-workflows/v3/cmd/workflow-controller
gitvscode ➜ ~/go/src/github.com/argoproj/argo-workflows (01bf1cfc8) $ git checkout HEAD~ -- Makefile
vscode ➜ ~/go/src/github.com/argoproj/argo-workflows (01bf1cfc8) $ make dist/workflow-controller
GIT_COMMIT=01bf1cfc82834a5fc50bcba42cdacc0eb1fbcde1 GIT_BRANCH=HEAD GIT_TAG=untagged GIT_TREE_STATE=dirty RELEASE_TAG=false DEV_BRANCH=true VERSION=latest
KUBECTX=k3d-k3s-default DOCKER_DESKTOP=false K3D=true DOCKER_PUSH=false
RUN_MODE=local PROFILE=minimal AUTH_MODE=hybrid SECURE=false STATIC_FILES=false ALWAYS_OFFLOAD_NODE_STATUS=false UPPERIO_DB_DEBUG=0 LOG_LEVEL=debug NAMESPACED=true
make: 'dist/workflow-controller' is up to date.
vscode ➜ ~/go/src/github.com/argoproj/argo-workflows (01bf1cfc8) $ touch workflow/controller/controller.go
vscode ➜ ~/go/src/github.com/argoproj/argo-workflows (01bf1cfc8) $ make dist/workflow-controller
GIT_COMMIT=01bf1cfc82834a5fc50bcba42cdacc0eb1fbcde1 GIT_BRANCH=HEAD GIT_TAG=untagged GIT_TREE_STATE=dirty RELEASE_TAG=false DEV_BRANCH=true VERSION=latest
KUBECTX=k3d-k3s-default DOCKER_DESKTOP=false K3D=true DOCKER_PUSH=false
RUN_MODE=local PROFILE=minimal AUTH_MODE=hybrid SECURE=false STATIC_FILES=false ALWAYS_OFFLOAD_NODE_STATUS=false UPPERIO_DB_DEBUG=0 LOG_LEVEL=debug NAMESPACED=true
make: 'dist/workflow-controller' is up to date.

This works well under native linux and under the .devcontainer docker.

If someone could verify the xargs+find invocation I use actually works under native Mac, that would be good. I know MacOS ships with some horrifically outdated unix tools.

Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

Thanks! Great write-up in the description.

@Joibel Joibel marked this pull request as draft June 5, 2024 14:06
@agilgur5 agilgur5 added area/build Build or GithubAction/CI issues area/contributing Contributing docs, ownership, etc. Also devtools like devcontainer and Nix labels Jun 5, 2024
Makefile Outdated Show resolved Hide resolved
@Joibel
Copy link
Member Author

Joibel commented Jun 7, 2024

During CI the generate go files (specifically pkg/apis/workflow/v1alpha1/openapi_generated.go gets generated without a "newer than bin/openapi-gen" timestamp. This I cannot reproduce locally, but causes it to get continuously regenerated as out of date. So I've added a touch to it's generation, which fixes the CI.

This isn't the first time github's CI has had odd timestamp behaviour (#13023)

@Joibel Joibel marked this pull request as ready for review June 7, 2024 11:08
Copy link
Member

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

Ah I have had this issue before of the binaries failing to update, but hadn't looked into it as it was fairly simple to workaround. Thanks for figuring out the issue! That it was only using directories is hard to tell from a quick glance

I left some small suggestions below that might optimize this

Dockerfile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@agilgur5 agilgur5 changed the title fix(build): fix makefile dependencies for go binaries fix(build): proper make target deps for go binaries Jun 8, 2024
@agilgur5 agilgur5 changed the title fix(build): proper make target deps for go binaries fix(build): correct make target deps for go binaries Jun 8, 2024
The dependencies for the golang binaries are only dependent on the
directories not the actual files.

This means that if you just edit the file with vscode on a mac a
rebuild doesn't rebuild the binary. Some editors update the
timestamp of the directory as well, and then the old code works.

See the last make in this for evidence of the problem
```
vscode ➜ ~/go/src/github.com/argoproj/argo-workflows (01bf1cfc8) $ make dist/workflow-controller
GIT_COMMIT=01bf1cfc82834a5fc50bcba42cdacc0eb1fbcde1 GIT_BRANCH=HEAD GIT_TAG=untagged GIT_TREE_STATE=dirty RELEASE_TAG=false DEV_BRANCH=true VERSION=latest
KUBECTX=k3d-k3s-default DOCKER_DESKTOP=false K3D=true DOCKER_PUSH=false
RUN_MODE=local PROFILE=minimal AUTH_MODE=hybrid SECURE=false STATIC_FILES=false ALWAYS_OFFLOAD_NODE_STATUS=false UPPERIO_DB_DEBUG=0 LOG_LEVEL=debug NAMESPACED=true
make: 'dist/workflow-controller' is up to date.
vscode ➜ ~/go/src/github.com/argoproj/argo-workflows (01bf1cfc8) $ touch workflow/controller/controller.go
vscode ➜ ~/go/src/github.com/argoproj/argo-workflows (01bf1cfc8) $ make dist/workflow-controller
GIT_COMMIT=01bf1cfc82834a5fc50bcba42cdacc0eb1fbcde1 GIT_BRANCH=HEAD GIT_TAG=untagged GIT_TREE_STATE=dirty RELEASE_TAG=false DEV_BRANCH=true VERSION=latest
KUBECTX=k3d-k3s-default DOCKER_DESKTOP=false K3D=true DOCKER_PUSH=false
RUN_MODE=local PROFILE=minimal AUTH_MODE=hybrid SECURE=false STATIC_FILES=false ALWAYS_OFFLOAD_NODE_STATUS=false UPPERIO_DB_DEBUG=0 LOG_LEVEL=debug NAMESPACED=true
CGO_ENABLED=0 go build -gcflags '' -v -ldflags '-X github.com/argoproj/argo-workflows/v3.version=latest -X github.com/argoproj/argo-workflows/v3.buildDate=2024-06-05T13:10:23Z -X github.com/argoproj/argo-workflows/v3.gitCommit=01bf1cfc82834a5fc50bcba42cdacc0eb1fbcde1 -X github.com/argoproj/argo-workflows/v3.gitTreeState=dirty -X github.com/argoproj/argo-workflows/v3.gitTag=untagged -extldflags -static' -o dist/workflow-controller ./cmd/workflow-controller
github.com/argoproj/argo-workflows/v3/cmd/workflow-controller
gitvscode ➜ ~/go/src/github.com/argoproj/argo-workflows (01bf1cfc8) $ git checkout HEAD~ -- Makefile
vscode ➜ ~/go/src/github.com/argoproj/argo-workflows (01bf1cfc8) $ make dist/workflow-controller
GIT_COMMIT=01bf1cfc82834a5fc50bcba42cdacc0eb1fbcde1 GIT_BRANCH=HEAD GIT_TAG=untagged GIT_TREE_STATE=dirty RELEASE_TAG=false DEV_BRANCH=true VERSION=latest
KUBECTX=k3d-k3s-default DOCKER_DESKTOP=false K3D=true DOCKER_PUSH=false
RUN_MODE=local PROFILE=minimal AUTH_MODE=hybrid SECURE=false STATIC_FILES=false ALWAYS_OFFLOAD_NODE_STATUS=false UPPERIO_DB_DEBUG=0 LOG_LEVEL=debug NAMESPACED=true
make: 'dist/workflow-controller' is up to date.
vscode ➜ ~/go/src/github.com/argoproj/argo-workflows (01bf1cfc8) $ touch workflow/controller/controller.go
vscode ➜ ~/go/src/github.com/argoproj/argo-workflows (01bf1cfc8) $ make dist/workflow-controller
GIT_COMMIT=01bf1cfc82834a5fc50bcba42cdacc0eb1fbcde1 GIT_BRANCH=HEAD GIT_TAG=untagged GIT_TREE_STATE=dirty RELEASE_TAG=false DEV_BRANCH=true VERSION=latest
KUBECTX=k3d-k3s-default DOCKER_DESKTOP=false K3D=true DOCKER_PUSH=false
RUN_MODE=local PROFILE=minimal AUTH_MODE=hybrid SECURE=false STATIC_FILES=false ALWAYS_OFFLOAD_NODE_STATUS=false UPPERIO_DB_DEBUG=0 LOG_LEVEL=debug NAMESPACED=true
make: 'dist/workflow-controller' is up to date.
```

Signed-off-by: Alan Clucas <[email protected]>
This doesn't make it into the final image, just the builder image

Because we now depend on the .go files we need to be able to rebuild
`pkg/apis/workflow/v1alpha1/openapi_generated.go`, who's make target
depends upon `/go/bin/openapi-gen` which needs libc-dev to build.

Signed-off-by: Alan Clucas <[email protected]>
@agilgur5 agilgur5 changed the title fix(build): correct make target deps for go binaries fix(build): correct make target deps for Go binaries Aug 3, 2024
@Joibel Joibel merged commit b88a23a into argoproj:main Aug 15, 2024
28 checks passed
@Joibel Joibel deleted the makefile-files-fix branch August 15, 2024 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build Build or GithubAction/CI issues area/contributing Contributing docs, ownership, etc. Also devtools like devcontainer and Nix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants