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 dockerfile to build correct go binary #54

Merged
merged 2 commits into from
Dec 19, 2022

Conversation

arnopensource
Copy link
Contributor

Removed 2 superfluous lines from docker/Dockerfile.multiarch that caused undefined environment variables in the Makefile which caused amd64 go builds in all cases

More details on woodpecker-ci/woodpecker#1443

@lafriks
Copy link
Contributor

lafriks commented Dec 18, 2022

Why is ARG HOME removed?

@arnopensource
Copy link
Contributor Author

Because it is still at line 11

Since the duplicated line for TARGEOS and TARGETARCH created a bug I though it was the same for HOME

I didn't test it though, I will do it now

@arnopensource
Copy link
Contributor Author

Ok I searched a little but I have no idea how the HOME arg should be used :/
And I just realized that I deleted the default value (/app) of the argument

Any help on the subject would be appreciated

@lafriks
Copy link
Contributor

lafriks commented Dec 18, 2022

@6543 can you help on this?

@anbraten
Copy link
Member

anbraten commented Dec 18, 2022

Have a look at https://docs.docker.com/engine/reference/builder/#understand-how-arg-and-from-interact

From reading the buildx docs again I would confirm that TARGETOS, ... is directly passed by buildx. It then can be used inside the dockerfile / build run process. I guess setting it above FROM will override the values passed by buildx.

@anbraten
Copy link
Member

For home I would just add the default to the lower arg home line.

@arnopensource
Copy link
Contributor Author

arnopensource commented Dec 19, 2022

I spent some time in the docs to understand the arguments coming from buildx and their scopes, see:

Same conclusion as @anbraten :

  • declaring TARGETOS and TARGETARCH in the global scope was overriding the values coming from buildx
  • we should be fine using only the ARG HOME in the relevant build step since it is only used here, I will add the default value that I accidentally deleted

Since the ARG HOME is not at the top of the file anymore, it is less visible. I could maybe add a line in the README.md about it ?

@anbraten anbraten merged commit 4447028 into woodpecker-ci:master Dec 19, 2022
@arnopensource arnopensource deleted the arm-fix branch December 19, 2022 17:43
@6543
Copy link
Member

6543 commented Dec 19, 2022

hah that it was - thanks ❤️

@6543 6543 added the bug Something isn't working label Dec 19, 2022
@6543 6543 added this to the v2.1.0 milestone Dec 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants