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

Updated Dockerfile* and .dockerignore #178

Merged
merged 1 commit into from
May 13, 2022

Conversation

mtelvers
Copy link
Member

To fix --version parameter

@MisterDA
Copy link
Contributor

I think that dune subst is useless here, we're using dune-build-info to embed the build information, and we're not substituting strings like VERSION in the source.

@mtelvers
Copy link
Member Author

I have tried it without dune subst and it does seem to be necessary:

$ grep subst Dockerfile.worker
RUN opam exec -- dune subst

Then build it and test:

~/ocluster$ docker build -f Dockerfile.worker .
Sending build context to Docker daemon  1.726MB
Step 1/17 : FROM ocaml/opam:debian-11-ocaml-4.13@sha256:fe25836078456e1691b59e28b8fdf1a1feb34f122c9810642282d5d9530fb4ef AS build
...
Successfully built b8852f6f0651
~/ocluster$ docker run b8852f6f0651 --version
v0.1-101-g18e2b79

If I remove the line: sed -i '/dune subst/d' Dockerfile.worker

~/ocluster$ docker build -f Dockerfile.worker .
Sending build context to Docker daemon  1.726MB
Step 1/16 : FROM ocaml/opam:debian-11-ocaml-4.13@sha256:fe25836078456e1691b59e28b8fdf1a1feb34f122c9810642282d5d9530fb4ef AS build
...
Successfully built 54a00130a9d5
~/ocluster$ docker run 54a00130a9d5 --version
n/a

@MisterDA
Copy link
Contributor

I've tested it too and it's correct, but I don't understand why dune subst works in this case. Without it however it's clear what's happening: dune doesn't embed build information unless the executable gets promoted or installed.

I'd be interested in knowing why the build info gets embedded in that case. Otherwise, something that works too and imo isn't so bad:

diff --git a/Dockerfile.worker b/Dockerfile.worker
index 8d52928..f0fc39a 100644
--- a/Dockerfile.worker
+++ b/Dockerfile.worker
@@ -7,12 +7,12 @@ RUN opam pin -yn /src/obuilder/
 WORKDIR /src
 RUN opam install -y --deps-only .
 ADD --chown=opam . .
-RUN opam exec -- dune subst
-RUN opam config exec -- dune build ./_build/install/default/bin/ocluster-worker
+RUN opam config exec -- dune build -p ocluster,ocluster-api @install
+RUN opam config exec -- dune install --prefix=/usr/local --destdir=pkg --section=bin --relocatable ocluster

 FROM debian:11
 RUN apt-get update && apt-get install docker.io libev4 curl gnupg2 git libsqlite3-dev ca-certificates netbase -y --no-install-recommends
 WORKDIR /var/lib/ocluster-worker
 ENTRYPOINT ["/usr/local/bin/ocluster-worker"]
 ENV PROGRESS_NO_TRUNC=1
-COPY --from=build /src/_build/install/default/bin/ocluster-worker /usr/local/bin/
+COPY --from=build /src/pkg/usr/local/bin/ocluster-worker /usr/local/bin/

@MisterDA
Copy link
Contributor

ah, dune subst also creates a (version ...) stanza in the _build directory, that's why it works.

@MisterDA MisterDA self-requested a review May 12, 2022 15:30
@mtelvers mtelvers merged commit 18888c5 into ocurrent:master May 13, 2022
@mtelvers mtelvers deleted the fix-version-parameter branch May 13, 2022 10:25
tmcgilchrist added a commit to tmcgilchrist/opam-repository that referenced this pull request Nov 9, 2022
CHANGES:

