Whoops, I don't think a test feature flag exists

[Pat-Lafon]

I was poking at #892 and I noticed this warning pop up. I had to double check(and I see this is a common confusion it seems https://stackoverflow.com/questions/72908829/what-is-the-difference-between-cfgtest-and-cfgfeature-test).

Given the code in question, I think it just always pulled from mod lrgrammar; without issue?

[Pat-Lafon]

It seems like this was a feature flag until it was removed in #786. This currently then fails to work because the build script expects to find the "CARGO_FEATURE_TEST" environment variable which is no longer set.

lalrpop/lalrpop/build.rs

Line 35 in f1060df

if env::var("CARGO_FEATURE_TEST").is_ok() {

Thus no lalrpop grammar gets built

[dburgener]

Thanks for the deep investigation! Took me a while to wrap my head around, but I guess the idea is that for non-test builds, we just use the bootstrapped binary, but for test, we use that bootstrapped binary to build the existing grammar and run tests against that. That seems to make sense.

So you probably just want to turn the build.rs part into gating on #[cfg(test)] instead of the feature as well, right? It does seem a little weird to me that the setup code is done unconditionally. I'm not sure any of it is needed if we're not doing the test rebuild? I'd say move that in the block while you're there unless I'm missing something.

[Pat-Lafon]

So you probably just want to turn the build.rs part into gating on #[cfg(test)] instead of the feature as well, right? It does seem a little weird to me that the setup code is done unconditionally. I'm not sure any of it is needed if we're not doing the test rebuild? I'd say move that in the block while you're there unless I'm missing something.

Yeah that all makes sense to me. Unfortunately there is a wrinkle that #[cfg(test)] doesn't work in build.rs scripts! See rust-lang/cargo#1581 and rust-lang/cargo#13294. It seems we can either add back the test feature flag I guess or follow the suggestion in issues to isolate the code you want to run in your build script(which is all of it for us) into a new crate that you use as a dev dependency.

[dburgener]

Unfortunately there is a wrinkle that #[cfg(test)] doesn't work in build.rs scripts!

Ah, that probably explains the original choice to add a test feature flag.

I think just restoring the test feature flag seems like the least bad option to me. The whole "make a new crate as a dev-dependency" is a lot of complexity.

I guess the other question is: do we really need this functionality? We check that we've updated the grammar on PRs, so we're not at risk of the latest grammar ending up untested. It's a potential trap for local iteration though, which is a downside. I'm not sure the cost of the test feature flag is enough to justify that tradeoff.

[Pat-Lafon]

I was thinking about it and it seems best to remove the special test path. It adds some extra complexity and potentially means that you are not syncing what you are testing with what you are trying to ship. More so, it's just kinda a bad user experience to have this non-standard hack... I think I've just always mindlessly used cargo test without thinking I needed additional flags

[dburgener]

Yeah, I want to give this a second read with more focus on the details, but I really like the approach you have now after a quick first pass.