global_allocator and logger could be moved to uefi-services

[phip1611]

I think we could move the global_allocator and the logger entirely from uefi to uefi-services. This would clean up the uefi crate. And uefi-services wouldn't be as thin as it is right now anymore.

Thoughts?

[GabrielMajeri]

See also my proposal from #560 (comment), in which case we would create new crates for these two features.

[phip1611]

In fact, considering PRs like #552, it might be a good idea to go beyond renaming the features and instead switch to a much more granular project structure:

uefi could remain the core library, with a feature alloc which would define convenience methods that require dynamic allocation;

a uefi-global-allocator crate with a global allocator implementation that uses the UEFI memory allocation API;

a uefi-logger crate with a logger implementation that uses the UEFI standard output;

In this case, we won't even need a uefi-services crate anymore; or it would simply become a wrapper for the other crates.

-- @GabrielMajeri

TL;DR: I'd prefer to stick with uefi and uefi-services but move the global allocator and the logger entirely to uefi-services.

Having dedicates crates for uefi-global-allocator, uefi-logger, etc., doesn't really bring a benefit. I do not see a value in having multiple crates with very limited functionality (less than 500 lines).

I see uefi as the base abstraction and uefi-services as additional funtionality that helps to integrate an UEFI application into existing parts of the Rust ecosystem (logging, allocation, ...).

Additionally, if we have multiple crates for everything, we also need to make sure that all of them are compatible with the uefi base. That's hard to manage and to communicate (uefi-foo@0.10 is based on uefi@0.18, uefi-bar is based on uefi@0.15 etc.). This is tremendously simplified by only having uefi and uefi-services.

What are your thoughts?

[GabrielMajeri]

I do not see a value in having multiple crates with very limited functionality (less than 500 lines).

Well, we use crates like that all the time :) for example cfg-if, ucs2, etc.

I strongly believe crates should be as modular and specialized as possible -- I like the Unix philosophy of "each tool does one thing well", instead of having Swiss knives with a ton of features. And it seems there are other Rust developers which think the same.

Having dedicates crates for uefi-global-allocator, uefi-logger, etc., doesn't really bring a benefit. I do not see a value in having multiple crates with very limited functionality (less than 500 lines).

I see uefi as the base abstraction and uefi-services as additional funtionality that helps to integrate an UEFI application into existing parts of the Rust ecosystem (logging, allocation, ...).

As we've seen recently in #552, not everybody wants the "all-or-nothing" package that uefi-services currently provides. That being said, I'd love to have other uefi-rs developers/users chime in with their views on this topic 😄

Additionally, if we have multiple crates for everything, we also need to make sure that all of them are compatible with the uefi base. That's hard to manage and to communicate (uefi-foo@0.10 is based on uefi@0.18, uefi-bar is based on uefi@0.15 etc.). This is tremendously simplified by only having uefi and uefi-services.

I agree that this could be problematic, but I think there'd be one easy solution for this: switching to a major 1.x version for the core uefi-macros and uefi crates.

I'll admit that I never really had any concrete plan for such a transition, since I don't know precisely when the libraries will be "ready" enough for this 😅... but considering we didn't have any major API-breaking changes in a while, we might consider the core library to be stable.

What are your thoughts?

That we should get more feedback from more people ;)

[raccog]

I use the uefi and uefi-services crates for my boot loader. I think it would be nice to have each extra feature in separate crates.

I've been relying on uefi-services in my boot loader for stuff like logging, allocating, and panic handling, but I plan to eventually switch to my own implementations. If the extra features are split into separate crates, it would likely be easier to do this by incrementally removing each crate as I re-implement them myself.

This is just for my specific use case, though, I'd like to hear from other people on this as well. :)

[nicholasbishop]

I've been pondering this issue on and off for a while now, without coming to any particularly strong opinions. Some thoughts:

