Getting r-efi ready for use in Rust std #33

Ayush1325 · 2022-06-22T14:12:43Z

This PR includes the following changes:

Add support to build as an std dependency.
Decouple function signatures from structures: This allows using the same function signatures in r-efi dependencies instead of redefining them downstream.
Add mod.rs files to the modules: Not a big change but makes things a bit neater.
Make vendor module optional: Since stuff inside the vendor module is not always going to be available, it should be optional.

Signed-off-by: Ayush Singh ayushsingh1325@gmail.com

Just some cargo setup required for std dependencies. Signed-off-by: Ayush Singh <ayushsingh1325@gmail.com>

This allows using the same function signatures in r-efi dependencies without having to maintain two copies of it. Will also be helpful when we migrate to using "efiapi" completely. Signed-off-by: Ayush Singh <ayushsingh1325@gmail.com>

Added mod.rs files in module folders. Signed-off-by: Ayush Singh <ayushsingh1325@gmail.com>

vendor module should be optional Signed-off-by: Ayush Singh <ayushsingh1325@gmail.com>

dvdhrm

Thanks for the changes! I pulled the std-integration of Cargo.toml (plus some minor adjustments to the comments). I have posted comments on the other 3 commits.

dvdhrm · 2022-06-23T11:02:04Z

src/signatures.rs

