Skip to content
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

feature(pkg): lock file configuration in workspace #7835

Merged

Conversation

rgrinberg
Copy link
Member

Signed-off-by: Rudi Grinberg me@rgrinberg.com

type t = Common.t
type t =
{ base : Common.t
; lock : Path.Source.t option
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be simpler to drop the option and default this field to Dune_pkg.Lock_dir.path during parsing if no value is specified in the workspace? Is it ever desirable for dune to distinguish between the case where lock is omitted and the case where it's explicitly set to the default path?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it ever desirable for dune to distinguish between the case where lock is omitted and the case where it's explicitly set to the default path?

There's a couple of situations where the difference is useful:

  1. A lock file that hasn't been explicitly specified and is absent is not an error. While a lock file that has been explicitly specified and is absent is an error.
  2. When the types reflect exactly what the user has written, you can use them for serialization.

The 2nd point is a bit more general, so I tend to writing things in this style. However, 1. is the important one here.

@gridbugs
Copy link
Collaborator

Thinking about how this interacts with lockdir generation. Since lockdirs are associated with contexts, would it make sense to have dune pkg lock take a context name so it knows where to put the generated lockdir rather than letting the user specify the lockdir path directly?

@rgrinberg
Copy link
Member Author

Thinking about how this interacts with lockdir generation. Since lockdirs are associated with contexts, would it make sense to have dune pkg lock take a context name so it knows where to put the generated lockdir rather than letting the user specify the lockdir path directly?

I'm thinking that our current scheme works alright. There's no special location to use for a lockdir for a particular context. They all read from the default location unless explicitly specified.

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

<!-- ps-id: 4098e148-2708-484c-a5a2-7b5c3f3e1994 -->
@rgrinberg rgrinberg force-pushed the ps/rr/feature_pkg___lock_file_configuration_in_workspace branch from 2244e79 to 94af974 Compare May 31, 2023 14:31
@gridbugs
Copy link
Collaborator

gridbugs commented Jun 1, 2023

Thinking about how this interacts with lockdir generation. Since lockdirs are associated with contexts, would it make sense to have dune pkg lock take a context name so it knows where to put the generated lockdir rather than letting the user specify the lockdir path directly?

I'm thinking that our current scheme works alright. There's no special location to use for a lockdir for a particular context. They all read from the default location unless explicitly specified.

Just clarifying that our current scheme is to let the user pass a lockdir path to dune pkg lock, and it will assume the default (dune.lock) if no lockdir is specified.

I think we do need to change dune pkg lock to be aware of the lockdirs associated with a context though. E.g. if there is only one context in the workspace file and it uses a non-default lockdir location, running dune pkg lock with no arguments should create the lockdir at the path specified in the context instead of the default location. Also my understanding is that dune build will build its targets in all contexts so maybe it makes sense to add a flag to dune pkg lock to cause it to (re)generate lockdirs for all contexts to match.

@rgrinberg
Copy link
Member Author

E.g. if there is only one context in the workspace file and it uses a non-default lockdir location, running dune pkg lock with no arguments should create the lockdir at the path specified in the context instead of the default location

This behavior sounds like it's trying to guess the user's intent a little too much. I'm not opposed though. I don't yet have any concrete use cases for multi context/multi lockdir builds in mind.

Also my understanding is that dune build will build its targets in all contexts so maybe it makes sense to add a flag to dune pkg lock to cause it to (re)generate lockdirs for all contexts to match.

Not all commands are like that. For example, dune describe will use the default context unless other specified.

@gridbugs
Copy link
Collaborator

gridbugs commented Jun 2, 2023

E.g. if there is only one context in the workspace file and it uses a non-default lockdir location, running dune pkg lock with no arguments should create the lockdir at the path specified in the context instead of the default location

This behavior sounds like it's trying to guess the user's intent a little too much. I'm not opposed though. I don't yet have any concrete use cases for multi context/multi lockdir builds in mind.

Fair enough. Happy to leave it as is for now and refine the UX when we have a clearer idea of how this will be used in practice.

@rgrinberg
Copy link
Member Author

Okay so let's add dune pkg lock --all to the TODO list. Eventually we can talk about making that default.

@rgrinberg rgrinberg merged commit bd5aac2 into main Jun 2, 2023
40 of 41 checks passed
@rgrinberg rgrinberg deleted the ps/rr/feature_pkg___lock_file_configuration_in_workspace branch June 2, 2023 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants