Skip to content

Add another test for substituting in dependencies#1537

Merged
rgrinberg merged 3 commits intoocaml:masterfrom
rgrinberg:more-subs
Nov 14, 2018
Merged

Add another test for substituting in dependencies#1537
rgrinberg merged 3 commits intoocaml:masterfrom
rgrinberg:more-subs

Conversation

@rgrinberg
Copy link
Copy Markdown
Member

cc @ejgallego it seems to work here.

This raname was accidentally omitted

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
@rgrinberg rgrinberg requested a review from a user November 13, 2018 20:46
@rgrinberg rgrinberg requested a review from emillon as a code owner November 13, 2018 20:46
@ejgallego
Copy link
Copy Markdown
Collaborator

I see thanks, it seems the problem was the error message I was getting because I was not using the right library name.

I do wonder tho how the substitution happens for install files that are not part of a library, that is to say, data files in directory data where data/dune only contains (install ...) stanzas.

@rgrinberg
Copy link
Copy Markdown
Member Author

I see thanks, it seems the problem was the error message I was getting because I was not using the right library name.

What mistake did you make? I'd like to improve dune to give proper error in such cases.

I do wonder tho how the substitution happens for install files that are not part of a library, that is to say, data files in directory data where data/dune only contains (install ...) stanzas.

Yeah, indeed that's a bit confusing. I wonder if we should have a %{pkg:..} macro for such cases? If you run into such a case, please open a ticket and we can discuss if %{pkg:..} would help.

@ejgallego
Copy link
Copy Markdown
Collaborator

What mistake did you make? I'd like to improve dune to give proper error in such cases.

Use %{lib:foo} where foo is not a library name. The error message is a bit confusing indeed:

Error: Unknown macro %{lib:..}

Yeah, indeed that's a bit confusing. I wonder if we should have a %{pkg:..} macro for such cases? If you run into such a case, please open a ticket and we can discuss if %{pkg:..} would help.

Indeed thanks, we run into that case as the coq package has many files that are not part of a ML library; I've added a workaround for now by declaring a dummy lib, however it may not be ideal; not sure tho.

@rgrinberg
Copy link
Copy Markdown
Member Author

Okay, let's ask @emillon and @diml what they think about %{pkg:..} and the workaround of dummy libs.

@ghost
Copy link
Copy Markdown

ghost commented Nov 14, 2018

What would %{pkg:...} expand to? When installed, the files of a package are a bit scattered around :/

On the other hand, we could have %{share:<pkg>}, ... i.e. one %{xxx:...} form for every install section.

@ejgallego
Copy link
Copy Markdown
Collaborator

On the other hand, we could have %{share:<pkg>}, ... i.e. one %{xxx:...} form for every install section.

That'd look great indeed; and would surely suffice for most cases.

For Coq's object files we may end up using ${lib:...} tho, as object files in Coq can be both OCaml object files or custom .vo.

@rgrinberg
Copy link
Copy Markdown
Member Author

What would %{pkg:...} expand to? When installed, the files of a package are a bit scattered around :/

On the other hand, we could have %{share:}, ... i.e. one %{xxx:...} form for every install section.

What I had in mind was %{pkg:<pkg>/share/...} but I think that's basically equivalent to your suggestion. I'd still prefer if we had some sort of common macro prefix rather than %{share:..}, %{etc:..}.

@ghost
Copy link
Copy Markdown

ghost commented Nov 14, 2018

We could also have: %{pkg-file:<pkg>:<section>:<filename>}. Though %{share:...}, %{etc:...} nicely extend the existing %{lib:...}, %{bin:...} forms.

@rgrinberg
Copy link
Copy Markdown
Member Author

I'm probably overestimating the risks here, but it's possible that a new section would be added that could conflict with an existing form.

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.

2 participants