-
Notifications
You must be signed in to change notification settings - Fork 455
Add doc about making brew package with dune pkg #12744
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
4df4a69 to
c706970
Compare
Leonidas-from-XIV
left a comment
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 guide is an interesting idea on how to leverage dune pkg for system packages. I haven't thought of this before but it is quite neat that you only need to depend on dune.
doc/howto/homebrew-package.rst
Outdated
| project must have a source archive hosted online somewhere (e.g. a gzipped | ||
| tarball on the project's github release page), and this archive must contain a | ||
| lockdir completely specifying the project's dependencies. Such a lockdir can be | ||
| generated by running ``dune pkg lock``. |
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.
But there are practically no packages that do this?
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.
Not yet, but hopefully once package management is released packages will start doing this.
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.
It seems weird to me to have a guide that's not applicable to anyone? Especially given that we're mostly pushing for autolocking, so even with stable dune package management I would expect most packages to not use a lock.
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.
What if we framed locking the project as part of the guide, rather than as a prerequisite for following the guide. I suppose the problem with that would be that all the existing released source archives still won't have a lockdir in them, but I think it's ok that this be a guide for people who want the next release of their project to be available on homebrew. I think it's risky to encourage people to make homebrew packages that solve dependencies as part of the installation process.
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.
The issue with locking as part of packaging is that the packager would need to effectively fork upstream and use a custom tarball which will most like raise eyebrows for homebrew maintainers, especially given the situation of compromised supply chains in FOSS…
I guess a rather large patch that adds the lock dir contents to the package could work however and be practical enough to work on most OCaml software in the wild.
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.
Ah yes I hadn't considered the use case where someone other than the project maintainer is adding the homebrew package, and the project doesn't have a checked-in lockdir. I intended this to be a guide for publishing one's own package on homebrew, not someone else's package.
As you correctly point out, it would require the author of the homebrew package to lock the project themselves, and have the homebrew package download a custom tarball which contains the lockdir, which does look suspicious.
I'll add an explanation for how to have homebrew solve dependencies at install time.
|
|
||
| depends_on "dune" => :build | ||
|
|
||
| def install |
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.
Adding
system "dune", "pkg", "lock"
here could make this work for nearly any OCaml package that uses Dune. To me this seems more practical as it does not require the tarball to ship with lock files that support the platform that the user is trying to package for.
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 think the problem with that approach is that builds won't be reproducible if the source archive doesn't include a lockdir. My preference would be for the guide to instruct how to release a homebrew package that never fails to build due to solver errors, and always builds the same artifacts on any given platform.
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.
Since reproducible builds are a goal for homebrew encouraging an approach which support reproducibility seems advisable?
However, since presumably homebrew dependencies can be updated between builds, I'm not sure how much this is goal is actually achieved in practice in homebrew.
If we had a flag like dune build --pkg-enabled or could set the envar with DUNE_CONFIG__PKG_ENABLED=true dune build, then we could have a single instruction set, that would work with a checked in lock dir if present, or fallback to a lockless dependency management otherwise, right?
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.
Reads pretty good to me! Awesome to have this documented!
819effa to
4aeb5d6
Compare
|
Based on @Leonidas-from-XIV's feedback I've updated the doc to suggest locking the dependencies at install time. |
doc/howto/homebrew-package.rst
Outdated
| depends_on "dune" => :build | ||
|
|
||
| def install | ||
| system "dune", "pkg", "lock" |
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.
Won't this force projects to update their locks, so even if you have a checked in lock dir it will force deps to be updated? That would be very bad behavior, IMO.
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.
If we cannot fix this in a clean way like #12744 (comment) (that would work correctly with either a lock checked or without a lock, without a conditional command), then I suggest:
- We plan to add a feature that will allow a clean command (adding an issue accordingly).
- For this PR, just note in the set up that having a checked in lock directory OR a workspace with
(pkg enabled)is currently a prerequisite for this tutorial.
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.
The doc says
Note the first install command:
dune pkg lock. This is only necessary if the source archive doesn't contain a lockdir...
But yes, also happy to say that for now projects either need a checked-in lockdir or (pkg enabled) in dune-workspace and remove dune pkg lock from the install commands.
shonfeder
left a comment
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.
Noting a change request, because I think forcing an unconditional relock in build recipes could have an untenable behavior.
Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
4aeb5d6 to
96240a4
Compare
|
After speaking with @shonfeder, I've changed the example formula to include |
No description provided.