-
-
Notifications
You must be signed in to change notification settings - Fork 308
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: Address build warnings in Ubuntu Dockerfile #4090
base: main
Are you sure you want to change the base?
Conversation
Do you want to try using the hadolint linter too? It also makes use of shellcheck when it is available on the system. There's more suggestions that will be made. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick first review, (only the first file as it should be synced with the other), and only the diff.
I'll need to take a deeper look with the full context if it still makes sense with the full picture.
#ENV MYCFLAGS="-O2 -std=gnu99 -m64" | ||
#ENV MYLDFLAGS="-s" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't be needed if these are set as args at the top of the Dockerfile
Dockerfile
Outdated
ARG MYLDFLAGS="-s -Wl,--no-undefined -lblas" | ||
ARG MYCFLAGS="-O2 -std=gnu99 -m64" | ||
ARG MYCXXFLAGS="" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it a convention to use these variable names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, they have just been copied from my old compile script from decades ago and carried around since then :-D
Feel free to change the variable names.
Are you familiar with the difference between ARG and ENV? There's also a pattern to use to have build args passed between stages, and includes setting the ARG's value to an ENV. During the community sprint, me and @neteler fixed a situation where the variables were actually empty and didn't do what was expected, thus the build has been doing the wrong thing for a while. If you have the chance to take a look at the build artifacts on docker desktop (the docker builds aren't made yet on PRs here), or simply do the build on a computer with Docker Desktop, you could take a look at the resulting info (and args) that remain in the layers, and see if the correct ones are used and passed between layers. Otherwise, if you'd want to limit the scope of the PR, apart from commented out stuff, and conditional to a full review of where each ENV+ARG is defined and used, this PR is quite fine by itself. (The AS and setting of env with a = instead of the deprecated space between is totally trivial) |
I added Open Container Initiative (OCI) metadata to the image using build arguments (https://github.com/opencontainers/image-spec/blob/main/annotations.md). It currently requires users to set and pass the arguments. I will have a look at how this could be avoided / use default values if not provided by the user... |
There were a couple of warnings about undefined environment variables (and style) when building the ubuntu docker image.
This PR addresses those and also limits numpy to < 2.0. While GRASS GIS is compatible with NumPy 2.0, a bunch of python packages that may be used wit GRASS GIS are not, so for the time being I suggest to ship NumPy < 2...
I guess it would be possible to use ARG instead of ENV for a bunch of variables used, so less environment variables are set in the final docker image... Should that be changed?