-
Notifications
You must be signed in to change notification settings - Fork 430
Add support for x-maintenance-intent in dune-project #11274
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
Conversation
8612074
to
a3a9074
Compare
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 for the contribution! 👍
You need to update the tests to reflect your changes. I would split this in two PRs: the part that updates the code and the part that updates Dune itself (in case it breaks for the former).
Also, could you add an entry into the changes directory?
2917028
to
6314d7b
Compare
6314d7b
to
d1a4775
Compare
Thanks for the quick review! I've added tests, an entry in |
d1a4775
to
7b81397
Compare
test/blackbox-tests/test-cases/dune-project-meta/basic-generate.t
Outdated
Show resolved
Hide resolved
7b81397
to
be0a9b6
Compare
That sounds like a useful thing to do. Is there a need to validate this field at all? |
34d5210
to
5353bb8
Compare
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.
Nice feature, however due to how we handle compatibility an upgrade from Dune 3.17 to 3.18 should not trigger user visible changes (that is only allowed via dune-lang), so my suggestion is to gate the generation to dune-lang 3.18+. That will also remove lots of test changes, since these wouldn't change anymore.
test/blackbox-tests/test-cases/dune-project-meta/basic-generate.t
Outdated
Show resolved
Hide resolved
This is perhaps not the right place to comment, because this is more about opam rather than dune, but if you generate Personally I'd like to see a much more explicitly elaborated story on how the How are the intent values to be interpreted by users and by opam-repository maintainers? How does it affect archiving? What happens if the maintainers just vanish? What sort of automation is planned based on that field? ... Like discussed in this PR, I would expect most people to just add Seriously, If I (and most other package authors) just set the field as ”latest” and be done with it, then how does that help? I'm sceptical that a field like this really helps. Intent is subjective. Actual behaviour is objective. People are optimistic and tend to lie about their own intentions. The expectation seems to be that package authors are good citizen and will carefully maintain the To me this If the idea is to use Or perhaps something like FYI: Much of the above is duplicated from what I posted on OCaml Discord on the opam-repository channel, before noticing this PR. |
Thanks, @polytypic, for reporting this!
Unfortunately, considering Dune is only implementing opam policies, this is not the right place to talk about it. My suggestion is to raise the issue on discuss.ocaml.org to make the discussion more public (Discord requires an account to be accessed). I would suggest this thread as it is the last one about the maintenance. Regarding your concerns, you raised interesting questions, and it is worth discussing them in the right place 😸 |
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.
Nice. I have one small concern about the changes in basic-generate.t
where I don't think the changes are required and a small documentation improvement but otherwise this looks very good.
|
||
.. versionadded:: 3.18 | ||
|
||
The same as (and takes precedences over) the corresponding global field. |
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 you add a link to the global field? The global field has the links to the documentation which is nice, so being able to go there directly without having to search for it would be useful.
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 any way to link to an anchor with rst?
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.
@Leonidas-from-XIV for uniformity, I don't think this is what we want. If you look above, there is no link to global.
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.
@art-w No, I don't think so, at least not easily. I think the other file would need to define a named reference at the location where the field is defined and we would need to link to that. Or use a plugin which automatically creates these anchors. If you find an easier way, I'd love to know about it, it does feel like an odd omission in reST/RTD.
@maiste I understand your uniformity argument but I think it should not prevent us from making the documentation easier to navigate. I think it is always better to add a link (and we could do that for other fields that allow overriding as well, or say "all the fields are overrides of the global, link here", though that's not really true as you can't have global depends
)
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, my argument makes more sense regarding the code than the documentation. I'm fine with both in the end 😄
test/blackbox-tests/test-cases/dune-project-meta/basic-generate.t
Outdated
Show resolved
Hide resolved
846804a
to
9d2ee9d
Compare
14abe7e
to
2ce02f9
Compare
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 removed the unrelated test changes and rebased. Seems to work fine and the CI failure is unrelated and worked-around by #11293. I think it is fine to merge this before that one is merged.
2ce02f9
to
6a7d745
Compare
Signed-off-by: ArthurW <[email protected]> Signed-off-by: Marek Kubica <[email protected]>
Signed-off-by: ArthurW <[email protected]> Signed-off-by: Marek Kubica <[email protected]>
Signed-off-by: ArthurW <[email protected]> Signed-off-by: Marek Kubica <[email protected]>
Signed-off-by: ArthurW <[email protected]> Signed-off-by: Marek Kubica <[email protected]>
Signed-off-by: ArthurW <[email protected]> Signed-off-by: Marek Kubica <[email protected]>
6a7d745
to
263bf0b
Compare
Thanks @Leonidas-from-XIV ! @rgrinberg > Regarding validation, could this be addressed in a follow-up PR if the lack of validation turns out to be an issue? Otherwise I'm happy to add the validation now. |
I think we should add validation as soon as possible as doing it post the release would mean the validation would have to be guarded behind the new version. @Leonidas-from-XIV could you revert this if validation isn't ready before we release 3.18 |
Sure, will keep that in mind and make sure not to release 3.18 without 👍 (or revert before). |
* Add support for opam x-maintenance-intent in dune-project Signed-off-by: ArthurW <[email protected]> Signed-off-by: Marek Kubica <[email protected]> * Add tests for maintenance_intent Signed-off-by: ArthurW <[email protected]> Signed-off-by: Marek Kubica <[email protected]> * Document maintenance_intent Signed-off-by: ArthurW <[email protected]> Signed-off-by: Marek Kubica <[email protected]> * Default maintenance_intent to (latest) Signed-off-by: ArthurW <[email protected]> Signed-off-by: Marek Kubica <[email protected]> * No x-maintenance-intent in opam files for dune<3.18 Signed-off-by: ArthurW <[email protected]> Signed-off-by: Marek Kubica <[email protected]> --------- Signed-off-by: ArthurW <[email protected]> Signed-off-by: Marek Kubica <[email protected]>
* Add support for opam x-maintenance-intent in dune-project Signed-off-by: ArthurW <[email protected]> Signed-off-by: Marek Kubica <[email protected]> * Add tests for maintenance_intent Signed-off-by: ArthurW <[email protected]> Signed-off-by: Marek Kubica <[email protected]> * Document maintenance_intent Signed-off-by: ArthurW <[email protected]> Signed-off-by: Marek Kubica <[email protected]> * Default maintenance_intent to (latest) Signed-off-by: ArthurW <[email protected]> Signed-off-by: Marek Kubica <[email protected]> * No x-maintenance-intent in opam files for dune<3.18 Signed-off-by: ArthurW <[email protected]> Signed-off-by: Marek Kubica <[email protected]> --------- Signed-off-by: ArthurW <[email protected]> Signed-off-by: Marek Kubica <[email protected]> Signed-off-by: Chris Armstrong <[email protected]>
CHANGES: ### Fixed - Support HaikuOS: don't call `execve` since it's not allowed if other pthreads have been created. The fact that Haiku can't call `execve` from other threads than the principal thread of a process (a team in haiku jargon), is a discrepancy to POSIX and hence there is a [bug about it](https://dev.haiku-os.org/ticket/18665). (@Sylvain78, ocaml/dune#10953) - Fix flag ordering in generated Merlin configurations (ocaml/dune#11503, @voodoos, fixes ocaml/merlin#1900, reported by @vouillon) ### Added - Add `(format-dune-file <src> <dst>)` action. It provides a replacement to `dune format-dune-file` command. (ocaml/dune#11166, @nojb) - Allow the `--prefix` flag when configuring dune with `ocaml configure.ml`. This allows to set the prefix just like `$ dune install --prefix`. (ocaml/dune#11172, @rgrinberg) - Allow arguments starting with `+` in preprocessing definitions (starting with `(lang dune 3.18)`). (@amonteiro, ocaml/dune#11234) - Support for opam `(maintenance_intent ...)` in dune-project (ocaml/dune#11274, @art-w) - Validate opam `maintenance_intent` (ocaml/dune#11308, @art-w) - Support `not` in package dependencies constraints (ocaml/dune#11404, @art-w, reported by @hannesm) ### Changed - Warn when failing to discover root due to reads failing. The previous behavior was to abort. (@KoviRobi, ocaml/dune#11173) - Use shorter path for inline-tests artifacts. (@hhugo, ocaml/dune#11307) - Allow dash in `dune init` project name (ocaml/dune#11402, @art-w, reported by @saroupille) - On Windows, under heavy load, file delete operations can sometimes fail due to AV programs, etc. Guard against it by retrying the operation up to 30x with a 1s waiting gap (ocaml/dune#11437, fixes ocaml/dune#11425, @MSoegtropIMC) - Cache: we now only store the executable permission bit for files (ocaml/dune#11541, fixes ocaml/dune#11533, @ElectreAAS) - Display negative error codes on Windows in hex which is the more customary way to display `NTSTATUS` codes (ocaml/dune#11504, @MisterDA)
CHANGES: ### Fixed - Support HaikuOS: don't call `execve` since it's not allowed if other pthreads have been created. The fact that Haiku can't call `execve` from other threads than the principal thread of a process (a team in haiku jargon), is a discrepancy to POSIX and hence there is a [bug about it](https://dev.haiku-os.org/ticket/18665). (@Sylvain78, ocaml/dune#10953) - Fix flag ordering in generated Merlin configurations (ocaml/dune#11503, @voodoos, fixes ocaml/merlin#1900, reported by @vouillon) ### Added - Add `(format-dune-file <src> <dst>)` action. It provides a replacement to `dune format-dune-file` command. (ocaml/dune#11166, @nojb) - Allow the `--prefix` flag when configuring dune with `ocaml configure.ml`. This allows to set the prefix just like `$ dune install --prefix`. (ocaml/dune#11172, @rgrinberg) - Allow arguments starting with `+` in preprocessing definitions (starting with `(lang dune 3.18)`). (@amonteiro, ocaml/dune#11234) - Support for opam `(maintenance_intent ...)` in dune-project (ocaml/dune#11274, @art-w) - Validate opam `maintenance_intent` (ocaml/dune#11308, @art-w) - Support `not` in package dependencies constraints (ocaml/dune#11404, @art-w, reported by @hannesm) ### Changed - Warn when failing to discover root due to reads failing. The previous behavior was to abort. (@KoviRobi, ocaml/dune#11173) - Use shorter path for inline-tests artifacts. (@hhugo, ocaml/dune#11307) - Allow dash in `dune init` project name (ocaml/dune#11402, @art-w, reported by @saroupille) - On Windows, under heavy load, file delete operations can sometimes fail due to AV programs, etc. Guard against it by retrying the operation up to 30x with a 1s waiting gap (ocaml/dune#11437, fixes ocaml/dune#11425, @MSoegtropIMC) - Cache: we now only store the executable permission bit for files (ocaml/dune#11541, fixes ocaml/dune#11533, @ElectreAAS) - Display negative error codes on Windows in hex which is the more customary way to display `NTSTATUS` codes (ocaml/dune#11504, @MisterDA)
With progress being made on the opam packages archiving policy, a lot of published packages are going to add an
x-maintenance-intent: ["..."]
to their opam files. When using dune to generate the opam files, adding this new field requires an extra.opam.template
file. Right now the custom opam templates escape hatch are fairly exceptional (~60 published projects uses them), so it would be nice for all the others to stay on the happy path if possible.In this PR, the opam
x-maintenance-intent: [...]
is only generated if the user specified(maintenance_intent ...)
in their dune-project. Perhaps it should produce["(latest)"]
by default?See #11272 (cc @hannesm)