-
Notifications
You must be signed in to change notification settings - Fork 38
Sort tokens in CTLexerBuilder codegen for deterministic output #576
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
Conversation
|
This looks to me like a good catch to me, I am scratching my head though wondering why |
|
Hmm, it does. There's something strange going on. Cargo doesn't re-run |
|
Alright, I'm not sure what exactly happening with rebuilds, but I've added a sort to the lexer generator as well, hope it'll reduce the number of spurious builds. |
|
I'm definitely not familiar with However I didn't see anything in vergen that seemed to indicate how to get it to output such a file, and doesn't |
|
Well, I'm happy with all the changes but I'll give some time in case Laurence has any further comments. |
|
In the end, the culprit turned out to be a mounted file system that didn't handle time stamps correctly (this was a WSL thing). So, this PR doesn't actually fix anything because there was nothing to fix except my environment. Still, it might be good to keep generated files consistent, so feel free to merge or reject this 😸 |
| // Record the time that this version of lrlex was built. If the source code changes and rustc | ||
| // forces a recompile, this will change this value, causing anything which depends on this | ||
| // build of lrlex to be recompiled too. | ||
| let timestamp = env!("VERGEN_BUILD_TIMESTAMP"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this environment variable always defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you can see it also used in the ct_token_map codegen at the top of the diff context in the first patch in this series. 719da09
I believe it is ensured by:
https://docs.rs/vergen/latest/vergen/struct.BuildBuilder.html#method.build_timestamp
https://github.com/softdevteam/grmtools/blob/master/lrlex/build.rs#L4
along with the compile time caching behavior of the env! macro:
https://www.cs.brandeis.edu/~cs146a/rust/doc-02-21-2015/std/macro.env!.html
So I think this is including a timestamp from the time when lrlex/build.rs is built to every token_map.rs,
so this addition syncs things so that applies to the main lex output as well!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, yes, I can see. @ratmice I think you're best placed to judge this PR.
|
@taminomara Even if this doesn't fix your original issue, I feel like the deterministic output is an improvement for reproducible builds. But we should probably rewrite the commit message to to reflect that, I'd suggest something to the |
lrlex/src/lib/ctbuilder.rs
Outdated
| let timestamp = env!("VERGEN_BUILD_TIMESTAMP"); | ||
| write!(outs, "// lrlex build time: {}\n\n", quote!(#timestamp),).ok(); | ||
| outs.push_str(&syn::parse_str(&unformatted) | ||
| .map(|syntax_tree| prettyplease::unparse(&syntax_tree)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this patch needs cargo fmt and fixing of cargo clippy errors before it'll pass CI.
ct_token_map
No description provided.