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

Adapt the behavior of dune subst for dune projects #960

Merged
6 commits merged into from Jul 8, 2018
Merged

Adapt the behavior of dune subst for dune projects #960

6 commits merged into from Jul 8, 2018

Conversation

ghost
Copy link

@ghost ghost commented Jul 6, 2018

Projects are better defined with dune, so we don't need the automatic project name detection jbuilder was doing and can simply read the dune-project file. Additionally, since dune subst is only meant to be called from opam files, this PR simplify this command a bit so that it doesn't looks up the root of the workspace. This fix #954 which was due to users often forgetting to pass "-p" name to the dune subst invocation.

Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@ghost ghost requested review from rgrinberg and emillon July 6, 2018 15:10
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@emillon
Copy link
Collaborator

emillon commented Jul 6, 2018

If there is no dune-project file, one will be created automatically. What part takes care of this? Will it run and do the right thing if dune subst is run in a project without a dune-project file?

@ghost
Copy link
Author

ghost commented Jul 6, 2018

It's done in File_tree.load: whenever we encounter a dune file, we make sure the dune-project file exists and create it if not. dune subst doesn't call File_tree.load, so this won't happen. Additionally, the dune-project file that's automatically created doesn't have a name field, so dune subst would still fail on such a file. The name must be set explicitly by the user.

You must pass a [--name] command line argument."
in
if not (List.mem name ~set:package_names) then
die "@{<error>Error@}: file %s.opam doesn't exist." name;
Copy link
Member

Choose a reason for hiding this comment

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

Could you harmonize this error message with the one on line 180? Also I wonder if it makes sense to include the package names.

Copy link
Author

Choose a reason for hiding this comment

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

What do you have in mind for this error message?

Copy link
Member

Choose a reason for hiding this comment

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

Looking at this again, the error on line 180 is slightly different. So ignore that comment. Everything looks fine. Though it might still be useful to include package_names in the error for convenience.

@rgrinberg
Copy link
Member

Btw, I notice that subst doesn't really have a test suite at the moment. @diml would you mind adding some general tests for it? Otherwise I can just create a ticket and we can do it at another time.

Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@ghost
Copy link
Author

ghost commented Jul 7, 2018

Sure, I added a test

You must pass a [--name] command line argument."
in
if not (List.mem name ~set:package_names) then
die "@{<error>Error@}: file %s.opam doesn't exist." name;
Copy link
Member

Choose a reason for hiding this comment

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

Looking at this again, the error on line 180 is slightly different. So ignore that comment. Everything looks fine. Though it might still be useful to include package_names in the error for convenience.

@rgrinberg
Copy link
Member

PS, the tests still need to pass:

File "test/blackbox-tests/test-cases/subst/file.ml", line 2, characters 15-30:
Error: variable "authors" not found in opam file

Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@ghost
Copy link
Author

ghost commented Jul 7, 2018

Ah, it's because ./boot --subst tries to edit the tests as well... I fixed that.

Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@ghost
Copy link
Author

ghost commented Jul 8, 2018

Though it might still be useful to include package_names in the error for convenience.

Yh, I'm not sure how to word the error message. The name comes from the dune-project file, not from the command line. So the problem is either that:

  • the (name ...) field is wrong
  • the <package>.opam file has the wrong name

@ghost
Copy link
Author

ghost commented Jul 8, 2018

I'm merging for now, we can improve this message in another PR

This pull request was closed.
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.

jbuilder subst -n <name> is sometimes not properly rooted
3 participants