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: make ignored rules fallback #8706

Merged
merged 4 commits into from
Sep 21, 2023
Merged

fix: make ignored rules fallback #8706

merged 4 commits into from
Sep 21, 2023

Conversation

emillon
Copy link
Collaborator

@emillon emillon commented Sep 19, 2023

Fixes #8703

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

@emillon emillon requested a review from rgrinberg September 19, 2023 13:48
@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
Copy link
Collaborator Author

emillon commented Sep 20, 2023

I had a look at how we could extend that mechanism (turning to fallback instead of removing them) to user-provided rules but I'm not sure how it would work in practice.

Did you mean something like the following?

  • if dune-lang < 3.5, ignore "strictly promote" rules, interpret promote-until-clean as intended
  • if dune-lang >= 3.5 but < 3.11, ignore all promote rules
  • if dune-lang >= 3.11, turn all promote rules into fallback rules

@rgrinberg
Copy link
Member

To be clear, making --ignore-promoted-rules depend on the version in the dune-project is a mistake. This option should work for all rules (internal and user defined) across all versions of the dune language.

@emillon
Copy link
Collaborator Author

emillon commented Sep 20, 2023

Ok. We can remove that version check. It's likely to surface some issues in some projects, though. What do we do for 3.11? Is the fix in this PR enough, and we'll remove the version check for user rules in 3.12?

@rgrinberg
Copy link
Member

It's likely to surface some issues in some projects, though. What do we do for 3.11? Is the fix in this PR enough, and we'll remove the version check for user rules in 3.12?

I'd just do it in one go. It becomes hard to keep a map in your head of how things work between different versions of dune if we split a logical change in chunks like that.

Tbh, if this change is so painful to include in 3.11, you could always revert it in that branch and we could keep it in main until we decide to make another release.

@emillon
Copy link
Collaborator Author

emillon commented Sep 20, 2023

OK. The next round of testing will reveal what breaks. I'll do the other change in a separate PR to ease reverting.

@emillon
Copy link
Collaborator Author

emillon commented Sep 21, 2023

Other fix is in #8721

@emillon emillon mentioned this pull request Sep 21, 2023
Closed
@emillon emillon merged commit b326c30 into ocaml:main Sep 21, 2023
20 checks passed
@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)
emillon added a commit to emillon/dune that referenced this pull request Sep 21, 2023
@emillon
Fix changelog for ocaml#8706
b26a59c 
Signed-off-by: Etienne Millon <me@emillon.org>
rgrinberg
rgrinberg reviewed Sep 21, 2023
View reviewed changes
src/dune_rules/super_context.ml
Some rule.targets
let rule = make_rule t ?mode ?loc ~dir build in
let+ () = Rules.Produce.rule rule in
Some rule.targets
Copy link
Member

Choose a reason for hiding this comment

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

This function no longer needs to return option

Copy link
Member

Choose a reason for hiding this comment

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

#8725

rgrinberg added a commit to rgrinberg/jbuilder that referenced this pull request Sep 21, 2023
@rgrinberg
Revert "fix: make ignored rules fallback (ocaml#8706)"
9d7b0e2 
This reverts commit b326c30.
rgrinberg added a commit to rgrinberg/jbuilder that referenced this pull request Sep 21, 2023
@rgrinberg
Revert "fix: make ignored rules fallback (ocaml#8706)"
cb4e8b4 
This reverts commit b326c30.

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: make ignored rules fallback (ocaml#8706)"
05b694c 
This reverts commit b326c30.

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

@rgrinberg rgrinberg rgrinberg left review comments

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

Successfully merging this pull request may close these issues.

missing opam file causes build failure
2 participants
@emillon @rgrinberg