Skip to content

Conversation

@gridbugs
Copy link
Collaborator

Dune's lock directories are currently not portable. Naming them "dune.lock" might suggest to users of other packgae managers that it is safe to check them into version control and share them between developers potentially using different machines. To discourage this behaviour, lockdirs are renamed from "dune.lock" to ".dune-solution-cache". The fact that this directory begins with a period and contains the word "cache" should hopefully discourage users from checking them into version control.

For the same reason, the collection of dev tool lockdirs is renamed from "dev-tools.locks" to ".dune-tools-solution-cache".

In the future it's likely we will enable portable lock directories by default, at which point they will become safe to check into version control. At this point we can change the default lock directory name back to "dune.lock" and encourage checking them into version control.

@gridbugs gridbugs force-pushed the hide-lockdirs branch 4 times, most recently from 428f1a2 to 934a26a Compare October 29, 2025 06:48
@gridbugs gridbugs marked this pull request as draft October 29, 2025 06:52
Dune's lock directories are currently not portable. Naming them
"dune.lock" might suggest to users of other packgae managers that it is
safe to check them into version control and share them between
developers potentially using different machines. To discourage this
behaviour, lockdirs are renamed from "dune.lock" to
".dune-solution-cache". The fact that this directory begins with a
period and contains the word "cache" should hopefully discourage users
from checking them into version control.

For the same reason, the collection of dev tool lockdirs is renamed from
"dev-tools.locks" to ".dune-tools-solution-cache".

In the future it's likely we will enable portable lock directories by
default, at which point they will become safe to check into version
control. At this point we can change the default lock directory name
back to "dune.lock" and encourage checking them into version control.

Signed-off-by: Stephen Sherratt <[email protected]>
@gridbugs
Copy link
Collaborator Author

This currently doesn't work because dune ignores directories beginning with a period. It works if you add (dirs :standard .dune-solution-cache) to a dune file in the top level of projects, but obviously that's not the right solution. Instead we should prevent dune from ignoring directories named .dune-solution-cache.

@rgrinberg
Copy link
Member

rgrinberg commented Oct 29, 2025

Aren't we going to encourage users to check in the portable lock dirs? Morever, we already have a workflow where the lock directory doesn't even need to exist in the source directory at all and can be generated on the fly. Isn't that enough for everybody who doesn't want to commit these?

@Leonidas-from-XIV
Copy link
Collaborator

This currently doesn't work because dune ignores directories beginning with a period. It works if you add (dirs :standard .dune-solution-cache) to a dune file in the top level of projects, but obviously that's not the right solution. Instead we should prevent dune from ignoring directories named .dune-solution-cache.

Would it be feasible to just extend :standard with .dune-solution-cache? Generally the issue is that the user is free to define any lock dir in dune-workspace so we should probably auto-opt-in every of these paths into being tracked.

Morever, we already have a workflow where the lock directory doesn't even need to exist in the source directory at all and can be generated on the fly. Isn't that enough for everybody who doesn't want to commit these?

#11775 has stalled because of issues with the current way dev-tools are implemented and it brings a lot of changes that haven't been tested. So if we want to publish the MSP reasonably soon it might need to descoped from the MSP. I would really like to see it succeed but if we need to re-implement dev-tools before landing #11775 that might just take too long.

In any case, renaming .dune.lock is not blocking work from #11775 to continue. It enables us to publish the MSP and consider the lock dirs an implementation detail for now, and mostly going away after #11775 is merged.

@rgrinberg
Copy link
Member

I don't think renaming this directory is blocking your MSP anyhow.

The lock directory is not just an implementation detail and should be committed when appropriate. Otherwise, why do we have portable lock directories at all?

Copy link
Collaborator

@Leonidas-from-XIV Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of changes are unfortunately rather verbose but necessary. I've added some comments on where we can avoid changes and make the text a bit less specific.

val debug_package_logs : bool ref

(** Whether we are ignoring "dune.lock/". *)
(** Whether we are ignoring ".dune-solution-cache/". *)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe update this comment to not mention the value. Especially since it doesn't just ignore dune.lock, it ignores every lock directory.

@@ -1,4 +1,4 @@
We test how opam files with depopts fields are translated into dune.lock files:
We test how opam files with depopts fields are translated into .dune-solution-cache files:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit misleading, as dune.lock is not a file.

Suggested change
We test how opam files with depopts fields are translated into .dune-solution-cache files:
We test how opam files with depopts fields are translated into lock directories:

Solution for .dune-solution-cache:
- foo.1.2

$ find ${default_lock_dir} | sort
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe adding something like | sed "s/^${default_lock_dir}/LOCK_DIR/" could be nice to avoid changing output could be a good idea here.

@@ -1,4 +1,4 @@
Test that shows what happens when dune.lock is ignored.
Test that shows what happens when .dune-solution-cache is ignored.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Test that shows what happens when .dune-solution-cache is ignored.
Test that shows what happens when the lock dir is ignored.

> EOF

