-
Notifications
You must be signed in to change notification settings - Fork 360
chore: Use Dune package management #3132
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
I've been using dune package management to build this project whenever I've needed to contribute to it over the past year and it's worked fine until recently. Here are some PRs/issues relating to fixing the problems I've encountered recently: |
There's also currently an issue with the |
This is great! Thanks for the work, @sabine and @gridbugs. Upon offline discussion with Sabine, we wanted to test this on MacOS and Windows to ensure the workflow remains unaffected for contributors on those platforms. I tested with WSL2 (which is the recommended way to develop ocaml.org on Windows), and the ocaml.org project (with The ocaml.org playground doesn't build, as expected - as it seems to have some dependency issues. I'll try to investigate what's happening there! |
Sounds great! Looks like we'll be migrating fully to |
a279318
to
d48543c
Compare
Thanks @Sudha247 for sabine#53. This makes the playground dune build succeed. One thing I observe on my machine: Both on the main branch, and on this branch, running the playground build just deletes the |
HACKING.md
Outdated
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.
Replace all ````` by ````bash` in this file
dune-project
Outdated
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.
Could the removal of the dependency on Anton's river fork be merged ahead in a separate PR?
Dockerfile
Outdated
RUN opam install . --deps-only | ||
# Install Dune Developer Preview | ||
RUN curl -fsSL https://get.dune.build/install | sh | ||
RUN /bin/bash -c 'source "/root/.local/share/dune/env/env.bash"' |
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.
Does this have any effect? The environment disappears after this call.
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.
We must not download a dev preview on each build, that's brittle and wasteful.
Dockerfile
Outdated
RUN sudo apk -U upgrade --no-cache && sudo apk add --no-cache \ | ||
RUN apk -U upgrade --no-cache && apk add --no-cache \ | ||
# to download and install Dune Developer Preview with alpine:3.21 | ||
build-base patch tar ca-certificates git rsync curl sudo bash \ |
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 there a need for sudo
, rsync
, bash
or nano
?
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.
You also probably don't need curl
. Docker can itself download remote files. See https://docs.docker.com/reference/dockerfile/#adding-files-from-a-url
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.
Indeed, but dune uses curl internally for (url ..)
fields, so we still need it.
rpclib-lwt)) | ||
(astring (= 0.8.5)) | ||
(base-bigarray (= base)) | ||
(base-bytes (= base)) |
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.
I don't think it is customary to depend on all these base-*
packages. After all, they're just transitive dependencies of the compiler selected.
Makefile
Outdated
watch: ## Watch for the filesystem and rebuild on every change | ||
opam exec -- dune build @run -w --force --no-buffer | ||
dune build @run -w --force --no-buffer |
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.
I read the HACKING doc a bit quickly, and ran make watch
instead of make start
first:
$ make watch
dune build @run -w --force --no-buffer
File "src/ocamlorg_web/lib/dune", line 13, characters 2-15:
13 | mirage-kv-mem))
^^^^^^^^^^^^^
Error: Library "mirage-kv-mem" not found.
-> required by library "ocamlorg_web" in _build/default/src/ocamlorg_web/lib
-> required by executable main in src/ocamlorg_web/bin/dune:2
-> required by _build/default/src/ocamlorg_web/bin/main.exe
-> required by alias src/ocamlorg_web/bin/run in src/ocamlorg_web/bin/dune:8
######################################################################## 100.0%
Done: 95% (95/100, 5 left, 1 failed) (jobs: 1)Browserslist: caniuse-lite is outdated. Please run:
npx update-browserslist-db@latest
Why you should do it regularly: https://github.com/browserslist/update-db#readme
Rebuilding...
Done in 1911ms.
Shouldn't every rule that does a build depend on dune.lock
, instead of just a couple of them? (or taking a step back, I thought the usual thing to do is commit the dune.lock
, so I'm a bit confused about why it's being generated on the fly).
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.
thanks, you're right. need to update the Makefile
@@ -0,0 +1,14 @@ | |||
(version | |||
0.26.2+binary-ocaml-5.2.1-built-2024-12-04.0-x86_64-unknown-linux-musl) |
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.
Be careful checking this file in as it's not portable. This version of ocamlformat will only work on linux. I'd recommend adding the dev-tools.locks directory to .gitignore so it doesn't get checked in and each user can install the version of the dev tools appropriate to their platform.
The install script is ready here, thanks to @gridbugs' work: https://github.com/ocaml-dune/dune-bin-install @Sudha247 is planning to add documentation here about how to use it. It also sounds like we should take Steve's advice about ignoring the lock dir, and IIUC @cuihtlauac's review has been addressed and just needs to be re-requested to clear the change request? |
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.
Small remarks:
- File
.github/workflows/scrape_changelog.yml
should be updated. - There are a few remaining references to
olinkcheck
that should be removed, it is no longer used
Big question
Is this PR proposing to build ocaml.org with a version of Dune that changes daily? If that is the case, I must object. This is unwise. I support building ocaml.org as a regression test in Dune developer preview CI. However, using an unstable build tool in a production delivery chain is reckless.
(pin | ||
(name olinkcheck) | ||
(url "git+https://github.com/tarides/olinkcheck") | ||
(package | ||
(name olinkcheck))) |
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.
PR #3144 Removed olinkcheck. I dont think this is needed any longer
(pins olinkcheck) | ||
(version_preference newest) | ||
) |
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.
Same remark w.r.t. PR#3144
- name: Use Dune Developer Preview | ||
uses: ocaml-dune/setup-dune@v0 |
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.
I'm not sure I fully understand this.
Are we attempting to build ocaml.org with a build tool that changes daily?
The PR in its current state proposes this approach. However, as @shonfeder noted above, we can use @gridbugs' installer to install the released Dune binary (currently version 3.19.1). I'll update the PR and documentation accordingly. The idea is to use |
This patch changes the build configuration to use Dune Package Management.
Done / To do:
dune-workspace
to pinopam-repository
just as we currently do (moved the pins fromdune-project
there / removedriver
pin, as it's no longer necessary / pin opam-repository insidedune-workspace
)Dockerfile
.opam
filesMakefile
s to reflect using only Dune, and no longer usingopam
CONTRIBUTING.md
andHACKING.md
playground/
to build with Dune Developer Previewdune pkg
needs to be initialised. Rationale: this is needed to measure first build time without using docker~/.cache/ocamlorg
isdune pkg
also using~/.cache/dune
. What's the impact of deleting it?dune.lock
directory is enough to go back to the old build -- relevant only for open PRs / current work in progress, we should encourage people to rebase on main and use Dune package management after the merge of this patchTo test this
Current status
I believe this is ready to merge.