pkg: allow pkg for release builds

[gridbugs]

Fixes #11375

@rgrinberg do you remember why "--ignore-lock-dir" was added to the arguments passed by "--release"? Is it now safe to remove?

[Leonidas-from-XIV]

Shouldn't --ignore-lock-dir be added to the options of -p instead? Since we want to avoid OPAM triggering dune pkg.

[rgrinberg]

-p triggers --release. So yes, this is the reason. Opam release builds should ignore lock directories

[gridbugs]

@rgrinberg would there be any harm in moving --ignore-lock-dir to -p. People coming from rust will probably run dune build --release under the (false) belief that it will build their project with more optimization and less debugging, as that's what a "release profile" means in that ecosystem. It will reduce adoption friction if --release doesn't ignore the lockdir.

[rgrinberg]

Grepping through the opam repository shows me there's a handful of packages that use --release instead of -p. If you'd like to devise a plan to switch them, then sure I don't object.

[gridbugs]

Looks like there's only 5 or so packages whose latest version uses --release in their opam file. I've checked and each package has its opam file checked in to VCS. None of these packages have lockdirs in any of their existing source archives, so we just need to make sure that new releases of these packages change to -p, at least if they start checking in their lockdir. I think we should announce that using --release in opam files is deprecated, make the change to dune moving --ignore-lock-dir to -p, and submit PRs to update the opam files checked into the current projects.

[Leonidas-from-XIV]

We could already switch the existing releases to -p via a PR to opam-repository, unless there is something specific that prevents them from using --release in favor of -p. This needs to be done anyway, otherwise a new Dune version that enables package mgmt in --release would fail to build the old packages (or add an upper bound on Dune for these packages, but this is generally something we should try to avoid).

[rgrinberg]

Looks like there's only 5 or so packages whose latest version uses --release in their opam file. I've checked and each package has its opam file checked in to VCS. None of these packages have lockdirs in any of their existing source archives, so we just need to make sure that new releases of these packages change to -p, at least if they start checking in their lockdir. I think we should announce that using --release in opam files is deprecated, make the change to dune moving --ignore-lock-dir to -p, and submit PRs to update the opam files checked into the current projects.

Given the small number of packages in question, I think you should add a step to create an issue in the respective bug trackers telling people they need to switch.

You could also add a new flag (--release-without-lockdir?) that has the old behavior for ease of transition.

[gridbugs]

I think you should add a step to create an issue in the respective bug trackers

I was thinking I'd just send a PR that fixes the issue directly, though raising an issue would be less work.

You could also add a new flag (--release-without-lockdir?) that has the old behavior for ease of transition.

Why not just let people use --release --ignore-lockdir? It would presumably have the same effect and not require adding an additional flag. The issue with either of these solutions is it will require projects to update their lower bound for dune to 3.18 (assuming that's the first version where --release does not imply --ignore-lockdir).

[Leonidas-from-XIV]

Sounds like a good solution. So if I understand the conclusion correctly we would do the following:

1. We will move --ignore-lock-dir from --release to -p
2. We will create some PRs for upstream/opam-repository to migrate from --release to -p
3. Update the failing tests

I think this is quite nice, because in opam-health-check we needed to run --profile=release instead of --release because otherwise the lock file would be ignored and while a possible workaround it's also just a surprising and unintuitive UX, so this PR is a solid improvement in my eyes.

@gridbugs Do you need help with these? If you want I can handle the projects that use --release and submit the fixes.

[gridbugs]

Thanks @Leonidas-from-XIV . I'll make PRs to upstream this change into individual projects today and link them here.

[bobot]

Sorry, for clarification are you removing --ignore-lock-dir ?

We have multiple packages in a repository that are installed with a unique opam file. We can revisit this choice, (opam pin remove makes it now easier to remove all the pins of a whole repository), or adding the -p of all the packages.

(Coming from frama-c)

[gridbugs]

Upstreaming PRs:

• frama-c
• opsian
• llvm
• aws-config

tezos-rust-libs is deprecated so I ignored it.

[gridbugs]

Sorry, for clarification are you removing --ignore-lock-dir ?

No we're not removing --ignore-lock-dir, just the behaviour --release so that it no longer implies --ignore-lock-dir.

[gridbugs]

Regarding frama-c (cc @bobot) it sounds like in the ideal world that project would be able to pass --release --ignore-lock-dir, however dune does not currently allow both of those flags to be passed at the same time, because the former implies the latter, and the way dune treats flags means that this is like passing --ignore-lock-dir twice, which is not allowed. After we change dune so that --release no longer implies --ignore-lock-dir then both flags can be passed at the same time, but of course old versions of dune will still have the old behaviour.

[maiste]

What should we do with this PR? Should we ignore it for 3.18?
I'm wondering if there is any blocker preventing us from merging, as @Leonidas-from-XIV already opens the PR on opam to fix this, and it seems straight forward to add the --ignore-lock-dir to get back the old behavior.

I'm wondering if we can't specify the constraint with opam like this:

build : [
  [ "dune" "build" "--release"] {dune:version < 3.18}
  [ "dune" "build" "--release" "--ignore-lock-dir"] {dune:version >= 3.18}
]

According to the opam manual:

foo:bar refers to bar in the installed version of foo, and may be undefined if foo is not installed: for example, a package depending on foo can use foo:version to toggle different compatibility modes depending on the installed version of foo.

WDTY?

[Leonidas-from-XIV]

I'm fairly certain you can attach the filter just to the argument you want filter (e.g. how @runtest is usually conditional on with-test)

build: [
  "dune"
  "build" 
  "--release"
  "--ignore-lock-dir" {dune:version >= 3.18}
]

I personally think we should just do the change in 3.18 and fix up packages in opam-repository where necessary (which it mostly isn't since I assume no package come with lockdirs anyway)