Building test works when the dune.lock is visible to dune.
Building test works when the .dune-solution-cache is visible to dune.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Building test works when the .dune-solution-cache is visible to dune.
Building test works when the lock dir is visible to dune.

Specific lock files can be given as positional arguments.
$ outdated dune.lock
dune.lock is up to date.
$ outdated .dune-solution-cache
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use ${default_lock_dir} here?


Local file system
$ runtest "(copy \"$PWD/dummy\")" 2>&1 | sed "s#$(pwd)#PWD#" | sed '/ *^\^*$/d' | sed '\#^File ".*dune.lock/foo.pkg", line 2, characters#d'
$ runtest "(copy \"$PWD/dummy\")" 2>&1 | sed "s#$(pwd)#PWD#" | sed '/ *^\^*$/d' | sed '\#^File ".*.dune-solution-cache/foo.pkg", line 2, characters#d'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use ${default_lock_dir} here?

$ solve_project() {
> cat >dune-project
> dune pkg lock dune.lock
> dune pkg lock .dune-solution-cache
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
> dune pkg lock .dune-solution-cache
> dune pkg lock "${default_lock_dir}"

> (lang dune 3.8)
> (lock_dir
> (path dune.lock)
> (path .dune-solution-cache)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this might work:

Suggested change
> (path .dune-solution-cache)
> (path ${default_lock_dir})

total_number_of_transitive
in
let lock_dir_path = Stdune.Path.of_string "dune.lock" in
let lock_dir_path = Stdune.Path.of_string ".dune-solution-cache" in
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this could be read from somewhere.

But from what I see this doesn't really need to be changed, does it? It's just not the standard lock dir but the code presumably works with any path?

Copy link
Member

@rgrinberg rgrinberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I've outlined, this is not the way to go. It is merely a cosmetic change in the short term, and is certainly not the way to go in the long term.

@Leonidas-from-XIV
Copy link
Collaborator

The lock directory is not just an implementation detail and should be committed when appropriate. Otherwise, why do we have portable lock directories at all?

In the workflow of #11775 the lock dir is basically an implementation detail, the user does not have to create it nor even be aware of it. #12394 allows people to override the automatic locking with an explicit lock dir (which then should be committed!), but I don't think that that is going to be a default workflow for most users.

As for portable lock dirs, they're not enabled by default yet. There are also some open questions on what the right behavior should be. But generally they should probably be made the default and replace non-portable lock dirs.

@rgrinberg
Copy link
Member

With #11775, this PR becomes completely irrelevant as nothing is created in the source in the first place. So it's more of an argument to avoid this change.

but I don't think that that is going to be a default workflow for most users.

Between the two alternatives, it could certainly be argued that you're picking the better default. But the problem is that we don't have two alternatives yet. Once we have them, we could have these discussions.

As for portable lock dirs, they're not enabled by default yet. There are also some open questions on what the right behavior should be. But generally they should probably be made the default and replace non-portable lock dirs.

We do need to decide what "by default" is going to mean here. What are some of these open questions? I know there are issues with the implementation, but it's still a strict improvement over what we have for those that need this feature.

@Leonidas-from-XIV
Copy link
Collaborator

Leonidas-from-XIV commented Oct 29, 2025

(It's a bit straying of the path of this PR, because I think we should rather have this discussion in a PR about enabling portable lock dirs by default)

What are some of these open questions?

I think a big one is about locking on the platforms that are not in the default set. Say I am on FreeBSD, portable lock dirs will not generate a solution for my system, thus I can't build at all which is quite poor UX. Non-portable lock files work in such cases at the moment, generating a single non-portable FreeBSD solution.

However adding the current system by default to the solution isn't a panacea, as someone relocking the same repo on a different system will then accidentally remove the FreeBSD solution again.

Thus @gridbugs suggested printing an error saying "please add (code snippet here) to your dune-workspace to make this work" in such cases. Which is probably the most reasonable solution, but the user experience in such cases is rather mediocre.

@Alizter
Copy link
Collaborator

Alizter commented Oct 29, 2025

I'd suggest we only lock for the current platform and allow users to opt in and lock for others if they wish. If a mac user comes a across a linux lock file, they won't be able to run it. At which point they should relock, this would then lock both platoforms from then on.

@Leonidas-from-XIV
Copy link
Collaborator

That would require the lock step to read what platforms are locked already.

I don't think that's a particularly good design, because that makes the new lock dir dependent on previous lock dirs (hence @gridbugs' suggestion of opting in new platforms solutions via dune-workspace). Generally I think it should be possible to remove the lock dir and recreate it cleanly without breaking other platforms (e.g. when git merge-ing two lockdirs it can be better to just remove and relock instead of solving a potentially complex merge).

@shonfeder
Copy link
Member

shonfeder commented Nov 7, 2025

Thanks for the work on this, @gridbugs. I think it helped clarify positions of the devs and motivate the related fix in #12671

However, as the main motivating need should be addressed by #12653, I think like this PR can be closed. Is that right?

@gridbugs
Copy link
Collaborator Author

Yep, closing in favour of #12653.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants