-
Notifications
You must be signed in to change notification settings - Fork 367
chore: Adopt Dune Package Management for the OCaml.org Build #3281
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
base: main
Are you sure you want to change the base?
Conversation
|
Here is an update on the
FROM alpine:3.21 AS build
RUN apk -U upgrade --no-cache && apk add --no-cache curl
RUN curl -v -6 -sSL https://github.com/ocaml-dune/dune-bin-install/releases/download/v1/install.sh
docker: Dockerfile
docker build --no-cache --progress=plaisetup $< .This fails in my setup. However, if I replace Here is the error log: bidon>make default
docker build --no-cache --progress=plain -f Dockerfile .
#0 building with "default" instance using docker driver
#1 [internal] load build definition from Dockerfile
#1 transferring dockerfile: 223B done
#1 DONE 0.0s
#2 [internal] load metadata for docker.io/library/alpine:3.21
#2 DONE 0.0s
#3 [internal] load .dockerignore
#3 transferring context: 2B done
#3 DONE 0.0s
#4 [1/3] FROM docker.io/library/alpine:3.21
#4 CACHED
#5 [2/3] RUN apk -U upgrade --no-cache && apk add --no-cache curl
#5 0.111 fetch https://dl-cdn.alpinelinux.org/alpine/v3.21/main/x86_64/APKINDEX.tar.gz
#5 0.240 fetch https://dl-cdn.alpinelinux.org/alpine/v3.21/community/x86_64/APKINDEX.tar.gz
#5 0.432 (1/3) Upgrading busybox (1.37.0-r12 -> 1.37.0-r13)
#5 0.475 Executing busybox-1.37.0-r13.post-upgrade
#5 0.490 (2/3) Upgrading busybox-binsh (1.37.0-r12 -> 1.37.0-r13)
#5 0.503 (3/3) Upgrading ssl_client (1.37.0-r12 -> 1.37.0-r13)
#5 0.515 Executing busybox-1.37.0-r13.trigger
#5 0.519 OK: 7 MiB in 15 packages
#5 0.562 fetch https://dl-cdn.alpinelinux.org/alpine/v3.21/main/x86_64/APKINDEX.tar.gz
#5 0.694 fetch https://dl-cdn.alpinelinux.org/alpine/v3.21/community/x86_64/APKINDEX.tar.gz
#5 0.880 (1/9) Installing brotli-libs (1.1.0-r2)
#5 0.914 (2/9) Installing c-ares (1.34.5-r0)
#5 0.931 (3/9) Installing libunistring (1.2-r0)
#5 0.956 (4/9) Installing libidn2 (2.3.7-r0)
#5 0.977 (5/9) Installing nghttp2-libs (1.64.0-r0)
#5 0.990 (6/9) Installing libpsl (0.21.5-r3)
#5 1.004 (7/9) Installing zstd-libs (1.5.6-r2)
#5 1.026 (8/9) Installing libcurl (8.12.1-r1)
#5 1.045 (9/9) Installing curl (8.12.1-r1)
#5 1.081 Executing busybox-1.37.0-r13.trigger
#5 1.085 OK: 12 MiB in 24 packages
#5 DONE 1.1s
#6 [3/3] RUN curl -v -6 -sSL https://github.com/ocaml-dune/dune-bin-install/releases/download/v1/install.sh
#6 12.16 * Could not resolve host: github.com
#6 12.16 * shutting down connection #0
#6 12.16 curl: (6) Could not resolve host: github.com
#6 ERROR: process "/bin/sh -c curl -v -6 -sSL https://github.com/ocaml-dune/dune-bin-install/releases/download/v1/install.sh" did not complete successfully: exit code: 6
------
> [3/3] RUN curl -v -6 -sSL https://github.com/ocaml-dune/dune-bin-install/releases/download/v1/install.sh:
12.16 * Could not resolve host: github.com
12.16 * shutting down connection #0
12.16 curl: (6) Could not resolve host: github.com
------
Dockerfile:5
--------------------
3 | RUN apk -U upgrade --no-cache && apk add --no-cache curl
4 |
5 | >>> RUN curl -v -6 -sSL https://github.com/ocaml-dune/dune-bin-install/releases/download/v1/install.sh
6 |
--------------------
ERROR: failed to solve: process "/bin/sh -c curl -v -6 -sSL https://github.com/ocaml-dune/dune-bin-install/releases/download/v1/install.sh" did not complete successfully: exit code: 6
make: *** [Makefile:2: docker] Error 1This is pretty similar to the error I observed in Here is my current understanding of the problem
|
|
Heads up that I can reproduce the issue @cuihtlauac reports when attempting to use IP 6 to connect to github (from any OS, natively and inside docker). Maybe not the most official source but a quick google found https://doesgithubhaveipv6yet.com. I've never experienced |
|
Having a means to tweak the options passed to |
|
I think for now just pass -4 whenever |
|
FYI, I’ve just tagged |
|
@cuihtlauac if you don't see anything that blocks us from merging (and potentially fixing issues as they come - I'm ready for this), then let's merge! |
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.
Awesome work getting this all in such good shape. And thanks for the careful reviews, @cuihtlauac!
|
What is blocking this from being merged? (Aside from the merge conflict, which is a symptom of this not being completed, rather than a cause). |
|
I remain concerned about Both ran after a first successful build. The former is in this PR's current branch, the latter is in However, I have successfully built using |
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.
IMO there's nothing blocking, we just need to merge and if there's anything that comes up, we fix forward.
|
Yeah, the performance impact on |
|
I've pushed commit 1cd3871 to address the make clean issue |
|
Previously, |
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.
Looks mostly good to me. I recommend not merging on Friday and being ready to roll back if anything goes south. The PR can't test itself (In a convoluted but untested scenario, we could end up in a state where this PR is all green, merged and then subsequent PR don't build)
|
To reassure everyone, I tried running Note that on a different run I get the error about a different url ( |
|
Yesterday, I did the same test as @ElectreAAS, and it passed. Today, I did it again, and it failed. Here is the error I'm seeing: The only thing that changed from my side is the network. I was at my parents' yesterday, I'm at home today. We must conduct more tests to resolve this before merging :-( |
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.
make docker still fails either transiently or in relation to the network. This must be fixed before merging
|
AFAIU, the issue here is isolated to specific versions of Ubuntu and related to an interaction with Docker, apparently on specific networks. This does not seem to be within the purview of Thanks for the commendable work here, @sabine and @Sudha247, and for the careful reviews and diligence, @cuihtlauac and @ElectreAAS. |
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.
This worked for me. I have some suggestions on the Dockerfile.
|
I thought that buildkit syntax was enabled on the deployer, apparently it's not? # syntax=docker/dockerfile:1EDIT: I've looked at the deployer and buildkit should be enabled. Could you try adding this line? |
* use `COPY --link` Co-authored-by: Antonin Décimo <[email protected]>
* use `COPY --link` Co-authored-by: Antonin Décimo <[email protected]>
Keep the cache as this is a multi-staged build. This will avoid downloading the package index twice. Co-authored-by: Antonin Décimo <[email protected]>
* use `ADD --link` Co-authored-by: Antonin Décimo <[email protected]>
use `ADD` to download HTML compiler manuals repo Co-authored-by: Antonin Décimo <[email protected]>
Co-authored-by: Antonin Décimo <[email protected]>
Apparently even though BuildKit is enabled in the deployer, it still needs Dockerfiles to opt-in the syntax. Co-authored-by: Antonin Décimo <[email protected]>
Co-authored-by: Antonin Décimo <[email protected]>
…use it would otherwise complain about --install-root
|
After checking on Slack, I believe it was confirmed that adding I also updated the included Backstage OCaml post to reflect the current state of the build updates and (I believe) fully addressed @MisterDA's Oct 3 review, now. With this, I believe the PR is ready to merge. |
This is the reopen of #3132. Me and @cuihtlauac met to migrate all the open tasks over here. I hope we got them all!
Open issues:
make dockerfails on Ubuntu 22.04 with curl error 6General concerns that haven't been fully decided / documented:
Remaining simple todos::
README.md, section “Getting Started”versionparameter to the GitHub Action: update action to take a parameter 'version' to install stable Dune ocaml-dune/setup-dune#10, and the changes from this PR will only work afterwards)make cleanmust be implemented in such a way that it doesn't delete the dependencies (this would be equivalent to deleting the opam switch. All we needmake cleanto do is to delete all the build artifacts!)[ ] add a new commandmake wipethat deletes the dependencies? (This would be useful in the docs, to tell people to use this when they run into trouble because we advanced the opam-repository pin for ocaml.org's dependencies.)To put in maintenance backlog, without blocking this PR:
What Contributors and Maintainers Need to Do
In order to switch from the old opam-based build to the Dune build:
>= 3.20installedmakePerformance Impact
This Branch
Clean build, after using make clean:
Repeat build without changes:
Clean build, including installation of dependencies:
Before Patch
Clean build, after using make clean:
Repeat build without changes:
Clean build, including installation of dependencies: