Version dune-workspace and ~/.config/dune/config files
#932
Conversation
ghost
requested review from
src/main.ml
Outdated
| else | ||
| None) | ||
| let p = Path.of_string Workspace.filename in | ||
| if Path.exists p then |
There was a problem hiding this comment.
Unrelated to the pr but we should change this to use Option.some_if
|
One thing I'm wondering about is that if we should move the first line handling to the Version_file itself. Currently, the error message for a missing first line is: We should be able to do better if the EDIT: Or even more simply, we can just change the message to substitute |
| @@ -580,8 +579,10 @@ let installed_libraries = | |||
| let env = Main.setup_env ~capture_outputs:common.capture_outputs in | |||
| Scheduler.go ~log:(Log.create common) ~common | |||
| (Context.create | |||
| (Default { targets = [Native] | |||
| ; profile = Config.default_build_profile }) | |||
| (Default { loc = Loc.of_pos __POS__ | |||
There was a problem hiding this comment.
Given that this location will make it to the user, I wonder how useful it really is. Maybe a virtual location will be better?
| -> 'a | ||
| end | ||
|
|
||
| module Make(Data : sig type t end) = struct |
There was a problem hiding this comment.
Wondering what application you have in mind for this data field. It took me more time than I will admit that this argument isn't actually used anywhere. Would be simpler to just keep Make generative for now.
|
@diml I ended up adding a minor test suite for this feature because it was quite sparsely tested before. In the process I fixed a minor bug and did some touch ups. |
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
- dune requires the (lang ...) line - jbuilder accepts it but fallback to the jbuild syntax if it's not present Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
Added for testing but could come in handy elsewhere Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
|
Bleh, it's easier to just merge this thing for me now and the comments can be discussed later. I haven't made any important comments that need to be addressed promptly. @emillon feel free to review this asynchronously. |
|
Thanks for adding tests. BTW, since the option to select the build profile is called |
|
Jérémie Dimino <notifications@github.com> writes:
Thanks for adding tests. BTW, since the option to select the build
profile is called `profile` on the command line and in the workspace
file, I think the variable should be called `profile` as well.
This was my first choice but I hesitated since profile is really
generic. I'll make a PR and document this variable.
|
|
👍 for me, thanks for the tests to document the feature. |
ghost
mentioned this pull request
This PR implements #929