Skip to content

unicode_names2_generator as build dependency #17

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

Merged
merged 5 commits into from
Mar 3, 2023

Conversation

youknowone
Copy link

@youknowone youknowone commented Mar 2, 2023

Because generator is now deterministic, we can use it as build dependency.

And finally formatting and CI for formatting

closes #16
closes #15

@youknowone youknowone changed the title split generator binary and library unicode_names2_generator as build dependency Mar 2, 2023
@youknowone youknowone force-pushed the split-lib branch 2 times, most recently from dd4f705 to 921e151 Compare March 2, 2023 07:45
@progval
Copy link
Owner

progval commented Mar 2, 2023

Not that I'm against this change, but what does non-determinism have to do with being used as a build-dep?

@youknowone
Copy link
Author

No problem if everything is perfect.
But suppose we have glitch on hash or random function at edge case. With deterministic build, the full database can be tested and it guarantees the generated code doesn't have bug even if there is bug on hash or random.
With non-deterministic build, users randomly will get bugs by each build time in rare chance.

@progval
Copy link
Owner

progval commented Mar 2, 2023

it looks like you ran rustfmt in addition to your changes in the unicode_names2_generator as build dependency commit. Could you insert a commit with only the rustfmt changes before that commit, so the commit can be read more easily?

@progval
Copy link
Owner

progval commented Mar 3, 2023

thanks

@progval progval merged commit 5cd226f into progval:master Mar 3, 2023
@youknowone youknowone deleted the split-lib branch March 3, 2023 18:13
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