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.

Sign up for GitHub

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

Jump to bottom

Add support for dynamic dependencies via (include ..) in (deps) #5442

Closed
wants to merge 7 commits into from
Closed

Add support for dynamic dependencies via (include ..) in (deps) #5442

wants to merge 7 commits into from

Conversation

anmonteiro
Copy link
Collaborator

@anmonteiro anmonteiro commented Feb 16, 2022 •
edited
Loading

Signed-off-by: Antonio Nuno Monteiro anmonteiro@gmail.com

This implements "the full power of dynamic deps" in alternative to the more hacky #5440.

@anmonteiro anmonteiro mentioned this pull request Feb 16, 2022
Closed
@anmonteiro anmonteiro force-pushed the anmonteiro/dynamic-dependencies branch from ecd5c5e to b5ddc71 Compare February 16, 2022 08:36
@anmonteiro anmonteiro mentioned this pull request Feb 16, 2022
Merged
@rgrinberg rgrinberg added this to the 3.0.3 milestone Feb 18, 2022
rgrinberg
rgrinberg reviewed Feb 19, 2022
View reviewed changes
src/dune_rules/dep_conf_eval.ml Outdated
let+ contents = Build_system.read_file path ~f:Io.read_file in
let asts =
Dune_lang.Parser.parse_string ~fname:(Path.to_string path)
~mode:Dune_lang.Parser.Mode.Many contents
Copy link
Member

Choose a reason for hiding this comment

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

The choice of Many isn't, but it's inconsistent with how use it in the ordered set lang. I suggest we use Builld_system.read_sexp and interpret the list from it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I changed the implementation to use read_sexp and it just results in:

 Error: too many s-expressions found in input

Was your change that we add a ~mode argument to it for the use case here? I'm gonna go ahead and propose that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Proposal in 4341aa4

Copy link
Member

Choose a reason for hiding this comment

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

Here's the problem that I'm trying to point.

In this example:

(deps (:include foo))

The expectation is that foo is a Sexp.t list.

While in:

(flags (:include foo))

The expectation is that foo is Sexp.t.

I found this inconsistence to be jarring.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh you’re saying maybe the file needs to be a sexp, got it now.

there’s also the include vs :include difference. I think (:include here would bind a new named variable so I chose include

Copy link
Member

Choose a reason for hiding this comment

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

there’s also the include vs :include difference. I think (:include here would bind a new named variable so I chose include

Yes, that's fine. In ordered set lang, we need to distinguish between literals and special forms somehow. That's not necessary for dependency specifications.

@anmonteiro anmonteiro force-pushed the anmonteiro/dynamic-dependencies branch from 4341aa4 to 6b9d696 Compare February 20, 2022 07:54
@rgrinberg
Copy link
Member

I had a change of heart wrt versioning. I think it would be better if we delayed this feature for 3.1.0. For the %{read-lines} pr, it's more obvious that it was a bug fix because %{read} was enabled and that case was difficult to version. To me, this looks more like a totally new feature and we should wait until 3.1. Besides, we're inviting people to write wrong constraints in their opam files. If one uses this feature with the originally accepted version, they ought to require >= 3.0.3. I highly doubt people will remember to do that.

@anmonteiro
Copy link
Collaborator Author

That sounds good to me.

@rgrinberg rgrinberg modified the milestones: 3.0.3, 3.1.0 Feb 22, 2022
@anmonteiro anmonteiro force-pushed the anmonteiro/dynamic-dependencies branch from 6efc383 to c1f4eb7 Compare February 24, 2022 22:45
@anmonteiro
Add support for dynamic dependencies via (include ..) in (deps)
694b8e3 
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro
add another dep to test
f4d0412 
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro
demonstrate recursive power
f7897e0 
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro
reuse better
2273518 
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro
Use Action_builder.read_sexp
4b8ae03 
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro anmonteiro force-pushed the anmonteiro/dynamic-dependencies branch from c1f4eb7 to 1116088 Compare February 24, 2022 22:49
@anmonteiro
chore: make dynamic dependencies available starting from lang 3.1
d2bf4fb 
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro
fix: Use read_sexp in ~mode:Single
7936c24 
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro anmonteiro force-pushed the anmonteiro/dynamic-dependencies branch from 1116088 to 7936c24 Compare February 24, 2022 22:50
@anmonteiro
Copy link
Collaborator Author

I just updated the PR after the latest suggestions.

@rgrinberg
Copy link
Member

Cherry picked. Cheers.

@rgrinberg rgrinberg closed this Feb 28, 2022
This was referenced Mar 1, 2022
Merged
Merged
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Apr 15, 2022
@rgrinberg
[new release] xdg, stdune, ordering, ocamlc-loc, fiber, dyn, dune, du…
ba3c1b9 
…ne-site, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info and dune-action-plugin (3.1.0)

CHANGES:

- Add `sourcehut` as an option for defining project sources in dune-project
  files. For example, `(source (sourcehut user/repo))`. (ocaml/dune#5564, @rgrinberg)

- Add `dune coq top` command for running a Coq toplevel (ocaml/dune#5457, @rlepigre)

- Fix dune exec dumping database in wrong directory (ocaml/dune#5544, @bobot)

- Always output absolute paths for locations in RPC reported diagnostics
  (ocaml/dune#5539, @rgrinberg)

- Add `(deps <deps>)` in ctype field (ocaml/dune#5346, @bobot)

- Add `(include <file>)` constructor to dependency specifications. This can be
  used to introduce dynamic dependencies (ocaml/dune#5442, @anmonteiro)

- Ensure that `dune describe` computes a transitively closed set of
  libraries (ocaml/dune#5395, @esope)

- Add direct dependencies to $ dune describe output (ocaml/dune#5412, @esope)

- Show auto-detected concurrency on Windows too (ocaml/dune#5502, @MisterDA)

- Fix operations that remove folders with absolute path. This happens when
  using esy (ocaml/dune#5507, @EduardoRFS)

- Dune will not fail if some directories are non-empty when uninstalling.
  (ocaml/dune#5543, fixes ocaml/dune#5542, @nojb)

- `coqdep` now depends only on the filesystem layout of the .v files,
  and not on their contents (ocaml/dune#5547, helps with ocaml/dune#5100, @ejgallego)

- The mdx stanza 0.2 can now be used with `(implicit_transitive_deps false)`
  (ocaml/dune#5558, fixes ocaml/dune#5499, @emillon)

- Fix missing parenthesis in printing of corresponding terminal command for
  `(with-outputs-to )` (ocaml/dune#5551, fixes ocaml/dune#5546, @Alizter)
@rgrinberg rgrinberg mentioned this pull request Apr 15, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgrinberg rgrinberg rgrinberg left review comments

@snowleopard snowleopard Awaiting requested review from snowleopard

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.1.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@anmonteiro @rgrinberg