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.

Sign up for GitHub

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

Jump to bottom

Add bios and uefi cargo features (closes #287) #304

Merged
merged 9 commits into from
Jan 9, 2023
Merged

Add bios and uefi cargo features (closes #287) #304

merged 9 commits into from
Jan 9, 2023

Conversation

AlexJMohr
Copy link
Contributor

@AlexJMohr AlexJMohr commented Jan 2, 2023 •
edited
Loading

closes #287

  • Added bios and uefi features in Cargo.toml (both enabled by default)
  • Made mbrman and gpt dependencies optional, enabled by the bios and uefi features respectively
  • Moved BiosBoot to new src/bios/mod.rs
  • Moved mbr.rs to src/bios since it is only used in bios boot
  • Moved UefiBoot to new src/uefi/mod.rs
  • Moved gpt.rs and pxe.rs to src/uefi since they are only used in uefi boot
  • publicly export BiosBoot and UefiBoot in lib.rs, gated by the bios and uefi features respectively
  • Update build.rs to use the new features to conditionally compile bios or uefi related crates

@AlexJMohr
Add bios and uefi features
2e4160e 
Both features are enabled by default. mbrman and gpt are now optional,
and are pulled in by the bios and uefi features respectively.
@AlexJMohr
move bios and uefi specific code into submodules gated by features
eb9081e
@AlexJMohr
merge BOOTLOADER_*_VERSION contants into BOOTLOADER_VERSION
c639622 
They all had the same value anway
phil-opp
phil-opp reviewed Jan 2, 2023
View reviewed changes
build.rs Outdated Show resolved Hide resolved
@phil-opp
Copy link
Member

phil-opp commented Jan 2, 2023

Thanks a lot! I only took a quick look, but what I saw looked good. I try to do a full review in the next few days.

@jasoncouture jasoncouture mentioned this pull request Jan 4, 2023
Merged
@jasoncouture
Copy link
Contributor

I have an API change that will conflict with this (Two actually)

The newest refactors the separate BiosBoot/UefiBoot into a single DiskImageBuilder struct/impl.

The purpose of this upcoming change will be to support loading arbitrary files, and a text configuration file.

@AlexJMohr would you mind if I add these changes to that API change, since I'll be breaking things anyway?

@jasoncouture
Copy link
Contributor

Here's the API change that I can add this to: 154dbab

This was referenced Jan 5, 2023
Open
Closed
@AlexJMohr
Copy link
Contributor Author

@AlexJMohr would you mind if I add these changes to that API change, since I'll be breaking things anyway?

If it were up to me I'd say merge this PR first, then integrate the changes into your branch. But it's more up to @phil-opp on how to handle that.

phil-opp
phil-opp approved these changes Jan 5, 2023
View reviewed changes
Copy link
Member

@phil-opp phil-opp left a comment

Choose a reason for hiding this comment

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

Looks good, thanks a lot!

@phil-opp
Copy link
Member

phil-opp commented Jan 5, 2023

I pushed two more commits to allow running our test framework only for BIOS or UEFI and for testing the feature gates on the CI.

@phil-opp
Copy link
Member

phil-opp commented Jan 5, 2023

Regarding the conflicts with #302 and #313: We will have some conflicts either way and I don't think that one direction is significantly harder than the other. So I don't see any reason to block this PR, which is otherwise ready.

@jasoncouture I'm happy to help with resolving conflicts in #302!

@phil-opp phil-opp added the enhancement New feature or request label Jan 5, 2023
Freax13
Freax13 requested changes Jan 5, 2023
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
@phil-opp phil-opp disabled auto-merge January 5, 2023 11:56
@phil-opp phil-opp mentioned this pull request Jan 5, 2023
Merged
3 tasks
@phil-opp phil-opp merged commit 5e42ad0 into rust-osdev:main Jan 9, 2023
jasoncouture added a commit to jasoncouture/bootloader that referenced this pull request Jan 9, 2023
@jasoncouture
Revert "Merge pull request rust-osdev#304 from AlexJMohr/cargo-features"
3f4f594 
This reverts commit 5e42ad0, reversing
changes made to 94c71d5.
@phil-opp phil-opp mentioned this pull request Mar 12, 2023
Merged
phil-opp added a commit that referenced this pull request Dec 27, 2023
@phil-opp
Fix: Enable test runner again
7b018e2 
The test runner was accidentally disabled in #351, in an attempt to fix the publish errors introduced by #304 (caused by a bug in cargo: rust-lang/cargo#12225). As a result, the test runner became a no-op as neither the bios nor the uefi features were enabled.

This commit fixes the issue by enabling both features by default. Once the cargo bug is fixed, we might want to switch back to the feature configuration added of #304.

Fixes #405
@phil-opp phil-opp mentioned this pull request Dec 27, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phil-opp phil-opp phil-opp approved these changes

@Freax13 Freax13 Freax13 requested changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add cargo features to build only UEFI or BIOS bootloader
4 participants
@AlexJMohr @phil-opp @jasoncouture @Freax13