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

Add (alias ...), (mode ...) fields to (copy_files ...) stanza #3631

Merged
merged 7 commits into from
Jul 17, 2020

Conversation

nojb
Copy link
Collaborator

@nojb nojb commented Jul 15, 2020

What the title says:

  • (alias ...) field to specify an alias to which to attach the copy_files stanza's targets.
  • (mode ...) field with the same semantics as the one appearing in (rule ...).

The full syntax looks like:

(copy_files
 (alias foo)
 (mode promote-until-clean)
 (glob <glob>))

(maybe glob should be called names).

What do you think?

@nojb
Copy link
Collaborator Author

nojb commented Jul 15, 2020

This looks also related #3503 and (in a lesser way) #1244

@nojb nojb force-pushed the copy_files_long_form branch from 881f055 to cab6244 Compare July 15, 2020 19:24

let decode =
peek_exn >>= function
| List _ -> Dune_lang.Syntax.since Stanza.syntax (2, 7) >>> fields long_form
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This gives

Error: 'copy_files' is only available since version 2.7 of the dune language.
Please update your dune-project file to have (lang dune 2.7).

as error message, which is not exactly what we want; suggestions appreciated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

How about:

Error: (alias) and (mode) are available in this position only since version 2.7 of the dune language.
Please update your dune-project file to have (lang dune 2.7)

You can also have a look at how (enabled_if) behaves, we added support for it in more positions later on, so the error message might be similar.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the hint, I did something similar to what is done in other places.

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.

I'm not sure how to fix the error message issue, but it doesn't seem like a blocker. This needs a CHANGES/doc update as well.

~f:(fun alias ->
let alias = Alias.make alias ~dir in
Rules.Produce.Alias.add_deps alias targets)
def.alias;
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 move def.alias to be the first argumnt? Makes it simpler to read.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, it is fixed.

@nojb
Copy link
Collaborator Author

nojb commented Jul 15, 2020

Thanks for the lightning-quick review @rgrinberg! Do you think we should rename (glob ...) by (names ...) (or something else), or is glob OK?

@nojb nojb force-pushed the copy_files_long_form branch from cab6244 to 37436a5 Compare July 15, 2020 19:56
@nojb nojb requested a review from emillon as a code owner July 15, 2020 19:56
@nojb
Copy link
Collaborator Author

nojb commented Jul 15, 2020

Doc updated.

@rgrinberg
Copy link
Member

glob seems alright to me. Let's see if @jeremiedimino has another suggestion.

@ghost
Copy link

ghost commented Jul 16, 2020

Usually, we use names to denote files when we don't put the full filenames and let Dune infer the filenames from the context. For instance (c_names foo bar) to denote foo.c bar.c. In this case, I'd just use files .... It's consistent with the field names for install or cinaps.

@nojb nojb force-pushed the copy_files_long_form branch from 64f973c to 480d859 Compare July 16, 2020 08:46
nojb added 4 commits July 16, 2020 10:47
Signed-off-by: Nicolás Ojeda Bär <[email protected]>
Signed-off-by: Nicolás Ojeda Bär <[email protected]>
Signed-off-by: Nicolás Ojeda Bär <[email protected]>
@nojb nojb force-pushed the copy_files_long_form branch from 480d859 to 9332983 Compare July 16, 2020 08:48
@nojb
Copy link
Collaborator Author

nojb commented Jul 16, 2020

Renamed (glob ...) into (files ...).

nojb added 2 commits July 16, 2020 13:10
Signed-off-by: Nicolás Ojeda Bär <[email protected]>
@nojb
Copy link
Collaborator Author

nojb commented Jul 16, 2020

I fixed the error message and pushed a small test as well. On my side this is good to go.

@ghost
Copy link

ghost commented Jul 16, 2020

Cool, let's wait to see if @rgrinberg wants to comment on the naming, but otherwise it looks good to merge!

@rgrinberg
Copy link
Member

Looks good to me. I kind of wish the stanza was called copy though. We'll fix it in 3.0 I suppose :)

@nojb nojb merged commit f83aff7 into ocaml:master Jul 17, 2020
@nojb nojb deleted the copy_files_long_form branch July 17, 2020 04:45
@nojb
Copy link
Collaborator Author

nojb commented Jul 17, 2020

Merged, thanks!

@voodoos voodoos mentioned this pull request Jul 21, 2020
3 tasks
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Aug 14, 2020
…lugin, dune-private-libs and dune-glob (2.7.0)

CHANGES:

- Write intermediate files in a `.mdx` folder for each `mdx` stanza
  to prevent the corresponding actions to be executed as part of the `@all`
  alias (ocaml/dune#3659, @NathanReb)

- Read Coq flags from `env` (ocaml/dune#3547 , fixes ocaml/dune#3486, @gares)

- Allow bisect_ppx to be enabled/disabled via dune-workspace. (ocaml/dune#3404,
  @stephanieyou)

- Formatting of dune files is now done in the executing dune process instead of
  in a separate process. (ocaml/dune#3536, @nojb)

- Add a `--debug-artifact-substution` flag to help debug problem with
  version not being captured by `dune-build-info` (ocaml/dune#3589,
  @jeremiedimino)

- Allow the use of the `context_name` variable in the `enabled_if` fields of
  executable(s) and install stanzas. (ocaml/dune#3568, fixes ocaml/dune#3566, @voodoos)

- Fix compatibility with OCaml 4.12.0 when compiling empty archives; no .a file
  is generated. (ocaml/dune#3576, @dra27)

- `$ dune utop` no longer tries to load optional libraries that are unavailable
  (ocaml/dune#3612, fixes ocaml/dune#3188, @anuragsoni)

- Fix dune-build-info on 4.10.0+flambda (ocaml/dune#3599, @emillon, @jeremiedimino).

- Allow multiple libraries with `inline_tests` to be defined in the same
  directory (ocaml/dune#3621, @rgrinberg)

- Run exit hooks in jsoo separate compilation mode (ocaml/dune#3626, fixes ocaml/dune#3622,
  @rgrinberg)

- Add (alias ...), (mode ...) fields to (copy_fields ...) stanza (ocaml/dune#3631, @nojb)

- (copy_files ...) now supports copying files from outside the workspace using
  absolute file names (ocaml/dune#3639, @nojb)

- Dune does not use `ocamlc` as an intermediary to call C compiler anymore.
  Configuration flags `ocamlc_cflags` and `ocamlc_cppflags` are always prepended
  to the compiler arguments. (ocaml/dune#3565, fixes ocaml/dune#3346, @voodoos)

- Revert the build optimization in ocaml/dune#2268. This optimization slows down building
  individual executables when they're part of an `executables` stanza group
  (ocaml/dune#3644, @rgrinberg)

- Use `{dev}` rather than `{pinned}` in the generated `.opam` file. (ocaml/dune#3647,
  @kit-ty-kate)

- Insert correct extension name when editing `dune-project` files. Previously,
  dune would just insert the stanza name. (ocaml/dune#3649, fixes ocaml/dune#3624, @rgrinberg)

- Fix crash when evaluating an `mdx` stanza that depends on unavailable
  packages. (ocaml/dune#3650, @craigfe)

- Fix typo in `cache-check-probablity` field in dune config files. This field
  now requires 2.7 as it wasn't usable before this version. (ocaml/dune#3652, @edwintorok)

- Add `"odoc" {with-doc}` to the dependencies in the generated `.opam` files.
  (ocaml/dune#3667, @kit-ty-kate)

- Do not allow user actions to capture dune's stdin (ocaml/dune#3677, fixes ocaml/dune#3672,
  @rgrinberg)

- `(subdir ...)` stanzas can now appear in dune files used via `(include ...)`.
  (ocaml/dune#3676, @nojb)
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Aug 14, 2020
…lugin, dune-private-libs and dune-glob (2.7.0)

CHANGES:

- Write intermediate files in a `.mdx` folder for each `mdx` stanza
  to prevent the corresponding actions to be executed as part of the `@all`
  alias (ocaml/dune#3659, @NathanReb)

- Read Coq flags from `env` (ocaml/dune#3547 , fixes ocaml/dune#3486, @gares)

- Add instrumentation framework to toggle instrumentation by `bisect_ppx`,
  `landmarks`, etc, via dune-workspace and/or the command-line. (ocaml/dune#3404, ocaml/dune#3526
  @stephanieyou, @nojb)

- Formatting of dune files is now done in the executing dune process instead of
  in a separate process. (ocaml/dune#3536, @nojb)

- Add a `--debug-artifact-substution` flag to help debug problem with
  version not being captured by `dune-build-info` (ocaml/dune#3589,
  @jeremiedimino)

- Allow the use of the `context_name` variable in the `enabled_if` fields of
  executable(s) and install stanzas. (ocaml/dune#3568, fixes ocaml/dune#3566, @voodoos)

- Fix compatibility with OCaml 4.12.0 when compiling empty archives; no .a file
  is generated. (ocaml/dune#3576, @dra27)

- `$ dune utop` no longer tries to load optional libraries that are unavailable
  (ocaml/dune#3612, fixes ocaml/dune#3188, @anuragsoni)

- Fix dune-build-info on 4.10.0+flambda (ocaml/dune#3599, @emillon, @jeremiedimino).

- Allow multiple libraries with `inline_tests` to be defined in the same
  directory (ocaml/dune#3621, @rgrinberg)

- Run exit hooks in jsoo separate compilation mode (ocaml/dune#3626, fixes ocaml/dune#3622,
  @rgrinberg)

- Add (alias ...), (mode ...) fields to (copy_fields ...) stanza (ocaml/dune#3631, @nojb)

- (copy_files ...) now supports copying files from outside the workspace using
  absolute file names (ocaml/dune#3639, @nojb)

- Dune does not use `ocamlc` as an intermediary to call C compiler anymore.
  Configuration flags `ocamlc_cflags` and `ocamlc_cppflags` are always prepended
  to the compiler arguments. (ocaml/dune#3565, fixes ocaml/dune#3346, @voodoos)

- Revert the build optimization in ocaml/dune#2268. This optimization slows down building
  individual executables when they're part of an `executables` stanza group
  (ocaml/dune#3644, @rgrinberg)

- Use `{dev}` rather than `{pinned}` in the generated `.opam` file. (ocaml/dune#3647,
  @kit-ty-kate)

- Insert correct extension name when editing `dune-project` files. Previously,
  dune would just insert the stanza name. (ocaml/dune#3649, fixes ocaml/dune#3624, @rgrinberg)

- Fix crash when evaluating an `mdx` stanza that depends on unavailable
  packages. (ocaml/dune#3650, @craigfe)

- Fix typo in `cache-check-probablity` field in dune config files. This field
  now requires 2.7 as it wasn't usable before this version. (ocaml/dune#3652, @edwintorok)

- Add `"odoc" {with-doc}` to the dependencies in the generated `.opam` files.
  (ocaml/dune#3667, @kit-ty-kate)

- Do not allow user actions to capture dune's stdin (ocaml/dune#3677, fixes ocaml/dune#3672,
  @rgrinberg)

- `(subdir ...)` stanzas can now appear in dune files used via `(include ...)`.
  (ocaml/dune#3676, @nojb)
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.

3 participants