ocaml / dune

A composable build system for OCaml.
https://dune.build/
MIT License
1.6k stars 396 forks source link

`dune pkg lock -x <context>` should lock the associated lock dir #10910

Closed maiste closed 4 days ago

maiste commented 5 days ago

With Dune package management, when you are trying to lock a project with a specified context, it locks the default context instead of locking the lock directory associated with the context (if it exists). The following example should put the locked files to foo.lock but instead, it puts them in dune.lock when running dune pkg lock -x foo:

 (lock_dir
  (path foo.lock)
  (repositories mock))
 (context
  (default
   (name foo)
   (lock_dir foo.lock)))
rgrinberg commented 5 days ago

The only bug as far as I can tell is that we accept the -x parameter for dune pkg lock. dune pkg lock should be completely oblivious to contexts.

maiste commented 5 days ago

I see! I'm still familiarizing with build context in Dune. Sorry for the noise...

The issue here is that it comes with the Common.t term argument. Would there be a way to remove this argument?

rgrinberg commented 5 days ago

The name Common.t is a little too optimistic. It assumes that all of its flags are going to be useful to all commands. That's certainly not the case, as most of the flags from Common.t are in fact useless for dune pkg. This -x flag, for example, is only useful for build and install commands. So it should really not exist in common.

I don't really have concrete suggestions other than considering every flag in Common.t on a case by case basis and then making a decision on whether it really is "common".

maiste commented 5 days ago

OK, I understand! It is more a question of legacy here, then. It would need another issue to discuss such a redesign, if required :sweat_smile: From offline discussion, suggestions are to improve the content of the help message from the CLI and to add more information in the cross compilation page to explain the meaning of this flag.

On a second hand, the behavior that triggers this issue was that you can define a (lock_dir name.lock) stanza in the (context ...) stanza. From offline discussion, I learned it was made this way so you can define a lock directory "per system" (x86_64, Linux, Windows, etc) as they are not stable per system. Considering this stanza, it would make sens to have a way to build a lock dir associated with a context (for example, a --context flag). I know that you said context are oblivious of lock dir, but this stanza kind of breaks this rule :confused:

rgrinberg commented 5 days ago

Considering this stanza, it would make sens to have a way to build a lock dir associated with a context (for example, a --context flag). I know that you said context are oblivious of lock dir, but this stanza kind of breaks this rule 😕

You could indeed create such a flag. I don't think it would be very useful though. For a couple of reasons:

  1. It's more natural to refer to lock directories by their file path, and we already support that.
  2. A lock directory can belong to more than one context. So a --context flag would have the potential to update the lock directory for more than one context. Not really desirable IMO.
maiste commented 4 days ago

Indeed, using the path makes sense here. I'm closing this issue and the PR associated consequently.