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

Don't generate tables in the build.rs (do so in advance). #16

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thomcc
Copy link

@thomcc thomcc commented Jul 7, 2020

regex is a pretty slow dependency to build (well, regex-syntax is anyway), and it's pretty unnecessary for this crate to generate the table each time it builds.

I did this by just moving your build.rs to its own crate, which dumps the table to stdout (because it's easy, flexible, but mostly harder to mess up than taking a file argument. Although if you'd rather I do that, I can).

For clarity, cargo run -p print-table > src/case_folding_data.rs can update the pre-generated table.

Oh, also I updated you to unicode 13.0.0. I did it just because seems like good manners to do so when I'm updating the tables anyway, but I can undo it if you'd prefer.

Cargo.toml Show resolved Hide resolved
@thomcc
Copy link
Author

thomcc commented Jul 7, 2020

Ah, there's already a PR for unicode version bump in #14. I'll drop that commit from mine then.

@thomcc thomcc changed the title Don't generate tables in the build.rs, and bump unicode version. Don't generate tables in the build.rs (do so in advance). Jul 7, 2020
@thomcc
Copy link
Author

thomcc commented Jul 14, 2020

r? @SimonSapin (gentle nudge)

@crazymerlyn
Copy link

Found this when investigating compile time for RustPython. Looks like SimonSapin has transferred the project to unicode-rs. @Manishearth, does this change seem reasonable to you?

Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine. Could you also add CI that checks the codegen is up to date?

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.

3 participants