Move cdp build step into an integration test #80
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
CI is suffering from the yanked futures crate here, maybe review/merge #79 first? |
|
good call, this is certainly reasonable given that it takes quite a bit of time..
I agree that this is negligible and am in favor of merging this. something's off in the CI with the futures dependency, maybe that version got yanked? |
In the normal Rust module system, a module is represented in the
filesystem by a file of the same name. With the previous setup,
the generated code was saved in `<target_mod>.rs`, with that
file containing another `pub mod <target_mod> { .. }` inside.
Here we remove the outer containing module from the generated code
and instead add it in the cdp crate root, where the generated code
is included, to keep the same paths.
Replace the build script with an integration test that fails if the generated code is out of date. This has several advantages: * No need for downstream users to compile the build script * No need for downstream users to compile the pdl crate * No need for downstream users to run the code generation step * The generated code is more usable in an IDE * CI will fail if the generated code is out of date The main downside is probably that the generated code is kept in the repository, but this seems of relatively minimal impact, especially if the code generator/source files don't change often.
|
Thanks for the review! If you could publish a fresh release, that would be great. |
|
just published 0.3.4 release |
|
Awesome, thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I noticed that chromiumoxide is one of the slowest crates to compile in our dependency graph. This is mainly due to the amount of generated code, but the process by which the generated code is included can also be changed to limit impact on downstream compile times; this is a first step towards improving the situation.
Replace the build script with an integration test that fails if the
generated code is out of date. This has several advantages:
The main downside is probably that the generated code is kept in
the repository, but this seems of relatively minimal impact,
especially if the code generator/source files don't change often.