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

print cargo::rerun-if-changed directives from cttests/build.rs #543

Merged
merged 1 commit into from
Mar 28, 2025

Conversation

ratmice
Copy link
Collaborator

@ratmice ratmice commented Mar 28, 2025

Emit cargo::rerun-if-changed from src/.rs, src/.test, storaget.l, storaget.y, storaget.l.rs storaget.y.rs.

Files that are not emitted include *.test.l, *.test.y, and their respective .l.rs and .y.rs files. This is because the .test cgen_helper don't check timestamps and would always result in spurious rebuilds.

Emit cargo::rerun-if-changed from src/*.rs, src/*.test,
storaget.l, storaget.y, storaget.l.rs storaget.y.rs.

Files that are not emitted include *.test.l, *.test.y, and their respective
.l.rs and .y.rs files. This is because the .test cgen_helper don't check
timestamps and would always result in spurious rebuilds.
@ratmice ratmice force-pushed the cttests-rerun-if-changed branch from 12385fc to 270abee Compare March 28, 2025 01:59
@ltratt
Copy link
Member

ltratt commented Mar 28, 2025

I'm not sure if this is the right tool here, but would https://crates.io/crates/rerun_except help?

@ratmice
Copy link
Collaborator Author

ratmice commented Mar 28, 2025

I'm not certain, like I'm adding storaget.l.rs, and storage.y.rs, but not any of the .y.rs and .l.rs derived from .test files.
This is slightly more precise than the normal build.rs mechanism with cargo where we don't normally also add storaget.l, and .y (by default we only detect .rs extensions). But for that file we aren't generating from a .test file, so we should be avoiding spurious regeneration through the cache mechanism.

I think we could emit better filenames, like: foo.test.l, foo.test.y, foo.test.l.rs, foo.test.y.rs, and then
rerun_except(&["*.test.l", "*.test.y", "*.test.l.rs", "*.test.y.rs"]);

That is to say that currently without the .test being added storaget.{l,y,l.rs,y.rs} would match our inverted globs we want.

@ltratt
Copy link
Member

ltratt commented Mar 28, 2025

So probably best to merge as-is?

@ratmice
Copy link
Collaborator Author

ratmice commented Mar 28, 2025

Let me try changing the extensions, and see if it helps/hurts, I think it'll likely help

@ratmice
Copy link
Collaborator Author

ratmice commented Mar 28, 2025

I think I would have said it was an improvement,

We have to change all the lrlex_mod!("calc_wasm.test.l") and lrpar_mod!("calc_wasm.test.y").
to account for the new paths, however rerun_except doesn't actually work in this case.

We end up with something such as the following, however rerun_except appears to only set matches from src/**,
where our globs are all prefixed in OUT_DIR.

    let out_dir = std::env::var("OUT_DIR").unwrap();
    rerun_except(&[
        &format!("{out_dir}/*.test.y"),
        &format!("{out_dir}/*.test.l"),
        &format!("{out_dir}/*.test.l.rs"),
        &format!("{out_dir}/*.test.y.rs"),
    ])
    .unwrap();

That is to say, that the rerun-if-changed=OUT_DIR/storaget.l, and other OUT_DIR files isn't getting emitted by this line,
it's only emitting files from the cttests/ source directory.

Let me know if you'd like me to push this (somewhere) if you want to look at it

@ltratt
Copy link
Member

ltratt commented Mar 28, 2025

Ah, yes, rerun_except will almost certainly ignore target (because of the git traversal crate it uses). I'm inclined to merge as-is, as I think rerun_except is a mild square-ish peg for this particular round hole. OK to do so?

@ratmice
Copy link
Collaborator Author

ratmice commented Mar 28, 2025

Yeah, I think so, the excessive number format! calls are kind of burning a hole in my eyes.
But that is borrow checker/ownership stuff because of closures to the ctbuilder methods,
So i'm not sure what I could do to fix that.

So I think it is okay to merge as-is, in that I'm certain it could be improved but I'm not sure what we could do about it today.

@ltratt ltratt added this pull request to the merge queue Mar 28, 2025
@ratmice
Copy link
Collaborator Author

ratmice commented Mar 28, 2025

Alright so, the next 2 things on my list are

  1. recoverer or recoverykind for yacc %grmtools section
  2. lexerkind for lex %grmtools section,

Then after that perhaps experiment with #541 at least enough to do a proof of concept and see if it is something that requires any breaking changes or might be feasibly done without (now that we use #[non_exhaustive])

Merged via the queue into softdevteam:master with commit 8a3fe4f Mar 28, 2025
2 checks passed
@ratmice ratmice deleted the cttests-rerun-if-changed branch March 28, 2025 09:43
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