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

fix: remove [loc] from rule digest #9052

Merged
merged 1 commit into from
Nov 1, 2023

Conversation

rgrinberg
Copy link
Member

@rgrinberg rgrinberg commented Oct 31, 2023

The location of rules is only used for error messages. Therefore, we don't need to include it when computing digests.

@rgrinberg rgrinberg requested a review from snowleopard October 31, 2023 05:20
@rgrinberg rgrinberg force-pushed the ps/rr/fix__remove__loc__from_rule_digest branch from 7d3e728 to ae242de Compare October 31, 2023 05:21
@rgrinberg rgrinberg added this to the 3.12.0 milestone Oct 31, 2023
@ejgallego
Copy link
Collaborator

Will that mean that if I reformat a dune file that contains errorss, the errors will display the wrong position?

[That is to say, are errors in dune files sometime memoized?]

@rgrinberg
Copy link
Member Author

There will be no such issue. The key to remember is that we don't cache errors - only successes. So if your dune file contains errors, the rules derived from it will never be produced, hence never executed, hence their digests will never be calculated.

@snowleopard
Copy link
Collaborator

snowleopard commented Oct 31, 2023

It's easy to add a test to demonstrate this property, and to make sure we don't regress in future.

Copy link
Collaborator

@snowleopard snowleopard left a comment

Choose a reason for hiding this comment

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

Looks but please update the text of the test which demonstrates the improvement. You could also add a new test there to demonstrate that failing actions do get correctly rerun and produce errors with correct locations.

@rgrinberg rgrinberg force-pushed the ps/rr/fix__remove__loc__from_rule_digest branch from ae242de to a234bfa Compare October 31, 2023 23:49
Signed-off-by: Rudi Grinberg <[email protected]>

<!-- ps-id: 5a241e7f-d2bf-4732-b4c1-ac9e7db54618 -->
@rgrinberg rgrinberg force-pushed the ps/rr/fix__remove__loc__from_rule_digest branch from a234bfa to a5285d7 Compare November 1, 2023 00:02
@rgrinberg
Copy link
Member Author

Updated the test and added a new one.

@rgrinberg rgrinberg merged commit 29b25ac into main Nov 1, 2023
@rgrinberg rgrinberg deleted the ps/rr/fix__remove__loc__from_rule_digest branch November 1, 2023 16:08
@snowleopard
Copy link
Collaborator

Updated the test and added a new one.

Awesome, thanks!

emillon added a commit to emillon/opam-repository that referenced this pull request Nov 28, 2023
CHANGES:

- Introduce `$ dune ocaml doc` to open and browse documentation. (ocaml/dune#7262, fixes
  ocaml/dune#6831, @EmileTrotignon)

- `dune cache trim` now accepts binary byte units: `KiB`, `MiB`, etc. (ocaml/dune#8618,
  @Alizter)

- No longer force colors for OCaml 4.03 and 4.04 (ocaml/dune#8778, @rgrinberg)

- Introduce new experimental odoc rules (ocaml/dune#8803, @jonjudlam)

- Introduce the `runtest_alias` field to the `cram` stanza. This allows
  removing default `runtest` alias from tests. (@rgrinberg, ocaml/dune#8887)

- Do not ignore libraries named `bigarray` when they are defined in conjunction
  with OCaml 5.0 (ocaml/dune#8902, fixes ocaml/dune#8901, @rgrinberg)

- Dependencies in the copying sandbox are now writeable (ocaml/dune#8920, @rgrinberg)

- Absent packages shouldn't prevent all rules from being loaded (ocaml/dune#8948, fixes
  ocaml/dune#8630, @rgrinberg)

- Correctly determine the stanza of menhir modules when `(include_subdirs
  qualified)` is enabled (@rgrinberg, ocaml/dune#8949, fixes ocaml/dune#7610)

- Display cache location in Dune log (ocaml/dune#8974, @nojb)

- Re-run actions whenever `(expand_aliases_in_sandbox)` changes (ocaml/dune#8990,
  @rgrinberg)

- Rules that only use internal dune actions (`write-file`, `echo`, etc.) can
  now be sandboxed. (ocaml/dune#9041, fixes ocaml/dune#8854, @rgrinberg)

- Do not re-run rules when their location changes (ocaml/dune#9052, @rgrinberg)

- Correctly ignore `bigarray` on recent version of OCaml (ocaml/dune#9076, @rgrinberg)

- Add `test_` prefix to default test name in `dune init project` (ocaml/dune#9257, fixes
  ocaml/dune#9131, @9sako6)

- Add `coqdoc_flags` field to `coq` field of `env` stanza allowing the setting
  of workspace-wide defaults for `coqdoc_flags`. (ocaml/dune#9280, fixes ocaml/dune#9139, @Alizter)

- [coq rules] Be more tolerant when coqc --print-version / --config don't work
  properly, and fallback to a reasonable default. This fixes problems when
  building Coq projects with `(stdlib no)` and likely other cases. (ocaml/dune#8966, fix
  ocaml/dune#8958, @Alizter, reported by Lasse Blaauwbroek)

- Dune will now run at a lower framerate of 15 fps rather than 60 when
  `INSIDE_EMACS`. (ocaml/dune#8812, @Alizter)

- dune-build-info: when `version=""` is found in a `META` file, we now return
  `None` as a version string (ocaml/dune#9177, @emillon)

- Dune can now be built and installed on Haiku (ocaml/dune#8795, fix ocaml/dune#8551, @Alizter)

- Mark installed directories in `dune-package` files. This fixes `(package)`
  dependencies against packages that contain such directories. (ocaml/dune#8953, fixes
  ocaml/dune#8915, @emillon)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants