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

Discuss Crate Features #560

Closed
phip1611 opened this issue Nov 12, 2022 · 10 comments · Fixed by #561
Closed

Discuss Crate Features #560

phip1611 opened this issue Nov 12, 2022 · 10 comments · Fixed by #561

Comments

@phip1611
Copy link
Contributor

phip1611 commented Nov 12, 2022

I personally do not like the exts feature. With a name such as alloc feature, it is more clear what one gets. With exts not. Additionally, at the moment, people do not get what they expect from alloc feature. uefi-rs doesn't follow the conventions of the Rust ecosystem.

From a short investigation, we currently use the following features:

  • alloc
    • adds a #[global_allocator] but nothing more
  • exts
    • activates the alloc crate from the standard library distribution
    • activates all functionality that uses allocations

My suggestion is:

  • rename alloc module and feature to global_allocator
    • this also allows us to get rid of the weird alloc_api rename and use alloc everywhere
  • rename feature exts to alloc (as it is the default in the ecosystem, i.e., in other popular crates)
  • update all documentation

Additionally, we should think about whether it makes sense to add a few features with a more meaningful name and let them depend on the new alloc feature. However, at a first glance, this may be overkill at the moment.

@phip1611
Copy link
Contributor Author

phip1611 commented Nov 12, 2022

What do you think about my suggestion @GabrielMajeri @nicholasbishop ?

#561

@phip1611 phip1611 mentioned this issue Nov 12, 2022
2 tasks
@phip1611 phip1611 linked a pull request Nov 12, 2022 that will close this issue
2 tasks
@phip1611 phip1611 pinned this issue Nov 12, 2022
@timrobertsdev
Copy link
Contributor

For what it's worth, I think this is a great change. In addition to bringing us in line with the greater ecosystem and being more intuitive for users, this could allow us and our users greater flexibility when developing or using the crate.

@nicholasbishop
Copy link
Member

I like this proposal. I recall being confused by what the alloc and exts features did when I first looked at this crate, and as you say the common pattern in the ecosystem is to call alloc what we call exts.

Only question I have is whether the ecosystem has a convention for what to call the global_allocator feature -- is that name common in crates that implement that sort of thing? It seems like a good name to me, just wondering if there's any prior art we can point to.

@phip1611
Copy link
Contributor Author

phip1611 commented Nov 12, 2022

is that name common in crates that implement that sort of thing?

Not that I'm aware of. I guess that the amount of publicly available implementations of #[global_allocator]s is very limited. I expect that 99% users of custom global allocators need highly application-specific stuff. Therefore, I'd surprise me if there are any conventions. Aye?

@nicholasbishop
Copy link
Member

Yep that makes sense.

@GabrielMajeri
Copy link
Collaborator

With a name such as alloc feature, it is more clear what one gets. With exts not. Additionally, at the moment, people do not get what they expect from alloc feature. uefi-rs doesn't follow the conventions of the Rust ecosystem.

There wasn't a huge amount of planning/foresight when we had defined those features, so they can definitely be improved 😅

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.

@phip1611
Copy link
Contributor Author

phip1611 commented Nov 13, 2022

There wasn't a huge amount of planning/foresight when we had defined those features, so they can definitely be improved sweat_smile

Alright. I'm looking forward to your comments on #561

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:

Hm, I do not have a strong opinion here but here's one thing. If we publish more crates in this manner, say uefi-logger, external users might think that this is a universal logger for UEFI, but in fact, it only works with the uefi-crate. If there are competing libraries for UEFI, all crate names occupied by us will be incompatible to those implementations.

However, this is a general problem of the ecosystem on crates.io. On the other hand, seems like we create/maintain the standard UEFI library and there are no serious competitors.

I have one specific thing in mind here - https://crates.io/crates/printk - its name sounds like it is a universal kernel space logger. But in fact, it is a logger that works with the bootloader crate. I'D like to refrain from bringing similar confusion into the ecosystem

@GabrielMajeri
Copy link
Collaborator

external users might think that this is a universal logger for UEFI, but in fact, it only works with the uefi-crate. If there are competing libraries for UEFI, all crate names occupied by us will be incompatible to those implementations.

Isn't the uefi-* prefix already sort-of associated with our library? I mean, we already have the uefi-macros and uefi-services crates. Furthermore, competitors also seem to do something similar: see, for example, r-efi and r-efi-string.

Funny story: we weren't, in fact, the first owners of the uefi crate name. There used to be another crate published under the name uefi on crates.io, but it was not under active development and it lacked a lot of features. I simply tracked down the former owner by e-mail, and now we "own" the uefi crate name 😄

However, this is a general problem of the ecosystem on crates.io. On the other hand, seems like we create/maintain the standard UEFI library and there are no serious competitors.

Precisely my point. There's the r-efi project mentioned above, but they have a different scope and approach.

I have one specific thing in mind here - crates.io/crates/printk - its name sounds like it is a universal kernel space logger. But in fact, it is a logger that works with the bootloader crate. I'D like to refrain from bringing similar confusion into the ecosystem

Yeah, that's definitely very misleading. But it's an innate disadvantage of the first-come, first-served naming policy followed on crates.io. One can only hope that the crate will eventually either be renamed, or expanded to support non-bootloader-based projects 😅

@phip1611
Copy link
Contributor Author

phip1611 commented Nov 14, 2022

I'd propose to move the ongoing discussion of open points to #563.

The subject of this issue is to resolve the confusion of the current naming of features. #563 discusses what functionality should live in which feature or crate.

@nicholasbishop
Copy link
Member

I do intend to add some thoughts to both these issues, but things have been busy so haven't had a chance to yet. Hopefully soon :)

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 a pull request may close this issue.

4 participants