@@ -0,0 +1,988 @@
+pub mod system {


Any particular reason to put all these into a separate signatures module? Why not keep them close by to their definitions? That is, just move each one above the respective pub struct Protocol {? This would keep the code-structure closer to the specification and thus make it easier to review new additions or audit existing protocols.

We generally try to stay as close to the structure of the specification as possible. So I was just wondering what your rationale was.

Well, there was not any big reason to keep signatures in a separate module. I just thought that I should probably not clutter other namespaces in the generated docs, especially the system namespace.

The spec defines those signatures as separate types, so it would not clutter anything. I think it would be perfectly reasonable to extract them as you did and keep them close to their respective protocol.

If you prefer, I can move them myself. I don't mind either way! Let me know.

@dvdhrm Sure, if you don't mind then feel free to move them to whichever place makes the most sense to you.

I am a bit preoccupied with getting UEFI std to build again since one of the Rust recent commits (like 2 weeks old at this point) breaks all the restricted_std targets.

dvdhrm · 2022-06-23T11:09:46Z

src/vendor/mod.rs

-pub mod intel {
-    pub mod console_control;
-}
+pub mod intel;


Can you explain the rationale behind build: fix project structure? What is the advantage over the existing model? Is this driven by some style guideline? I just wonder what triggered you to change it, or why you prefer one over the other.

The most common project structure for Rust projects uses mod.rs instead of defining all the submodules in the same file. The benefits of this mostly come up when the module becomes big. Currently, since there is a single module, it is not all that useful of a change.

However, once more vendor-specific APIs are added, it should make module management easier.

Also, it helps with conditional compilation if in the future we want to make features more explicit. (example feature intel)

As far as I understand, there are a lot of vendor-specific extensions in UEFI and thus the possibility of vendor module growing is high if Rust std works under UEFI.

Yeah, I understand. That's why I expect contributors to do this conversion whenever they happen to add more sub-modules to those respective modules. But until then, I always feel like it makes it much harder to get good overviews of r-efi if every single module is moved into its own respective subdirectory.

@dvdhrm Ok, we can leave it be until then.

dvdhrm · 2022-06-23T11:11:49Z

Cargo.toml

@@ -37,6 +37,7 @@ compiler_builtins = { version = '0.1.0', optional = true }
 # examples from normal runs.
 examples = []
 rustc-dep-of-std = ['core', 'compiler_builtins/rustc-dep-of-std']
+vendor = []


Same as with the other commit: Can you explain the rationale behind it?

Also note that this breaks API, so we would have to bump the major version again. I would thus try to delay this, if we happen to agree on merging it.

Since the modules in the vendor are specific to the vendor and might not be present in all cases, I think it would be better to make their usage explicit.

I will also need to make a few other breaking changes in the future. For example, converting the function pointers to Option<fn ptr> instead. This is because the function pointers cannot be checked for null like regular pointers under normal cases. Thus all function pointers not present in the EFI 1.0 spec need to be Options to make it possible to check if they are present in the target.

The vendor change is not necessary for me right now. But if the Rust std does reach a stable state under UEFI, a lot more vendor-specific modules might be added in this crate, so it can be delayed until then.

Also, if the vendor modules end up relying on some external dependency, that can also be helped with conditional compilation.

I think I agree that vendor should be guarded. I never really found a good way to deal with vendor, anyway. Guarding it would reduce compile-times significantly, once lots of vendor modules are added. Mhhh, I need to think about this a bit.

dvdhrm · 2022-06-30T09:30:17Z

The std-setup part is merged, and the signatures are now extracted and exported. I will close this PR now. I you still want the guarded-vendoring or the changed module structure, can you proposed that in separate PRs so we can track and discuss it?

Thanks for the patches!

Ayush1325 · 2022-07-01T20:28:34Z

The std-setup part is merged, and the signatures are now extracted and exported. I will close this PR now. I you still want the guarded-vendoring or the changed module structure, can you proposed that in separate PRs so we can track and discuss it?

Maybe we should open an issue for discussion (Titled PROPOSAL or RFC)?

Ayush1325 added 4 commits June 22, 2022 19:33

build: setup for std dependency

3220350

Just some cargo setup required for std dependencies. Signed-off-by: Ayush Singh <ayushsingh1325@gmail.com>

Decoupled function signatures from structures

a5ea2ef

This allows using the same function signatures in r-efi dependencies without having to maintain two copies of it. Will also be helpful when we migrate to using "efiapi" completely. Signed-off-by: Ayush Singh <ayushsingh1325@gmail.com>

build: fix project structure

28e7149

Added mod.rs files in module folders. Signed-off-by: Ayush Singh <ayushsingh1325@gmail.com>

vendor: make vendor optional

3b2e09c

vendor module should be optional Signed-off-by: Ayush Singh <ayushsingh1325@gmail.com>

dvdhrm requested changes Jun 23, 2022

View reviewed changes

dvdhrm closed this Jun 30, 2022

Ayush1325 deleted the std-dep branch July 11, 2022 08:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Getting r-efi ready for use in Rust std #33

Getting r-efi ready for use in Rust std #33

Ayush1325 commented Jun 22, 2022

dvdhrm left a comment

dvdhrm Jun 23, 2022

Ayush1325 Jun 23, 2022 •

edited

dvdhrm Jun 23, 2022

Ayush1325 Jun 23, 2022 •

edited

dvdhrm Jun 23, 2022

Ayush1325 Jun 23, 2022 •

edited

dvdhrm Jun 23, 2022

Ayush1325 Jun 23, 2022

dvdhrm Jun 23, 2022

Ayush1325 Jun 23, 2022 •

edited

dvdhrm Jun 23, 2022

dvdhrm commented Jun 30, 2022

Ayush1325 commented Jul 1, 2022

Getting r-efi ready for use in Rust std #33

Getting r-efi ready for use in Rust std #33

Conversation

Ayush1325 commented Jun 22, 2022

dvdhrm left a comment

Choose a reason for hiding this comment

dvdhrm Jun 23, 2022

Choose a reason for hiding this comment

Ayush1325 Jun 23, 2022 • edited

Choose a reason for hiding this comment

dvdhrm Jun 23, 2022

Choose a reason for hiding this comment

Ayush1325 Jun 23, 2022 • edited

Choose a reason for hiding this comment

dvdhrm Jun 23, 2022

Choose a reason for hiding this comment

Ayush1325 Jun 23, 2022 • edited

Choose a reason for hiding this comment

dvdhrm Jun 23, 2022

Choose a reason for hiding this comment

Ayush1325 Jun 23, 2022

Choose a reason for hiding this comment

dvdhrm Jun 23, 2022

Choose a reason for hiding this comment

Ayush1325 Jun 23, 2022 • edited

Choose a reason for hiding this comment

dvdhrm Jun 23, 2022

Choose a reason for hiding this comment

dvdhrm commented Jun 30, 2022

Ayush1325 commented Jul 1, 2022

Ayush1325 Jun 23, 2022 •

edited

Ayush1325 Jun 23, 2022 •

edited

Ayush1325 Jun 23, 2022 •

edited

Ayush1325 Jun 23, 2022 •

edited