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

Combine first-party Rust builds #428

Merged
merged 2 commits into from
Oct 17, 2019
Merged

Combine first-party Rust builds #428

merged 2 commits into from
Oct 17, 2019

Conversation

iliana
Copy link
Contributor

@iliana iliana commented Oct 17, 2019

Issue #, if available: fixes #291

Description of changes:
Combines all of our first-party Rust builds into a single RPM.

Running cargo install in a loop recompiles every dependency every time because cargo install is not the right tool for this use case. Instead, we're building all the binary targets in workspaces/ at the same time, so we can compile each dependency once.

This lists all the crate names we want to build. Other options included --bins (which does not work with --exclude) or --all --exclude, but since you need to copy the binary in %install anyway it seemed reasonable to list all the crates to build.

This also makes sd_notify the default feature because cargo has strange behavior when trying to enable feature flags while building a workspace, and the discussion appears to be gridlocked between "fix it" and "don't break people", so I kind of just want to steer clear of that entirely.

Timing tests:

Tests performed on an m5.metal, ymmv (but this should be a clear enough improvement to everyone).

Fresh compile (run cargo make clean && cargo make fetch, then time cargo make world):

  • current develop: 45m40s
  • this branch: 35m1s (10m39s improvement)

Incremental apiserver build (run touch workspaces/api/apiserver/src/bin/apiserver.rs, then time cargo make world):

  • current develop: 10m23s
  • this branch: 4m27s (5m56s improvement)

Launch tests: An AMI built with these changes boots and joins my cluster.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Signed-off-by: iliana destroyer of worlds <iweller@amazon.com>
@@ -19,7 +19,7 @@ maplit = "1.0.1"
olpc-cjson = { path = "../updater/olpc-cjson" }
pem = "0.6.0"
rayon = "1.2"
reqwest = "0.9.20"
reqwest = { version = "0.9.20", features = ["rustls-tls"], default-features = false }
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this related to the other changes in the PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this failed when I was using --bins, but it's not wrong for us to be using rustls here when we're using it everywhere else.

packages/workspaces/workspaces.spec Show resolved Hide resolved
Copy link
Contributor

@sam-aws sam-aws left a comment

Choose a reason for hiding this comment

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

🤘

Co-Authored-By: Tom Kirchner <tjkirch@users.noreply.github.com>
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.

Improve Rust workspace build performance
5 participants