• The current API puts the implementation of Logger in uefi, and uefi-services handles the static storage, initialization, and shutdown of the logger. The implementation of the allocator is sort of similar, except the static storage is also in uefi, instead of uefi-services. It's a minor thing, but maybe worth cleaning up that difference (assuming it's not made moot by other changes).
• Is there much point to keeping the implementation of the logger/allocator separate from the storage/initialization/shutdown? I haven't come up with any very convincing reason why we should keep that separation.
• Is there an advantage to using separate crates vs using crate features? Either one allows the user to enable only the stuff they want. Using features has the advantage that the user doesn't have to carefully match up version numbers. Separate crates can be useful when the crates really are entirely separate, and the end-user might just want one or the other. So that applies to our ucs2 crate, but not so much to uefi-macros/uefi/uefi-services I think.

Again, I don't think I have a strong opinion here, but I'm mildly in favor of dropping the uefi-services crate and moving that functionality into the uefi crate. If the logger feature is enabled then a init_logger() function (or similar) would be available to initialize the logger and handle automatic shutdown when exiting boot services. If the global_alloc feature is enabled then a init_global_allocator function (or similar) would be available to handle initialization and automatic shutdown. The remaining feature is the panic handler, and that could be moved over easily too.

[phip1611]

Edit: Obsolete due to #705

Another thought: I think, we should separate the export of uefi::glboal_allocator::Allocator and setting it as #[global_allocator].

Actually, I'd prefer to remove

#[global_allocator]
static ALLOCATOR: Allocator = Allocator

entirely. This limits users. This is the responsibility of the app. For example, in one of my projects, I have a allocator facade that uses a hard code copy of the uefi allocator for that reason.

[phip1611]

After reading your thoughts, I think, I found another way to look at things:

I see the uefi crate as a collection of tools that allows competing implementations in an application for almost all aspects. uefi-services however enforces concrete policies, such as a logger (there can only be one), a global allocator (there can only be one), a panic handler (there can only be one). uefi-services is a helper for applications that provides some typical no_std application boilerplate but doesn't has much to do with uefi itself.

So I think, this is the main objective that differentiates those two crates and, with that in mind, the current separation of features doesn't make that much sense. For example, the #[global_allocator] should only be set by uefi-services, not uefi.

Therefore, I'm in favor of manifesting the just presented point of view in our project philosophy (if all agree). uefi then also doesn't need many optional crate features and just provides all basic building blocks for applications. Basic EFI applications may use uefi-services. Advanced EFI applications however will very likely choose a custom global allocator, a custom panic handler, and a custom logger for various reasons, thus, don't use uefi-services.

[phip1611]

How do we want to continue here, @nicholasbishop @GabrielMajeri

I'm in favor of deprecating uefi-services and just put everything in uefi. uefi-services is just a single lib.rs file, which doesn't justify for me having a dedicated crate. Especially as the global allocator and the logger come from uefi already....

Instead, we should provide a uefi::env module that says something like:

This is an opinionated optional module to init the environment of your EFI image with a logger and a global allocator.

[nicholasbishop]

I'm in favor of moving forward with deprecating uefi-services. Reasoning: it's a small but noticeable improvement in ergonomics for the end user; if they previously used uefi and uefi-services, now they just have one thing to depend on (and each time they upgrade the uefi dependency, they no longer have to update uefi-services in sync). The uefi-services code is tiny, so there should be no compilation-time impact from always including it, and the new API within uefi would be contained to a single new module.

Some bikeshedable notes on specifics:

• Add a new module to uefi to contain this code. I think uefi::env is fine, although there could be a slight bit of confusion since the API in uefi::env would have no relation to std::env.
• Add panic_handler feature to uefi, and move the whole panic_handler function over to uefi.
• We probably don't want to add new init_logger and init_alloc features to uefi, so instead we could make the uefi_services::init API a little more fine-grained. E.g. uefi::env::init(InitOpts { logger: true, allocator: true }) or something like that.
• Move the print and println functions over more or less unchanged.
• This would be a good opportunity to decide on the resolution for Fix uefi_services memory unsafety if application exits before exiting boot services #943. Potentially that behavior could be configured with a field in InitOpts as well?

[phip1611]

Got it. I think uefi::helpers would be a better name than.

[GabrielMajeri]

Agreed with everything that @nicholasbishop said above.

I'm not so sure about the last part with fixing #943 at the same time. It might be easier to first perform this refactoring/reorganization of the crate and then fix that as well.

[nicholasbishop]

Thanks for pushing this along and implementing the change @phip1611 :)