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

Only include dependencies when binary feature is enabled #68

Merged
merged 4 commits into from
Jul 25, 2019

Conversation

phil-opp
Copy link
Member

@phil-opp phil-opp commented Jul 18, 2019

This change vastly improves compile times when only lib.rs is compiled because no dependencies need to be compiled in this case. See rust-lang/cargo#1982 (comment) for more information about the approach.

This is a breaking change since a build of the executable without the binary feature will result in an error.

Depends on rust-osdev/bootimage#43, so we will need to increase the minimally supported bootimage version. To avoid public breakage, we should probably wait a bit after releasing the new bootimage version to give people time to upgrade.

This change vastly improves compile times when only `lib.rs` is compiled. However, it is a breaking change.
@phil-opp phil-opp requested a review from 64 July 18, 2019 09:47
This results in a much better error message when a build without that feature is attempted.
@phil-opp
Copy link
Member Author

phil-opp commented Jul 18, 2019

To try this change today:

  • Install bootimage 0.7.6 or later.

  • Update the bootloader dependency in your Cargo.toml:

    [dependencies]
    bootloader = { git = "https://github.com/rust-osdev/bootloader.git", branch="binary-feature", features = ["map_physical_memory"]}

Now building the bootloader library should no longer require compiling the bootloader dependencies:

>  cargo xbuild
   […sysroot build…]

   Compiling llvm-tools v0.1.1
   Compiling bootloader v0.6.4 (https://github.com/rust-osdev/bootloader.git?branch=binary-feature#aa2bbc14)
   Compiling blog_os v0.1.0 (/home/philipp/Documents/blog_os/code)
    Finished dev [unoptimized + debuginfo] target(s) in 1.36s

Copy link
Contributor

@64 64 left a comment

Choose a reason for hiding this comment

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

LGTM

@64
Copy link
Contributor

64 commented Jul 18, 2019

Actually there is one more thing - do we need the build script when the binary feature is disabled? We are still pulling in llvm-tools twice

@phil-opp
Copy link
Member Author

Good point! I disabled the build script and the llvm-tools dependency too now

@phil-opp
Copy link
Member Author

I think it's fine to merge this now. We will need to release a new semver-breaking version anyway, so no breakage should occur unless you manually increase the bootloader version in your Cargo.toml.

@phil-opp phil-opp merged commit 6be12a8 into master Jul 25, 2019
@bors bors bot deleted the binary-feature branch July 25, 2019 14:07
phil-opp added a commit that referenced this pull request Jul 25, 2019
@phil-opp
Copy link
Member Author

Published as version 0.7.0

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.

None yet

2 participants