- Update OBuilder to pull in Windows prereqs (@MisterDA ocurrent/ocluster#196, reviewed by @tmcgilchrist)
- Worker pool capacity metric (@mtelvers ocurrent/ocluster#195, reviewed by @talex5)
- Update Ocluster to be macOS capable (@patricoferris ocurrent/ocluster#152, reviewed by @tmcgilchrist)
- Use rsync-hardlink from OBuilder rsync store (@MisterDA ocurrent/ocluster#189, reviewed by @tmcgilchrist)
- Various Dune 3 fixes (@MisterDA ocurrent/ocluster#187, reviewed by @talex5)
- Switch from Debian to Ubuntu for Worker Builds (@mtelvers ocurrent/ocluster#185, reviewed by @dra27)
- Update to Prometheus 1.2 (@mtelvers ocurrent/ocluster#183, reviewed by @MisterDA)
- Default to GitLab merge-requests refspecs (@MisterDA ocurrent/ocluster#180, reviewed by @tmcgilchrist)
- GitLab: allow fetching merge-requests from remote origin (@MisterDA ocurrent/ocluster#175, reviewed by @tmcgilchrist)
- Updated Dockerfile* and .dockerignore (@mtelvers ocurrent/ocluster#178, reviewed by @tmcgilchrist and @MisterDA)
- Upgrade lwt to 5.5.0 (@maiste ocurrent/ocluster#171 ocurrent/ocluster#181, reviewed by @dra27)
- Added --terse option to ocluster-admin-show and added new command ocluster-admin-exec (@mtelvers ocurrent/ocluster#165, reviewed by @tmcgilchrist and @dra27)
- Support custom jobs, adds custom job specifications to the cluster API. (@patricoferris ocurrent/ocluster#156, reviewed by @talex5)
- Continuing the joy of submodule URL changes (@dra27 ocurrent/ocluster#164)
- Update ocaml/opam-repository SHA includes tar-unix.2.0.1 (@mtelvers ocurrent/ocluster#161, reviewed by @dra27)
- Run git submodule update when resetting the git clone (@MisterDA ocurrent/ocluster#163, reviewed by @tmcgilchrist)
- Cmdliner.1.1.0 support (@MisterDA ocurrent/ocluster#160)
- Explicitly set confirmation levels to allow for manually triggered jobs. (@tmcgilchrist ocurrent/ocluster#159, reviewed by @TheLortex)
- Merge Obuilder with rsync store (@MisterDA ocurrent/ocluster#155, reviewed by @talex5)
- Sqlite3 remove usage of "FALSE" for compatibility with older versions (@art-w ocurrent/ocluster#150, reviewed by @talex5)
- Revert adopting GNU tar format (@dra27 ocurrent/ocluster#148)
- Update obuilder to latest (@mtelvers ocurrent/ocluster#147)
- Fix deprecations in Fmt 0.8.10 (@tmcgilchrist ocurrent/ocluster#145)
- Lwt_unix.yield was deprecated in favor of Lwt.pause (@MisterDA ocurrent/ocluster#142, reviewed by @dra27)
- Remove runc build from Dockerfile.worker (@talex5 ocurrent/ocluster#140)
- Add Windows support (@MisterDA ocurrent/ocluster#128, reviewed by @dra27 and @talex5)
- Admin client: fix ref-counting on progress display (@talex5 ocurrent/ocluster#138)
- Add --verbose to README examples (@talex5 ocurrent/ocluster#137)
- Use --connect for the worker capability too (@talex5 ocurrent/ocluster#136)
- Make free-space check work on Windows (@talex5 ocurrent/ocluster#134)
- Support Fmt.cli and Logs.cli (@MisterDA ocurrent/ocluster#133, reviewed by @talex5)
- Windows support prerequisites (@talex5 and @MisterDA ocurrent/ocluster#132)
- Update to capnp-rpc 1.2 and fix connection handling (@talex5 ocurrent/ocluster#131)
- Improve reporting of connection errors (@talex5 ocurrent/ocluster#130)
- Depend on more current_* modules to build examples (@MisterDA ocurrent/ocluster#129, reviewed by @talex5)
- Add ca-certificates to Dockerfile (@talex5 ocurrent/ocluster#127)
- API to wait for a worker to drain, useful for scripts that need to wait for a worker to stop before continuing. (@talex5 ocurrent/ocluster#126)
- Allow pausing/unpausing/forgetting unconnected workers (@talex5 ocurrent/ocluster#125)
- Fix ref-leak when rejecting duplicate workers (@talex5 ocurrent/ocluster#124)
- Improve handling of a worker's active state (@talex5 ocurrent/ocluster#123)
- Report better error on duplicate worker registration (@talex5 ocurrent/ocluster#122)
- Switch pool tests to expect tests (@talex5 ocurrent/ocluster#121)
- Add timestamps to OBuilder logs (@talex5 ocurrent/ocluster#120)
- Fix opam constraint on digestif (@kit-ty-kate ocurrent/ocluster#118, reviewed by @talex5)
- Add support for secrets. Secrets are a way to transmit sensitive key-value pairs to workers, without having them displayed in any log files. (@TheLortex ocurrent/ocluster#116, reviewed by @talex5)
- Add optional label to build_obuilder (@TheLortex ocurrent/ocluster#113, reviewed by @talex5)
dinosaure pushed a commit to tmcgilchrist/opam-repository that referenced this pull request Dec 8, 2022
CHANGES:

- Update OBuilder to pull in Windows prereqs (@MisterDA ocurrent/ocluster#196, reviewed by @tmcgilchrist)
- Worker pool capacity metric (@mtelvers ocurrent/ocluster#195, reviewed by @talex5)
- Update Ocluster to be macOS capable (@patricoferris ocurrent/ocluster#152, reviewed by @tmcgilchrist)
- Use rsync-hardlink from OBuilder rsync store (@MisterDA ocurrent/ocluster#189, reviewed by @tmcgilchrist)
- Various Dune 3 fixes (@MisterDA ocurrent/ocluster#187, reviewed by @talex5)
- Switch from Debian to Ubuntu for Worker Builds (@mtelvers ocurrent/ocluster#185, reviewed by @dra27)
- Update to Prometheus 1.2 (@mtelvers ocurrent/ocluster#183, reviewed by @MisterDA)
- Default to GitLab merge-requests refspecs (@MisterDA ocurrent/ocluster#180, reviewed by @tmcgilchrist)
- GitLab: allow fetching merge-requests from remote origin (@MisterDA ocurrent/ocluster#175, reviewed by @tmcgilchrist)
- Updated Dockerfile* and .dockerignore (@mtelvers ocurrent/ocluster#178, reviewed by @tmcgilchrist and @MisterDA)
- Upgrade lwt to 5.5.0 (@maiste ocurrent/ocluster#171 ocurrent/ocluster#181, reviewed by @dra27)
- Added --terse option to ocluster-admin-show and added new command ocluster-admin-exec (@mtelvers ocurrent/ocluster#165, reviewed by @tmcgilchrist and @dra27)
- Support custom jobs, adds custom job specifications to the cluster API. (@patricoferris ocurrent/ocluster#156, reviewed by @talex5)
- Continuing the joy of submodule URL changes (@dra27 ocurrent/ocluster#164)
- Update ocaml/opam-repository SHA includes tar-unix.2.0.1 (@mtelvers ocurrent/ocluster#161, reviewed by @dra27)
- Run git submodule update when resetting the git clone (@MisterDA ocurrent/ocluster#163, reviewed by @tmcgilchrist)
- Cmdliner.1.1.0 support (@MisterDA ocurrent/ocluster#160)
- Explicitly set confirmation levels to allow for manually triggered jobs. (@tmcgilchrist ocurrent/ocluster#159, reviewed by @TheLortex)
- Merge Obuilder with rsync store (@MisterDA ocurrent/ocluster#155, reviewed by @talex5)
- Sqlite3 remove usage of "FALSE" for compatibility with older versions (@art-w ocurrent/ocluster#150, reviewed by @talex5)
- Revert adopting GNU tar format (@dra27 ocurrent/ocluster#148)
- Update obuilder to latest (@mtelvers ocurrent/ocluster#147)
- Fix deprecations in Fmt 0.8.10 (@tmcgilchrist ocurrent/ocluster#145)
- Lwt_unix.yield was deprecated in favor of Lwt.pause (@MisterDA ocurrent/ocluster#142, reviewed by @dra27)
- Remove runc build from Dockerfile.worker (@talex5 ocurrent/ocluster#140)
- Add Windows support (@MisterDA ocurrent/ocluster#128, reviewed by @dra27 and @talex5)
- Admin client: fix ref-counting on progress display (@talex5 ocurrent/ocluster#138)
- Add --verbose to README examples (@talex5 ocurrent/ocluster#137)
- Use --connect for the worker capability too (@talex5 ocurrent/ocluster#136)
- Make free-space check work on Windows (@talex5 ocurrent/ocluster#134)
- Support Fmt.cli and Logs.cli (@MisterDA ocurrent/ocluster#133, reviewed by @talex5)
- Windows support prerequisites (@talex5 and @MisterDA ocurrent/ocluster#132)
- Update to capnp-rpc 1.2 and fix connection handling (@talex5 ocurrent/ocluster#131)
- Improve reporting of connection errors (@talex5 ocurrent/ocluster#130)
- Depend on more current_* modules to build examples (@MisterDA ocurrent/ocluster#129, reviewed by @talex5)
- Add ca-certificates to Dockerfile (@talex5 ocurrent/ocluster#127)
- API to wait for a worker to drain, useful for scripts that need to wait for a worker to stop before continuing. (@talex5 ocurrent/ocluster#126)
- Allow pausing/unpausing/forgetting unconnected workers (@talex5 ocurrent/ocluster#125)
- Fix ref-leak when rejecting duplicate workers (@talex5 ocurrent/ocluster#124)
- Improve handling of a worker's active state (@talex5 ocurrent/ocluster#123)
- Report better error on duplicate worker registration (@talex5 ocurrent/ocluster#122)
- Switch pool tests to expect tests (@talex5 ocurrent/ocluster#121)
- Add timestamps to OBuilder logs (@talex5 ocurrent/ocluster#120)
- Fix opam constraint on digestif (@kit-ty-kate ocurrent/ocluster#118, reviewed by @talex5)
- Add support for secrets. Secrets are a way to transmit sensitive key-value pairs to workers, without having them displayed in any log files. (@TheLortex ocurrent/ocluster#116, reviewed by @talex5)
- Add optional label to build_obuilder (@TheLortex ocurrent/ocluster#113, reviewed by @talex5)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants