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: --ignore-promoted-rules should work on internal rules #8518

Merged
merged 1 commit into from
Aug 28, 2023
Merged

fix: --ignore-promoted-rules should work on internal rules #8518

merged 1 commit into from
Aug 28, 2023

Conversation

rgrinberg
Copy link
Member

Internal promotion rules such as generating opam files weren't being
ignored under --ignored-promoted-rules.

Signed-off-by: Rudi Grinberg me@rgrinberg.com

@rgrinberg rgrinberg added this to the 3.11.0 milestone Aug 26, 2023
@rgrinberg rgrinberg linked an issue Aug 26, 2023 that may be closed by this pull request
Dune sometimes changes *.opam files in release mode #8417
Open
@rgrinberg rgrinberg force-pushed the ps/rr/fix____ignore_promoted_rules_should_work_on_internal_rules branch 2 times, most recently from 9c2705c to 248bc99 Compare August 26, 2023 20:09
@rgrinberg rgrinberg force-pushed the ps/rr/fix____ignore_promoted_rules_should_work_on_internal_rules branch 2 times, most recently from a7280fc to f9709ed Compare August 26, 2023 21:33
Leonidas-from-XIV
Leonidas-from-XIV reviewed Aug 28, 2023
View reviewed changes
Copy link
Collaborator

@Leonidas-from-XIV Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

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

The fix is fairly straightforward and the code is actually sort of simpler than before which is nice but I would like to understand the until_clean setting. Or making the API a bit more self-descriptive.

src/dune_rules/rule_mode_decoder.mli Show resolved Hide resolved
@rgrinberg rgrinberg force-pushed the ps/rr/fix____ignore_promoted_rules_should_work_on_internal_rules branch from f9709ed to b6ba92a Compare August 28, 2023 13:54
@rgrinberg
Copy link
Member Author

Added a description. Let me know what you think of it.

Leonidas-from-XIV
Leonidas-from-XIV approved these changes Aug 28, 2023
View reviewed changes
src/dune_rules/rule_mode_decoder.mli Outdated

(** [is_ignored mode ~until_clean] will return if rule with [mode] should be
ignored. [until_clean] is used to set if [(promote (until-clean))] is
ignored as considerd by this function. Old versions of dune would
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is a typo here, but asides from this this makes it much easier to understand how this parameter should be used. Thanks.

(This might possibly be a case where a default argument could make sense, since in new code we don't want to use `Ignore I assume?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually think we should consider just removing the old behavior and making the breaking change. The reason is that preserving the old behavior makes it impossible to build old projects without modifying the source directory. This is quite a pain for those packaging software that uses dune.

Copy link
Collaborator

@Alizter Alizter Aug 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I vaguely remember a plan to deprecate the other promote modes, would it be possible to interpret them as (mode promote) from now on? (With a warning)

Copy link
Member Author

Choose a reason for hiding this comment

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

The only one that is deprecated is patch_back_source_tree

Copy link
Collaborator

Choose a reason for hiding this comment

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

@rgrinberg You mean, without patching the dune-lang version to whatever will include this change? Maybe worth asking @glondu if this would be helpful enough to break compatibility.

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean, without patching the dune-lang version to whatever will include this change?

Yes. Especially given that updating the dune version in the project file might run into other errors, patching isn't a reasonable workaround.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You might be right, as probably nobody depends on the existing incorrect behaviour.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, they do 😂

In any case, we can just have them release a new version and then do this change.

@rgrinberg
fix: --ignore-promoted-rules should work on internal rules
7f6e38b 
Internal promotion rules such as generating opam files weren't being
ignored under --ignored-promoted-rules.

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

<!-- ps-id: af82808a-164d-4f37-aefc-155d2b6b4eca -->
@rgrinberg rgrinberg force-pushed the ps/rr/fix____ignore_promoted_rules_should_work_on_internal_rules branch from b6ba92a to 7f6e38b Compare August 28, 2023 14:21
@rgrinberg rgrinberg merged commit 853490b into main Aug 28, 2023
@rgrinberg rgrinberg deleted the ps/rr/fix____ignore_promoted_rules_should_work_on_internal_rules branch August 28, 2023 17:00
@emillon emillon mentioned this pull request Sep 14, 2023
Closed
emillon added a commit to emillon/opam-repository that referenced this pull request Sep 14, 2023
@emillon
[new release] dune (15 packages) (3.11.0~alpha1)
d0b9c61 
CHANGES:

- Modules that were declared in `(modules_without_implementation)`,
  `(private_modules)` or `(virtual_modules)` but not declared in `(modules)`
  will raise an error. (ocaml/dune#7674, @Alizter)

- `enabled_if` now supports `arch_sixtyfour` variable (ocaml/dune#8023, fixes ocaml/dune#7997,
  @Alizter)- Use `posix_spawn` instead of `fork` on MacOS. This gives us a
  performance boost and allows us to re-enable thread. (ocaml/dune#8090, @rgrinberg)

- Experimental: Added a `$ dune monitor` command that can connect to a running
  `dune build` in watch mode and display the errors and progress. (ocaml/dune#8152,
  @Alizter)

- No longer emit linkopts(javascript) in META files (ocaml/dune#8168, @hhugo)

- The `progress` RPC procedure now has an extra field for the `In_progress`
  constructor for the number of failed jobs. (ocaml/dune#8212, @Alizter)

- Add a `--preview` flag to `dune fmt` which causes it to print out the changes
  it would make without applying them (ocaml/dune#8289, @gridbugs)

- Introduce `(source_trees ..)` to the install stanza to allow installing
  entire source trees. (ocaml/dune#8349, @rgrinberg)

- Deprecate install destination paths beginning with ".." to prevent packages
  escaping their designated installation directories. (ocaml/dune#8350, @gridbugs)

- Stop signing source files with substitutions. Sign only binaries instead
  (ocaml/dune#8361, fixes ocaml/dune#8360, @anmonteiro)

- Add `--stop-on-first-error` option to `dune build` which will terminate the
  build when the first error is encountered. (ocaml/dune#8400, @pmwhite and @Alizter)-
  Dune now displays the number of errors when waiting for changes in watch
  mode. (ocaml/dune#8408, fixes ocaml/dune#6889, @Alizter)

- Add `with_prefix` keyword for changing the prefix of the destination of
  installed files matched by globs. (ocaml/dune#8416, @gridbugs)

- Added experimental `--display tui` option for Dune that opens an interactive
  Terminal User Interface (TUI) when Dune is running. Press '?' to open up a
  help screen when running for more information. (ocaml/dune#8429, @Alizter and
  @rgrinberg)

- Add a `warnings` field to `dune-project` files as a unified mechanism to
  enable or disable dune warnings (@rgrinberg, 8448)

- `dune exec`: support syntax like `%{bin:program}`. This can appear anywhere
  in the command line, so things like `dune exec time %{bin:program}` now work.
  (ocaml/dune#6035, ocaml/dune#8474, fixes ocaml/dune#2691, @emillon, @Leonidas-from-XIV)

- RPC message styles are now serialised meaning that RPC diagnostics keep their
  Ansi styling. (ocaml/dune#8516, fixes ocaml/dune#6921, @Alizter)- Ignore internal promote rules
  when `--ignore-promoted-rules` is set (ocaml/dune#8518, fix ocaml/dune#8417, @rgrinberg)

- Truncate output from actions that produce too much output (@tov, ocaml/dune#8351)

- Allow libraries to shadow OCaml builtin libraries. Previously, builtin
  libraries would always take precedence. (@rgrinberg, ocaml/dune#8558)

- Remove warning against `.dune` files generated by pre dune 2.0 (ocaml/dune#8611,
  @rgrinberg)

- `dune utop` no longer links `utop` in "custom" mode, which should make this
  command considerably faster. (ocaml/dune#8631, fixes ocaml/dune#6894, @nojb)

- Ensure that package names in `dune-project` are valid opam package
  names. (ocaml/dune#8331, @emillon)

- dune init: parse `--public` as a public name (ocaml/dune#8603, fixes ocaml/dune#7108, @emillon)

- Remove versions 0.1 and 0.2 of the experimental ctypes extension. (ocaml/dune#8293,
  @emillon)
This was referenced Sep 15, 2023
Merged
Merged
emillon added a commit to emillon/dune that referenced this pull request Sep 19, 2023
@emillon
fix: make ignored rules fallback
68546ca 
Fixes ocaml#8703

This is a lighter change than the original in ocaml#8518.

Signed-off-by: Etienne Millon <me@emillon.org>
emillon added a commit to emillon/dune that referenced this pull request Sep 20, 2023
@emillon
fix: make ignored rules fallback
e1f578a 
Fixes ocaml#8703

This is a lighter change than the original in ocaml#8518.

Signed-off-by: Etienne Millon <me@emillon.org>
emillon added a commit that referenced this pull request Sep 21, 2023
@emillon
fix: make ignored rules fallback (#8706)
b326c30 
Fixes #8703

This is a lighter change than the original in #8518.

Signed-off-by: Etienne Millon <me@emillon.org>
@emillon emillon mentioned this pull request Sep 21, 2023
Closed
emillon added a commit to emillon/opam-repository that referenced this pull request Sep 21, 2023
@emillon
[new release] dune (15 packages) (3.11.0~alpha3)
b8446d7 
CHANGES:

- Turn internal promote rules into fallback rules when
  `--ignore-promoted-rules` is set (ocaml/dune#8518, ocaml/dune#8706, fix ocaml/dune#8417, fix ocaml/dune#8703,
  @rgrinberg, @emillon)

- Make copy sandbox support directory targets. (ocaml/dune#8705, fixes ocaml/dune#7724, @emillon)
rgrinberg added a commit to rgrinberg/jbuilder that referenced this pull request Sep 21, 2023
@rgrinberg
Revert "fix: --ignore-promoted-rules should work on internal rules (o…
b0328da 
…caml#8518)"

This reverts commit 853490b.
@emillon emillon mentioned this pull request Sep 21, 2023
Closed
rgrinberg added a commit to rgrinberg/jbuilder that referenced this pull request Sep 21, 2023
@rgrinberg
Revert "fix: --ignore-promoted-rules should work on internal rules (o…
fa3ade4 
…caml#8518)"

This reverts commit 853490b.

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
rgrinberg added a commit to rgrinberg/jbuilder that referenced this pull request Sep 21, 2023
@rgrinberg
Revert "fix: --ignore-promoted-rules should work on internal rules (o…
216c424 
…caml#8518)"

This reverts commit 853490b.

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
rgrinberg added a commit that referenced this pull request Sep 21, 2023
@rgrinberg
Revert "fix: make ignored rules fallback (#8706)" (#8726)
2067f71 
* refactor: simplify left over type signature

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

* Revert "refactor: simplify left over type signature"

This reverts commit ec929ce.

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

* Revert "fix: make ignored rules fallback (#8706)"

This reverts commit b326c30.

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

* Revert "fix: --ignore-promoted-rules should work on internal rules (#8518)"

This reverts commit 853490b.

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

* test: promote

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

---------

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Alizter Alizter Alizter left review comments

@Leonidas-from-XIV Leonidas-from-XIV Leonidas-from-XIV approved these changes

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

Successfully merging this pull request may close these issues.

Dune sometimes changes *.opam files in release mode
3 participants
@rgrinberg @Leonidas-from-XIV @Alizter