Introduce a Rust workspace for consolidating build dependencies #196
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
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Oh, I also didn't touch the Makefile, but in theory the multi-project rules (make clean, make update, make fmt), can be simplified now |
Note that this workspace only includes the high-level (platform-agnostic?) Veracruz libraries. This specifically excludes: - rust-examples/*, which compile to WebAssembly - runtime-manager/root-enclaves, sdks which have special requirements This was a surprisingly unintrusive change, though required a few tweaks: - Fixed dependencies that disagreed, this is probably a good thing anyways - Consolidated build profiles. There are package-specific build-profiles, but the only customization we had was for profile.release, and I suspect the crates that didn't customize profile.release were just never compiled outside of debug mode. - Consolidated patches. This is probably a good thing as I suspect that the patches were required for all dependent crates, and this will reduce mistakes when adding new crates in the future.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
dominic-mulligan-arm
added
always-refactoring
|
@geky can this be closed given @mathias-arm's work? |
|
Yep, closing for now. The code/branch is still available in case it's useful |
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.
This is a bit of exploratory work to try sharing build-time dependencies by a common Rust workspace.
Note that this workspace only includes the high-level (platform-agnostic?) Veracruz libraries. This specifically excludes:
rust-examples/*, which compile to WebAssembly
runtime-manager, root-enclaves, sdks which have special requirements
This has also only been tested for SGX, but since most of the platform-specific configuration remains in the Makefile, it should just work for other platforms.
This was a surprisingly unintrusive change, though required a few tweaks:
Fixed dependencies that disagreed, this is probably a good thing anyways
Consolidated build profiles. There are package-specific build-profiles, but the only customization we had was for profile.release, and I suspect the crates that didn't customize profile.release were just never compiled outside of debug mode.
Consolidated patches. This is probably a good thing as I suspect that the patches were required for all dependent crates, and this will reduce mistakes when adding new crates in the future.
Some other thoughts:
We've discussed breaking the workspaces out into per-platform workspaces + symlinks. This doesn't seem like a strict requirement, since you can still build each package with its own feature/profile/etc flags. But it may be nice for organizational reasons, and to avoid future risks with conflicting dependencies/patches between platforms.
Though this may be something that gets cleaned up nicely by Isolate platform specific changes to their own crates #81. If each platform is its own crate/workspace, inside the high-level, common Veracruz crates/workspace, then we would get the separation of per-platform configuration without need to use symlinks. It would result in some build-dependency duplication, but only worst case 2x vs Nx.
You may think you can use the documented package shorthand in the root
cargo build --package veracruz-test --features sgx, but you can't. At least now with the --features flag. You can, however, provide --features to builds in the directory or with --manifest-pathcargo build --manifest-path veracruz-test/Cargo.toml --features sgx, which does use the workspace as any local builds search upwards for a workspace Cargo.toml. Not sure why this limitation exists.building workspaces can't use --features flag rust-lang/cargo#5015
If you went the per-platform workspace direction, you could specify these feature flags as "default-features" in the workspace.
I also just wanted to clarify that both the relative path dependencies and non-root builds do work with workspaces and do use the common target directory. Which is why this required so few changes.
I also considered moving the rust-examples into a WebAssembly-specific workspace, but didn't as I'm not sure there's that much of an advantage there. The examples don't have that many dependencies, are each intended to be standalone (different build profiles?), and adopting a slightly more complex workspace system in the examples risks confusing users who just copy-paste an example to get started.
The workspace does include some non-root crates: freestanding-execution-engine, wasm-checker, generate-policy. These did have some dependencies on the rest of Veracruz, so I wonder if the sdk/test-collateral directories are the wrong place for them and they should be moved into root.
The data-generators are included in the workspace, I don't really have any thoughts on these.