Skip to content

Add support for dynamic deps coming from %{read-lines:...}#5440

Closed
anmonteiro wants to merge 5 commits intoocaml:mainfrom
anmonteiro:anmonteiro/try-dynamic-deps
Closed

Add support for dynamic deps coming from %{read-lines:...}#5440
anmonteiro wants to merge 5 commits intoocaml:mainfrom
anmonteiro:anmonteiro/try-dynamic-deps

Conversation

@anmonteiro
Copy link
Copy Markdown
Collaborator

@anmonteiro anmonteiro commented Feb 16, 2022

Motivation

  • I read in the Dune 3.0 release information that this should be possible but when trying it out I couldn't make it work

Implementation

  • The approach taken here is to try and parse multiple "paths" out of an expanded macro. If this is not the preferred approach, I'd love to hear alternatives!

Test plan

  • Added a test case to exemplify the behavior I'm talking about, which failed with the following message:
File "dune", line 26, characters 2-19:
26 |   %{read-lines:foo})
       ^^^^^^^^^^^^^^^^^
Error: Macro %{read-lines:..} expands to 2 values, however a single value is
expected here. Please quote this atom.

@anmonteiro
try fixing dyn deps
d7c5a54 
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro
add test case
b90d7cd 
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
rgrinberg
rgrinberg reviewed Feb 16, 2022
View reviewed changes
src/dune_engine/string_with_vars.ml Outdated

let decode = decode_manually Pform.Env.parse

let decode_many f =
Copy link
Copy Markdown
Member

@rgrinberg rgrinberg Feb 16, 2022

Choose a reason for hiding this comment

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

I don't see this function being used anywhere?

Copy link
Copy Markdown
Collaborator Author

@anmonteiro anmonteiro Feb 16, 2022

Choose a reason for hiding this comment

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

good catch, was from a previous implementation attempt.

@rgrinberg
Copy link
Copy Markdown
Member

rgrinberg commented Feb 16, 2022

Thanks, that is indeed an embarrassing omission. I haven't looked closely yet, but I think your fix is on the right track. But first let's make sure we're on the same page. The current behavior is such that (deps %{read-lines:foo}) will interpret lines in foo as file dependencies, right? If so, that isn't actually the full power of dynamic deps. For full power we should support (deps (:include filename)) which should allows to include glob, aliases, and all other dependencies other than files.

@anmonteiro
Copy link
Copy Markdown
Collaborator Author

anmonteiro commented Feb 16, 2022

My fix does the first thing, indeed. I haven't tried to implement (:include and other rules, but it might be fun. Any pointers?

Indeed these are currently interpreted as file dependencies. Should we try to evaluate them differently, is that what you're saying?

@anmonteiro
Remove unused code
b69621d 
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro anmonteiro force-pushed the anmonteiro/try-dynamic-deps branch from 1576f55 to b69621d Compare February 16, 2022 03:32
@anmonteiro anmonteiro mentioned this pull request Feb 16, 2022
Closed
@anmonteiro
Copy link
Copy Markdown
Collaborator Author

anmonteiro commented Feb 16, 2022

@rgrinberg I opened #5442 which I believe does what you suggest, and looks a lot better in my subjective opinion.

@rgrinberg
Copy link
Copy Markdown
Member

rgrinberg commented Feb 18, 2022

I had a discussion @snowleopard about both of you PR's. The discussion was mostly about whether these changes should make it 3.0.x or be delayed until 3.1.0. We decided that it's safe to include them in 3.0.3. Our original intention was clearly to support this in 3.0. For example, we already had support %{read} to read dependencies. It doesn't make sense to omit support for %{read-lines} as well. Of course the :include form is the most popular so we should support it as well.

@rgrinberg rgrinberg added this to the 3.0.3 milestone Feb 18, 2022
@anmonteiro
Copy link
Copy Markdown
Collaborator Author

anmonteiro commented Feb 18, 2022

coming to think of it I'm not sure whether #5440 should be included. for some reason (deps %{read-lines:./path/to/file}) reads really weird to me. And there's also the limitation that it only works with file dependencies. I'd actually propose we close it.

@rgrinberg
Copy link
Copy Markdown
Member

rgrinberg commented Feb 18, 2022

It's a bit weird to have %{read} and not %{read-lines} dont you think?

@anmonteiro
Copy link
Copy Markdown
Collaborator Author

anmonteiro commented Feb 18, 2022 •
edited
Loading

I guess that's fair. I'm happy to do the work, I just thought it'd be a little cumbersome. The problem isn't necessarily about read-lines, as you probably know, but rather that the expansion mode is ~mode:Single for each item in deps.

I thought that relaxing that made it a little inconsistent, but it's up to you. I'll personally be sticking to (include ./x) which seems easier to use (and does more!).

@rgrinberg rgrinberg removed the request for review from snowleopard February 19, 2022 05:12
snowleopard
snowleopard approved these changes Feb 19, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@snowleopard snowleopard left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! I added some suggestions for the test.

@anmonteiro
Copy link
Copy Markdown
Collaborator Author

anmonteiro commented Feb 20, 2022

Thanks for the reviews here. I addressed the suggestions and the test does indeed look more interesting to read now.

@snowleopard
Copy link
Copy Markdown
Collaborator

snowleopard commented Feb 20, 2022

Thanks for the reviews here. I addressed the suggestions and the test does indeed look more interesting to read now.

Great! Please add a changelog entry. Otherwise, I think it's ready to be merged.

@rgrinberg
Copy link
Copy Markdown
Member

rgrinberg commented Feb 20, 2022

I added a CHANGES entry and merged. Thanks.

@rgrinberg rgrinberg closed this Feb 20, 2022
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Mar 1, 2022
@rgrinberg
[new release] xdg, stdune, ordering, fiber, dyn, dune, dune-site, dun…
94a015b 
…e-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info and dune-action-plugin (3.0.3)

CHANGES:

- Do not enable warnings 63-70 by default (ocaml/dune#5476, fixes ocaml/dune#5464, @rgrinberg)

- Allow %{read-lines} to introduce dynamic dependencies like %{read}. (ocaml/dune#5440,
  @anmonteiro)

- Look up `gmake` before `make` (ocaml/dune#5474, fixes ocaml/dune#5470, @rgrinberg)

- Handle empty output from `getconf` (ocaml/dune#5473 fixes ocaml/dune#5471, @mndrix)

- Depend on any provided `foreign_archives` for ctypes stub generation (ocaml/dune#5475,
  @mbacarella)
@rgrinberg rgrinberg mentioned this pull request Mar 1, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rgrinberg rgrinberg rgrinberg approved these changes

@snowleopard snowleopard snowleopard approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

3.0.3

Development

Successfully merging this pull request may close these issues.

3 participants

@anmonteiro @rgrinberg @snowleopard