Add cli option to turn on/off package management #13458
Conversation
| $ dune pkg enabled --ignore-lock-dir | ||
| ^^^ This is a bug. It should be disabled. | ||
| We have a lock directory, but it is ignored. | ||
| It should behave the same as if there was no lockdir |
There was a problem hiding this comment.
The bug is with the detection mechanism of the dune pkg enabled command, but not the actual detection mechanism used in the important parts of the engine
There was a problem hiding this comment.
Can you create an issue about this?
|
Glad to see this coming into shape! Where can I find the analysis and design justification/explanation of how the different options interact or override each other? |
|
@shonfeder The test checks this extensively. |
|
Could we have a richer state instead of enabled/disabled? for instance:
|
Yes I saw the test. I am asking after the rationale and specification of the behavior. What is the general rule we are applying that should be applied in all similar cases? How will it interact with environment variables for the same functionality (which is also called for in the issue)? Tests of a single use case aren't sufficient to specify the rule and explain the rationale (tho a spec that is also tested is cool when that works out). |
I think we can safely add extensions to the value accepted here in follow-ups, without expanding the scope of this PR, so long as we are not committing to anything that limits those extensions. We are aware of your RFC drafts that touch on this flag, btw, and not ignoring them :) |
|
Personally I think the solution to #12819 should be a tool that edits This would also allow for extension on what can be configured (akin to
|
|
We already do this for enabling/disabling the cache. This PR is reusing the logic introduced there. This is not editing dune-workspace but rather overriding it in a precedented way. The title of this PR is perhaps inaccurate since the goal is not to enable/disable package management, but rather to provide a CLI analogue to the existing
This PR is filling this gap and nothing more. Regarding some other comments:
This is possible with
This is the default behaviour.
This can be configured by setting the platforms you want to solve for to one. Not the most ergonomic, but we would need to think a bit harder on a nice way to do this otherwise. If we don't have an issue open about this we should open one.
This is Regarding the precedence of the CLI vs workspace vs env vars vs config. I think this is something that should be thought about, but I would argue that it doesn't belong in this PR. This PR is following the precedent set by options such as The first precedent pair is inherited from cmdliner. We can of course change this, but it can be argued that it is the most expected. If you disagree, great, but let's instead discuss that in the relevant issue. |
Ok, then maybe I should ask: why? What is the use case that this is solving? I think adding more configuration flags just because it is possible makes the UX worse as it
I am worried about adding an option to Dune that we will have to discourage people from using. |
|
|
||
| let dash_p = | ||
| let dash_p_implies = [ "--release"; "--ignore-lock-dir"; "--only-packages" ] in | ||
| let dash_p_implies = [ "--release"; "--pkg=disabled"; "--only-packages" ] in |
There was a problem hiding this comment.
I believe this change is answering @Leonidas-from-XIV 's question regarding the need: --ignore-lock-dir is incorrect for the purpose of an Opam installation, since it shouldn't disable pkg if you have it enabled in your workspace.
On the other end, the explicit --pkg=enabled is very useful for CI! (for progressive adoption)
As mentioned by @ElectreAAS , it should be possible to refactor the flag --ignore-lock-dir to mean --pkg=always i.e. "enabled-but-ignore-existing-lock-dir", but it'll be easier to review this in a follow-up PR, separate from the introduction of the new --pkg flag feature. I think this will make the semantics clearer and open the way for more options for that flag (e.g. non portable lockfiles).
There was a problem hiding this comment.
Well fair enough. In such case I would actually suggest removing --ignore-lock-dir since its only purpose was to not accidentally trigger package management when a package that comes with a lock dir is distributed via OPAM. With --pkg=disabled supplanting that, I think it is completely obsolete (one could configure --pkg=enabled --ignore-lock-dir but it seems like a weird roundabout way to require autolocking and not a reasonable configuration option - if we wanted that then something like a hypothetical --pkg=autolock would be a much better way to express such a configuration).
This defeats the purpose of the issue, which is to make it possible to bypass (or enable) package management functionality without having to edit the files on disk (and so make the repo dirty, etc.) This is still declarative, but the declaration happens on the CLI. |
To be clear, I was not asking for it to be added in this PR, I was asking where it is documented/specified. :D |
art-w
left a comment
art-w
left a comment
There was a problem hiding this comment.
I think this PR is good and should be merged! It introduces a useful feature which both addresses the CI needs and provides a cleaner setting to disable pkg for opam installations.
However there's some follow-up refactoring work to do to clean up the interaction with --ignore-lock-dir described in the issue #13468, which would then enable the other improvements requested by everyone :)
|
I added in the PR description a link to a comment specifying the priority order. |
|
As this is a user-visible change, this needs a changelog entry (and probably an update in the docs too) |
|
I opened the follow-up #13472 that may better answer your concerns @Leonidas-from-XIV @samoht |
Leonidas-from-XIV
left a comment
Leonidas-from-XIV
left a comment
There was a problem hiding this comment.
I think the code is fine overall, my main concern would be to straight up remove --ignore-lock-dirs directly but that's in #13468.
ElectreAAS
force-pushed
the
push-xksznwmmqnkm
branch
2 times, most recently
from
caba848 to
883291d
Compare
|
I added a doc entry for the @Leonidas-from-XIV you added the field in the first place, but there was no doc entry. Was this an oversight or a deliberate choice? |
|
@ElectreAAS We don't write package management stuff in the changelog, so it was a deliberate choice to let it out. |
Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>
ElectreAAS
force-pushed
the
push-xksznwmmqnkm
branch
from
883291d to
287fa42
Compare
right, I somehow forgot about that. I removed it, but the doc entry is still there |
6 participants
Fixes #12819, see the motivation section of the issue.
Added
I added a
--pkgoption in the common section, taking eitherenabledordisabledas arguments. It does what it says, the behaviour is shared with thepkgfield in workspace files (note that the docs don't mention that field yet).The CLI option has priority over the workspace configuration. See added test for details.
Implementation notes
Interaction with
--ignore-lock-dirIt is rather simple:
pkg = enabledmeans autolocking is activated.pkg = disabledmeans no lock directory is ever considered anyway.I still think these two options are too related to be separated, and should be merged eventually, but that isn't included in this PR.
Overlap with the existing
pkgoptionThe
dune initcommand option has apkgoption which relates to either opam or esy, nothing related to what I'm doing here. However the name can't be used for two different options (thanks Cmdliner), so I added a booleanallow_pkg_flagto sort that out. Not the cleanest option, advice appreciated.Priority calculation
I found this comment in workspace.mli which clearly states priority order, worth a quick look
Documentation changes
I added an entry for the
pkgfield in workspace files which was missing, alluding to the CLI option. Rendered output visible here.cc @Alizter @art-w as they helped me with this <3