Issue with (mode promote) in rule-generation and CI interaction

[mbarbin]

Hello,

I'm using dune version 3.14.2. I recently tried to switch to using (mode promote) for rule-generation in a project PR.

I am following the migration between 2 patterns described here.

That is, making use of (mode promote) to avoid the extra promotion step when the generator is modified.

See details

Before:

(include dune.inc)

(rule
 (deps (source_tree .))
 (with-stdout-to
  dune.inc.gen
  (run gen/gen.exe)))

(rule
 (alias runtest)
 (action
  (diff dune.inc dune.inc.gen)))

After:

(include dune.inc)

(rule
 (mode promote)
 (alias runtest)
 (deps (source_tree .))
 (with-stdout-to
  dune.inc
  (run gen/gen.exe)))

The diff is:

------ /tmp/before
++++++ /tmp/after
@|-1,12 +1,9 ============================================================
 |(include dune.inc)
 |
 |(rule
+| (mode promote)
+| (alias runtest)
 | (deps (source_tree .))
 | (with-stdout-to
-|  dune.inc.gen
+|  dune.inc
 |  (run gen/gen.exe)))
-|
-|(rule
-| (alias runtest)
-| (action
-|  (diff dune.inc dune.inc.gen)))

Issue (cons of new pattern)

I've noticed that when using the new pattern with (mode promote), the dune runtest command succeeds in the CI even if there are uncommitted changes in the tree at the end. This is different from the previous behavior where the CI would fail if dune.inc wasn't stable at a given revision.

Possible Solution?

I wonder if it would make sense for the dune runtest to still do the promotion in (mode promote), but return an exit code of [1] when it had to promote changes (as opposed to [0] when it did not).

I'm usually using the following in CI's GitHub actions workflow's configs:

      - name: Run tests
        run: opam exec -- dune runtest

I'm curious if others have run into this and how they handle it. Any insights would be appreciated.

Thank you!

[emillon]

I think (mode promote) is working as intended here: it's specifically about propagating promotions to the target automatically. If you want the extra check, you can use the 2-step generate and diff pattern that you use. If manual promotions are a problem when developing, you can try passing --auto-promote which will make it act similarly to when (mode promote) is used. Or you can use (dynamic_include), which will make this unnecesary, but you'll have to restructure your rules.

[mbarbin]

Thank you for your response, @emillon !

I understand what you are saying about (mode promote) working as intended. My issue is simply with how it interacts with my CI process. I would think that having the CI accept PRs that introduce unstable rule generation is not an intended consequence of switching to (mode promote) in the first place, so I was hoping that you would acknowledge the issue (I may be confused).

When in dev mode, I think one would likely use -w so there is no issue, as the rule generation becomes stable as soon as it has run once.

I've been considering if there's a common GitHub Actions CI to run tests that people are using that checks that by the end, there are no uncommitted changes. If so, I would switch to that, and have no issue with (mode promote).

However, if the recommended GitHub Actions is simply to run dune runtest, I would advise adding to the Dune documentation that describes the pattern 2, a note mentioning this potentially undesirable behavior change of the CI on adoption.

Perhaps the end check can be done using the git diff --exit-code command as an extra step in the CI, to return an exit code of 1 if there are uncommitted changes. Here's an example:

    - name: Check for uncommitted changes
      run: git diff --exit-code

I plan on experimenting with this, and will let you know how it goes.

[mbarbin]

Result from the experimentation in this PR

I switched the CI to include the git diff --exit-code extra step:

    - name: Run tests
       run: |
         opam exec -- dune runtest
         git diff --exit-code

and confirmed by comparing two commits in the history, including one that changed the rules. With the extra step, the invalid commit is rejected.

I will consider switching to this CI pattern more globally in my repos. My advice is that this may be worth mentioning to the dune doc in the section about (mode promote).

I will await to see if you want to bring something to my attention regarding this topic, in case I am confused by something or if there's something else you'd want to advice, otherwise I'm happy to close this issue. Thanks for your help!

[mbarbin]

I will consider switching to this CI pattern more globally in my repos.

I wanted to report that I enabled a similar check for rejecting uncommitted changes as part of the CI check in all my repos on GitHub and have been running with it for a while now. I am happy with it.

My advice is that this may be worth mentioning to the dune doc in the section about (mode promote).

I'll leave up to you to decide whether you think my experience can benefit others. I'm happy with (mode promote) as it is. Thank you!