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

Fix #2061 and allow to promote a subset of the targets #2068

Merged
15 commits merged into from Apr 23, 2019
Merged

Fix #2061 and allow to promote a subset of the targets #2068

15 commits merged into from Apr 23, 2019

Conversation

ghost
Copy link

@ghost ghost commented Apr 17, 2019

(this PR depends on #2066)

This PR fixes the underlying problem exposed in #2061 and add a new promotion option to encode the need of projects affected by it.

Promoting only a subset of targets

It is now possible to promote only a subset of the targets of a rule via (mode (promote (only <predicate>))), for instance:

(ocamlyacc
 (modules foo)
 (mode (promote (only *.mli))))

In the above example, the user promotes the generated mli so that they can freely inspect it, however the .ml is kept in the _build directory.

The syntax of the promote mode was generalised to allow taking arbitraty options. For instance promote-until-clean can now be written (promote (until-clean)).

Fixing #2061

For projects using dune language < 1.10, if a rule produces multiple targets and only a strict subset is present in the source tree, we implicitly set its mode to (mode (promote (only <targets-present-in-the-source-tree>))). This avoids the race condition observed in #2061.

For projects using dune language >= 1.10, this is now a hard failure. This is technically a breaking change, however the old behaviour was more confusing than anything and it has been a warning for a very long time. Additionally, projects with a dune-project file won't be affected until they upgrade to (lang dune 1.10).

@ghost ghost requested review from aalekseyev, emillon and rgrinberg as code owners April 17, 2019 15:38
@ghost ghost mentioned this pull request Apr 17, 2019
Closed
@kit-ty-kate
Copy link
Member

I like this proposal very much. For the purpose of testing, would there be a way to trigger an error for any project that have this problem and not just those with (lang dune 1.10)? I would like to remove any potential problem from the opam-repository and if possible by also avoiding to modify every opam packages that uses dune or jbuilder. Something like a global environment variable or something like that would be very much appreciated. (or even if it's just a custom branch that is not going to be released that I can pull and test all the packages with would be very good)

@ghost
Copy link
Author

ghost commented Apr 17, 2019

No problem. I created a branch hard-fail-on-target-and-source-overlap in github.com/diml/dune. It's based on dune 1.9.1.

@kit-ty-kate
Copy link
Member

No problem. I created a branch hard-fail-on-target-and-source-overlap in github.com/diml/dune. It's based on dune 1.9.1.

Thank you!

jeremiedimino and others added 14 commits April 18, 2019 11:31
@jeremiedimino
Add a reproduction case for #2061
7b7dfcc 
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@jeremiedimino
Change the argument of Promote to a record
7c0bbd5 
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@jeremiedimino
Bump language to 1.10
c86cbaa 
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@jeremiedimino
Add a (promote <options>) rule mode form
1829532 
So that we can more easily add new option to the promote mode.

Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@jeremiedimino
Add support for (mode (promote (only ...)))
7eacf41 
To promote only a subset of the targets.

Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@jeremiedimino
Fix #2061
aa58ce4 
Improve the behavior when a rule in standard mode produces multiple
targets and only a subset is present in the source tree.

In this case, put the rule in (promote (only ...)) mode.

Additionally, dune now fails hard on projects using the dune language
>= 1.10 in such cases.

Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@jeremiedimino
Improve error message for multiple rules
cc47180 
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@jeremiedimino
Update changelog
a7a4cf9 
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@jeremiedimino
Make error message even simpler
05a2213 
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@jeremiedimino
Remove no longer necessary Not_a_rule_stanza case
a2215fc 
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@jeremiedimino
Add a hint for multiple rules error
cc3f438 
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@jeremiedimino
Use the compatibility behavior when using the jbuilder binary
83bd644 
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@aalekseyev
Merge branch 'master' into promote-subset
057a3c4
@aalekseyev
move legacy fallback rules fixup into a separate function
e67309c 
Signed-off-by: Arseniy Alekseyev <aalekseyev@janestreet.com>
aalekseyev
aalekseyev approved these changes Apr 18, 2019
View reviewed changes
Copy link
Collaborator

@aalekseyev aalekseyev 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.

@ghost ghost merged commit 7d5f47a into ocaml:master Apr 23, 2019
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request May 30, 2019
@rgrinberg
[new release] dune (1.10.0)
31f7a06 
CHANGES:

- Restricted the set of variables available for expansion in the destination
  filename of `install` stanza to simplify implementation and avoid dependency
  cycles. (ocaml/dune#2073, @aalekseyev, @diml)

- [menhir] call menhir from context root build_dir (ocaml/dune#2067, @ejgallego,
  review by @diml, @rgrinberg)

- [coq] Add `coq.pp` stanza to help with pre-processing of grammar
  files (ocaml/dune#2054, @ejgallego, review by @rgrinberg)

- Add a new more generic form for the *promote* mode: `(promote
  (until-clean) (into <dir>))` (ocaml/dune#2068, @diml)

- Allow to promote only a subset of the targets via `(promote (only
  <pred>))`. For instance: `(promote (only *.mli))` (ocaml/dune#2068, @diml)

- Improve the behavior when a strict subset of the targets of a rule is already
  in the source tree for projects using the dune language < 1.10 (ocaml/dune#2068, fixes
  ocaml/dune#2061, @diml)

- With lang dune >= 1.10, rules in standard mode are no longer allowed to
  produce targets that are present in the source tree. This has been a warning
  for long enough (ocaml/dune#2068, @diml)

- Allow %{...} variables in pps flags (ocaml/dune#2076, @mlasson review by @diml and
  @aalekseyev).

- Add a 'cookies' option to ppx_rewriter/deriver flags in library stanzas. This
  allow to specify cookie requests from variables expanded at each invocation of
  the preprocessor. (ocaml/dune#2106, @mlasson @diml)

- Add more opam metadata and use it to generate `.opam` files. In particular, a
  `package` field has been added to specify package specific information.
  (ocaml/dune#2017, ocaml/dune#2091, @avsm, @jonludlam, @rgrinberg)

- Clean up the special support for `findlib.dynload`. Before, Dune would simply
  match on the library name. Now, we only match on the findlib package name when
  the library doesn't come from Dune. Someone writing a library called
  `findlib.dynload` with Dune would have to add `(special_builtin_support
  findlib_dynload)` to trigger the special behavior. (ocaml/dune#2115, @diml)

- Install the `future_syntax` preprocessor as `ocaml-syntax-shims.exe` (ocaml/dune#2125,
  @rgrinberg)

- Hide full command on errors and warnings in development and show them in CI.
  (detected using the `CI` environment variable). Commands for which the
  invocation might be omitted must output an error prefixed with `File `. Add an
  `--always-show-command-line` option to disable this behavior and always show
  the full command. (ocaml/dune#2120, fixes ocaml/dune#1733, @rgrinberg)

- In `dune-workspace` files, add the ability to choose the host context and to
  create duplicates of the default context with different settings. (ocaml/dune#2098,
  @TheLortex, review by @diml, @rgrinberg and @aalekseyev)

- Add support for hg in `dune subst` (ocaml/dune#2135, @diml)

- Don't build documentation for implementations of virtual libraries (ocaml/dune#2141,
  fixes ocaml/dune#2138, @jonludlam)

- Fix generation of the `-pp` flag in .merlin (ocaml/dune#2142, @rgrinberg)

- Make `dune subst` add a `(version ...)` field to the `dune-project`
  file (ocaml/dune#2148, @diml)

- Add the `%{os_type}` variable, which is a short-hand for
  `%{ocaml-config:os_type}` (ocaml/dune#1764, @diml)

- Allow `enabled_if` fields in `library` stanzas, restricted to the
  `%{os_type}`, `%{model}`, `%{architecture}`, `%{system}` variables (ocaml/dune#1764,
  ocaml/dune#2164 @diml, @rgrinberg)

- Fix `chdir` on external and source paths. Dune will also fail gracefully if
  the external or source path does not exist (ocaml/dune#2165, fixes ocaml/dune#2158, @rgrinberg)

- Support the `.cc` extension fro C++ sources (ocaml/dune#2195, fixes ocaml/dune#83, @rgrinberg)

- Run `ocamlformat` relative to the context root. This improves the locations of
  errors. (ocaml/dune#2196, fixes ocaml/dune#1370, @rgrinberg)

- Fix detection of `README`, `LICENSE`, `CHANGE`, and `HISTORY` files. These
  would be undetected whenever the project was nested in another workspace.
  (ocaml/dune#2194, @rgrinberg)

- Fix generation of `.merlin` whenever there's more than one stanza with the
  same ppx preprocessing specification (ocaml/dune#2209 ,fixes ocaml/dune#2206, @rgrinberg)

- Fix generation of `.meriln` in the presence of the `copy_files` stanza and
  preprocessing specifications of other stanazs. (ocaml/dune#2211, fixes ocaml/dune#2206,
  @rgrinberg)
@rgrinberg rgrinberg mentioned this pull request May 30, 2019
Closed
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Jun 4, 2019
@rgrinberg
[new release] dune (1.10.0)
cfa54d7 
CHANGES:

- Restricted the set of variables available for expansion in the destination
  filename of `install` stanza to simplify implementation and avoid dependency
  cycles. (ocaml/dune#2073, @aalekseyev, @diml)

- [menhir] call menhir from context root build_dir (ocaml/dune#2067, @ejgallego,
  review by @diml, @rgrinberg)

- [coq] Add `coq.pp` stanza to help with pre-processing of grammar
  files (ocaml/dune#2054, @ejgallego, review by @rgrinberg)

- Add a new more generic form for the *promote* mode: `(promote
  (until-clean) (into <dir>))` (ocaml/dune#2068, @diml)

- Allow to promote only a subset of the targets via `(promote (only
  <pred>))`. For instance: `(promote (only *.mli))` (ocaml/dune#2068, @diml)

- Improve the behavior when a strict subset of the targets of a rule is already
  in the source tree for projects using the dune language < 1.10 (ocaml/dune#2068, fixes
  ocaml/dune#2061, @diml)

- With lang dune >= 1.10, rules in standard mode are no longer allowed to
  produce targets that are present in the source tree. This has been a warning
  for long enough (ocaml/dune#2068, @diml)

- Allow %{...} variables in pps flags (ocaml/dune#2076, @mlasson review by @diml and
  @aalekseyev).

- Add a 'cookies' option to ppx_rewriter/deriver flags in library stanzas. This
  allow to specify cookie requests from variables expanded at each invocation of
  the preprocessor. (ocaml/dune#2106, @mlasson @diml)

- Add more opam metadata and use it to generate `.opam` files. In particular, a
  `package` field has been added to specify package specific information.
  (ocaml/dune#2017, ocaml/dune#2091, @avsm, @jonludlam, @rgrinberg)

- Clean up the special support for `findlib.dynload`. Before, Dune would simply
  match on the library name. Now, we only match on the findlib package name when
  the library doesn't come from Dune. Someone writing a library called
  `findlib.dynload` with Dune would have to add `(special_builtin_support
  findlib_dynload)` to trigger the special behavior. (ocaml/dune#2115, @diml)

- Install the `future_syntax` preprocessor as `ocaml-syntax-shims.exe` (ocaml/dune#2125,
  @rgrinberg)

- Hide full command on errors and warnings in development and show them in CI.
  (detected using the `CI` environment variable). Commands for which the
  invocation might be omitted must output an error prefixed with `File `. Add an
  `--always-show-command-line` option to disable this behavior and always show
  the full command. (ocaml/dune#2120, fixes ocaml/dune#1733, @rgrinberg)

- In `dune-workspace` files, add the ability to choose the host context and to
  create duplicates of the default context with different settings. (ocaml/dune#2098,
  @TheLortex, review by @diml, @rgrinberg and @aalekseyev)

- Add support for hg in `dune subst` (ocaml/dune#2135, @diml)

- Don't build documentation for implementations of virtual libraries (ocaml/dune#2141,
  fixes ocaml/dune#2138, @jonludlam)

- Fix generation of the `-pp` flag in .merlin (ocaml/dune#2142, @rgrinberg)

- Make `dune subst` add a `(version ...)` field to the `dune-project`
  file (ocaml/dune#2148, @diml)

- Add the `%{os_type}` variable, which is a short-hand for
  `%{ocaml-config:os_type}` (ocaml/dune#1764, @diml)

- Allow `enabled_if` fields in `library` stanzas, restricted to the
  `%{os_type}`, `%{model}`, `%{architecture}`, `%{system}` variables (ocaml/dune#1764,
  ocaml/dune#2164 @diml, @rgrinberg)

- Fix `chdir` on external and source paths. Dune will also fail gracefully if
  the external or source path does not exist (ocaml/dune#2165, fixes ocaml/dune#2158, @rgrinberg)

- Support the `.cc` extension fro C++ sources (ocaml/dune#2195, fixes ocaml/dune#83, @rgrinberg)

- Run `ocamlformat` relative to the context root. This improves the locations of
  errors. (ocaml/dune#2196, fixes ocaml/dune#1370, @rgrinberg)

- Fix detection of `README`, `LICENSE`, `CHANGE`, and `HISTORY` files. These
  would be undetected whenever the project was nested in another workspace.
  (ocaml/dune#2194, @rgrinberg)

- Fix generation of `.merlin` whenever there's more than one stanza with the
  same ppx preprocessing specification (ocaml/dune#2209 ,fixes ocaml/dune#2206, @rgrinberg)

- Fix generation of `.merlin` in the presence of the `copy_files` stanza and
  preprocessing specifications of other stanazs. (ocaml/dune#2211, fixes ocaml/dune#2206,
  @rgrinberg)

- Run `refmt` from the context's root directory. This improves error messages in
  case of syntax errors. (ocaml/dune#2223, @rgrinberg)

- In .merlin files, don't pass `-dump-ast` to the `future_syntax` preprocessor.
  Merlin doesn't seem to like it when binary AST is generated by a `-pp`
  preprocessor. (ocaml/dune#2236, @aalekseyev)

- `dune install` will verify that all files mentioned in all .install files
  exist before trying to install anything. This prevents partial installation of
  packages (ocaml/dune#2230, @rgrinberg)
@rgrinberg rgrinberg mentioned this pull request Jun 4, 2019
Merged
Zimmi48 referenced this pull request in yoriyuki/Camomile Jun 4, 2019
@jeremiedimino
Switch to dune and include_subdirs
505202b 
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
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 Awaiting requested review from emillon

@rgrinberg rgrinberg Awaiting requested review from rgrinberg

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@kit-ty-kate @aalekseyev @jeremiedimino