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

docker: fix dart-sass arch mismatch, /cache permissions #12959

Merged
merged 2 commits into from
Oct 17, 2024

Conversation

dvdksn
Copy link
Contributor

@dvdksn dvdksn commented Oct 17, 2024

The dart-sass setup used the $BUILDARCH variable to fetch the dart-sass
binary, which resulted in the wrong architecture being downloaded when building
for another architecture. The first commit changes it to use $TARGETARCH
instead. It also simplifies/inlines the dart-sass setup.

The second commit fixes the permission issue with /cache being owned by root.

@dvdksn
Copy link
Contributor Author

dvdksn commented Oct 17, 2024

@bep can you try building a dart-sass project with this version? I pushed an image of this build to davidkarlsson416/hugo:12959

docker run -v .:/project davidkarlsson416/hugo:12959 build

@bep
Copy link
Member

bep commented Oct 17, 2024

docker run -v .:/project davidkarlsson416/hugo:12959 build

Works great, thanks.

The Docker build currently installs curl as a runtime dependency in the
final Dockerfile stage. The shell script to install the dart-sass is
also included in the final image. This change moves the dart-sass
download to a separate stage and copies the binary over to the final
stage.

It also fixes dart-sass for multi-platform builds by changing BUILDARCH
to TARGETARCH, ensuring that the dart-sass architecture matches the
output platform instead of the build platform.

Signed-off-by: David Karlsson <[email protected]>
@dvdksn dvdksn force-pushed the improve-dart-sass-setup-docker branch from 700c52e to 54011cf Compare October 17, 2024 13:39
@dvdksn
Copy link
Contributor Author

dvdksn commented Oct 17, 2024

@bep I saw your comment about ARG/ENV. I pushed a small edit to the first commit to make DART_SASS_VERSION work like the other version args. It makes it easier to maintain, and I like to stick with ENV only when I actually want to set an env var in the runtime container or use it to configure a build tool, like GOOS. Hope it makes sense.

@bep bep merged commit b5852d0 into gohugoio:master Oct 17, 2024
6 checks passed
@dvdksn dvdksn deleted the improve-dart-sass-setup-docker branch October 17, 2024 13:45
@bep
Copy link
Member

bep commented Oct 17, 2024

t makes it easier to maintain, and I like to stick with ENV only when I actually want to set an env var in the runtime container or use it to configure a build tool, like GOOS. Hope it makes sense.

@dvdksn yea, I removed that comment as I saw that we used ARG for the go versions. We can handle this later, but your argument only makes sense if Docker had a concept of "private ARGs".

As I understand it:

  • ARG can be set in argument to docker build and is such part of the "build API"
  • ENV is visible in the OS environment of the current shell, but not from the "outside" (e.g. not when running hugo, not as arguments to docker build).

@dvdksn
Copy link
Contributor Author

dvdksn commented Oct 17, 2024

Yes I get it, the ability to set these args on the command line is not desirable. But ARG is the better way to go for build-time variables, even if you don't plan on overriding them.

@bep
Copy link
Member

bep commented Oct 17, 2024

But ARG is the better way to go for build-time variables, even if you don't plan on overriding them.

How is it better? My argument is above is that the only practical difference in the "dart sass version" case above is that ARG can be set from the outside, which is a negative; what are the positive sides of ARG that outweighs the negative?

@dvdksn
Copy link
Contributor Author

dvdksn commented Oct 17, 2024

In this specific case, no real benefit; it's just more idiomatic. I think it's better to use ENV if you explicitly want an environment variable to configure a build step, or if you want to set an environment variable that persists in the final image and in containers created from it. And use ARG for variables that are only used to influence build execution.

@jmooring
Copy link
Member

https://www.docker.com/blog/docker-best-practices-using-arg-and-env-in-your-dockerfiles/#tldr

Also note:

Both ARG and ENV can be overridden from the command line in docker run and docker compose

@bep
Copy link
Member

bep commented Oct 17, 2024

Both ARG and ENV can be overridden from the command line in docker run and docker compose

I have not found any example/documentation about how to pass ENV variables from the command line to docker build.

@dvdksn
Copy link
Contributor Author

dvdksn commented Oct 17, 2024

I have not found any example/documentation about how to pass ENV variables from the command line to docker build.

You can't. You can't change the value of ENV outside the Dockerfile itself (unless you use ENV foo=$ARG...)

@jmooring
Copy link
Member

Elsewhere in that article the author provides this example (which I have not tested):

$ docker run --rm -e THEENV=bar envtest

@bep
Copy link
Member

bep commented Oct 17, 2024

docker run --rm -e THEENV=bar envtest

Sure, but the discussion above is about docker build and I see no -e option there.

@jmooring
Copy link
Member

Gotcha. When reading the article I didn't notice that docker build was excluded.

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

Successfully merging this pull request may close these issues.

Dockerfile Dart Sass permission issue on MacOS Check cache dir creation permissions vs Docker image
3 participants