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

[RFC] Lift subdirectory restriction on copy_files stanza? #1323

Merged
5 commits merged into from
Sep 24, 2018
Merged

[RFC] Lift subdirectory restriction on copy_files stanza? #1323

5 commits merged into from
Sep 24, 2018

Conversation

nojb
Copy link
Collaborator

@nojb nojb commented Sep 24, 2018

In #911 (comment), it is implied that this restriction should not be required anymore.

I don't yet know enough about the inner workings of dune to investigate this on my own, so:

  • Can this restriction indeed be removed?
  • If yes, is there anything else to adapt to make it so? What is the merlin issue alluded to in the comment in the code?

@nojb
Lift subdirectory restriction on copy_files stanza
9071f85 
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb nojb requested a review from a user September 24, 2018 09:28
@ghost
Copy link

ghost commented Sep 24, 2018

Yes it can be lifted. The limitation is that if you write this in src/dune: (copy_files ../foo/*.{ml,mli}), then merlin will not work in the foo directory because no .merlin will be generated there.

This seems like an acceptable limitation to me, especially given that copy_files is not restricted to OCaml files. include_subdirs is now the preferred way of defining multi-directory libraries, so in fact we could stop looking at copy_files stanzas when generating the .merlin.

BTW, in general we try to make it so that new features are only available if the user writes the correct version in the dune-project file via the (lang dune <version>) line. Could you update this PR to take this version number into account? Basically, what you need to do is capture this version while parsing the copy_files stanza via let%map version = Syntax.get Stanza.syntax ..., store it in the record and then change the check in simple_rules.ml to something like this:

if def.syntax_version < (1, 4) && not (Path.is_descendant ...) then
  Syntax.Error.since ...

You also need to upgrade the latest version of Syntax.stanza to (1, 4) in stanza.ml.

@rgrinberg
Copy link
Member

rgrinberg commented Sep 24, 2018 •
edited
Loading

You can actually make this available in 1.3 as we will need to re-release it as upgrading to opam 2.0 is required to submit packages now

@nojb
Only enable in 1.3 onwards
715bf34 
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb
Copy link
Collaborator Author

nojb commented Sep 24, 2018

Thanks for the review! I pushed a commit enabling this only from 1.3 onwards.

@rgrinberg
Copy link
Member

Would you mind updating the copy_files tests? You might need to organize them a little better with different roots for some test cases (you can look at the multi-dir tests for an example of how to do that).

Also, don't forget a CHANGES entry.

@nojb
Update CHANGES
61a6079 
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb nojb force-pushed the copy_files_restriction branch 2 times, most recently from d96e526 to 3cb7e76 Compare September 24, 2018 13:14
@nojb
Add test
c5f32a0 
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb nojb force-pushed the copy_files_restriction branch from 3cb7e76 to c5f32a0 Compare September 24, 2018 13:14
@nojb
Copy link
Collaborator Author

nojb commented Sep 24, 2018

Updated CHANGES and added a small test.

rgrinberg
rgrinberg approved these changes Sep 24, 2018
View reviewed changes
Copy link
Member

@rgrinberg rgrinberg 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 nojb. Thanks for the contribution

src/simple_rules.ml
ensures that [sources_and_targets_known_so_far] returns the
right answer for sub-directories only. *)
if not (Path.is_descendant glob_in_src ~of_:src_dir) then
if def.syntax_version < (1, 3) && not (Path.is_descendant glob_in_src ~of_:src_dir) then
Copy link
Member

Choose a reason for hiding this comment

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

Just a note that we usually wrap our code to 80 chars. I'm not sure if it's over the limit here, but something to keep in mind.

@ghost ghost merged commit 499aa2a into ocaml:master Sep 24, 2018
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Sep 24, 2018
@rgrinberg
[new release] dune (1.3.0)
f38ff31 
CHANGES:

- Support colors on Windows (ocaml/dune#1290, @diml)

- Allow `dune.configurator` and `base` to be used together (ocaml/dune#1291, fix
  ocaml/dune#1167, @diml)

- Support interrupting and restarting builds on file changes (ocaml/dune#1246,
  @kodek16)

- Fix findlib-dynload support with byte mode only (ocaml/dune#1295, @bobot)

- Make `dune rules -m` output a valid makefile (ocaml/dune#1293, @diml)

- Expand variables in `(targets ..)` field (ocaml/dune#1301, ocaml/dune#1320, fix ocaml/dune#1189, @nojb,
  @rgrinberg, @diml)

- Fix a race condition on Windows that was introduced in 1.2.0
  (ocaml/dune#1304, fix ocaml/dune#1303, @diml)

- Fix the generation of .merlin files to account for private modules
  (@rgrinberg, fix ocaml/dune#1314)

- Exclude the local opam switch directory (`_opam`) from the list of watched
  directories (ocaml/dune#1315, @dysinger)

- Fix compilation of the module generated for `findlib.dynload`
  (ocaml/dune#1317, fix ocaml/dune#1310, @diml)

- Lift restriction on `copy_files` and `copy_files#` stanzas that files to be
  copied should be in a subdirectory of the current directory.
  (ocaml/dune#1323, fix ocaml/dune#911, @nojb)
rgrinberg pushed a commit to rgrinberg/jbuilder that referenced this pull request Sep 25, 2018
@nojb @rgrinberg
Lift subdirectory restriction on copy_files stanza (ocaml#1323)
c81ecba 
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb nojb mentioned this pull request Sep 30, 2018
Closed
shonfeder pushed a commit to shonfeder/dune that referenced this pull request Dec 31, 2018
@nojb @shonfeder
Lift subdirectory restriction on copy_files stanza (ocaml#1323)
8bc01eb 
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emillon emillon emillon approved these changes

@rgrinberg rgrinberg rgrinberg approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@nojb @rgrinberg @emillon @jeremiedimino