-
Notifications
You must be signed in to change notification settings - Fork 392
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
Stop treating toolchains as packages in pkg rules #10534
Stop treating toolchains as packages in pkg rules #10534
Conversation
Previously toolchains were treated as special packages by the package rules logic. This change removes toolchains from the package dependency graph used by package rules (but not from any other part of dune such as the solver or lockfiles). This separation lets us remove a bunch of special treatment for packages that contain toolchains. Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
a061aa4
to
97f0934
Compare
@Leonidas-from-XIV can you take a second look at this please |
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 fairly good with the new generalized lock.
| Error e -> Fiber.return @@ Error e | ||
| Ok `Success -> Fiber.return (Ok `Success) | ||
| Ok `Failure -> | ||
let sleep_duration_s = 0.1 in |
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 could possibly be pulled out of the function. I don't know, even in the original code it is an admittedly ugly bit.
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've split this into two mutually recursive closures to avoid needing to pass as much info from one recursive call to the next.
src/dune_pkg/flock.ml
Outdated
let fd = | ||
Unix.openfile | ||
(Path.to_string lock_path) | ||
[ Unix.O_CREAT; O_WRONLY; O_SHARE_DELETE; Unix.O_CLOEXEC ] |
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.
That second Unix
there can be removed.
Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
97f0934
to
020b2ec
Compare
Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
This prevents multiple concurrent instances of dune interfering with one another while installing toolchains. Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
5712586
to
c740f6a
Compare
This grew out of the work I'm doing to read toolchain information from the opam repo rather than hardcoding it (https://github.com/tarides/team-build-system/issues/11), but the main benefit of this change is that it removes a bunch of special cases I needed to add so that packages containing toolchains were ignored by most of the package rules.
It's also going to make it easier to thread the extra information needed to represent the
ocaml-variants
toolchain to the parts of the code where it's needed in a later change.