Skip to content

Conversation

@ratmice
Copy link
Collaborator

@ratmice ratmice commented Mar 14, 2025

This is just testing previous CI changes catch things, do not merge!

@ratmice
Copy link
Collaborator Author

ratmice commented Mar 14, 2025

Hmm, I'm not exactly sure whether the checks ran or not/how to find the logs from the buildbot-build-script,
IIRC they might not run until we try to merge them, IIRC It doesn't try to test rustfmt etc until merge, so perhaps this
check is the same?

Anyhow, I'm a bit uncertain how to test something we don't actually want committed :)

@ltratt
Copy link
Member

ltratt commented Mar 15, 2025

Testing this locally, alas, is a bit tricky. I suppose one could use (say) sed to inject a deliberate mistake into the code and check it's caught?

@ratmice
Copy link
Collaborator Author

ratmice commented Mar 16, 2025

Welp, I'm definitely confused, and the way I have been thinking about the cache is slightly backwards from the way it is documented to work. It also doesn't help that in the interim I stumbled upon a known cargo footgun, in emitting rerun-if-changed.

rerun-if-changed apparently disables the automatic src/*.rs behavior i.e. if you set it once, you also have to set it for the rest of the src/ dir, so now that we emit that we ensured a catch-22, the cache will never be checked because the only way to run the check is to modify the input grammar, which then causes the cache check to not be run.

So i'm hoping we can resolve:
a) remove the rerun-if-changed
b) my misunderstanding of the cache check involves the flipping of the modification times check here
e.g. to

if FileTime::from_last_modification_time(out_rs_md)
                    < FileTime::from_last_modification_time(inmd)

I.e. I thought it was 'if the input is newer than the output` but the cache is unmodified we can skip regeneration.

But the comment seems to be describing something else, where the output is newer than the input, and we may still need to regenerate. But I don't understand because the subsequent branches don't seem to be able to cause it to regenerate, only skip the regeneration.

So, I'm uncertain whether that comparison should be flipped e.g. to <, or i'm just too confused.
Sorry 😢 🤦 Now that we are getting into it, I realize that my previous understanding isn't semantically valid. e.g. the input file can change actions which means it needs to be regenerated since those aren't part of the cache.

Anyhow the following shell script does "panic" if I do a couple of things:

  • disable the emitting of rerun-if-changed
  • flip the modification time comparison

At least that makes the cache do what I thought it was supposed to.

#! /bin/sh

set -e

export RUSTFLAGS="--cfg grmtools_extra_checks"
root=`pwd`
cd $root/lrpar/examples/calc_actions
cargo build
echo "2 + 3 * 4" | cargo run | grep "Result: 14"
touch src/main.rs src/calc.y
echo "Second build"
export CACHE_EXPECTED="y"
 cargo build -vvv
unset CACHE_EXPECTED

So if my understanding is wrong we definitely seem to need to

  • remove the touch src/calc.y line
  • cause the cache to be invalidated by say modifying the lex file?

@ratmice
Copy link
Collaborator Author

ratmice commented Mar 16, 2025

So above and beyond that for some reason I don't actually see touch src/main.rs rerunning build.rs
So, I'm also confused about how to even get this executable where the cache check is to run reliably in a way which doesn't invalidate the cache (like how touching build.rs would modify the vergen timestamp would).

Anyhow I think that there are probably multiple layers to my confusion, and I wouldn't be surprised to learn that
some change in cargo has made these cache check branches more difficult to reach since this code was written.

Edit: Aha, Footgun 2x, I forgot to remove the rerun-if-changed line from lrlex too.

@ratmice ratmice mentioned this pull request Mar 16, 2025
@ratmice ratmice force-pushed the ci_testing_ignore branch from 06e96ed to a9f67f5 Compare March 16, 2025 22:05
@ratmice
Copy link
Collaborator Author

ratmice commented Mar 16, 2025

Well, I'm not sure what more I can try, I feel confident that the .buildbot.sh check should be working,
and the change in this series should be triggering it, as is the case running .buildbot.sh locally from this branch.

As such I'll probably just close this but do let me know if there are any ideas.

@ratmice ratmice closed this Mar 16, 2025
@ltratt
Copy link
Member

ltratt commented Mar 17, 2025

@ratmice I agree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants