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

Revisit module structure and re-exports #155

Open
robin-nitrokey opened this issue Mar 26, 2024 · 4 comments
Open

Revisit module structure and re-exports #155

robin-nitrokey opened this issue Mar 26, 2024 · 4 comments

Comments

@robin-nitrokey
Copy link
Member

When working on splitting Trussed into a client API and a backend implementation, I noticed the following:

Currently, many types that are declared in Trussed are re-exported in multiple locations. For example, the Platform trait:

  • defined in platform::Platform
  • re-exported as Platform (top-level)
  • re-exported as types::Platform

External types are also re-exported in multiple locations:

  • platform re-exports rand_core::{CryptoRng, RngCore}
  • types re-exports, among others, many heapless and littlefs2 types

At the same time, we have many modules that only contain one or two public types. For example, error only contains Error and Result. store::certstore only contains Certstore and ClientCertstore. While it makes sense to group the implementation into a module, it does not make sense to use trussed::error::Error, trussed::store::certstore::Certstore or trussed::platform::Platform to refer to these types.

I propose the following guidelines for cleaning up imports and modules:

  1. Types defined in public Trussed modules are not re-exported at all. Types from private Trussed modules are only re-exported once, from their parent module.
  2. Modules that only contain a few public types and that are not behind a feature gate are made private and types are re-exported in the parent module.

The goal of these changes would be to improve readability of the Trussed code and of the code using Trussed, to make it easier to import the correct types and to avoid surprising or unexpected imports.

I’m not sure how to deal with re-exports of external types. One option would be to only re-export them in a single location, types, or to have a re-export of the entire crate on the top-level and not re-export individual types.

What do you think?

@sosthene-nitrokey
Copy link
Contributor

Looks good.

For the re-export of third-party crates I would go to the extent that they should generally be avoided. I don't really see the benefit of re-exporting the rand traits for example.

@robin-nitrokey
Copy link
Member Author

I think re-exports do have some benefits. Especially for types like heapless_bytes::Bytes and littlefs2::Path that are fundamental to Trussed, it would be less convenient to always have to add heapless-bytes and littlefs2 dependencies to all crates using Trussed (and always having to look up the compatible version). I agree that it is less important for the rand traits as they are only part of Platform.

@sosthene-nitrokey
Copy link
Contributor

it would be less convenient to always have to add heapless-bytes and littlefs2 dependencies to all crates using Trussed (and always having to look up the compatible version)

We already have to do that for littlefs2 and many other crates because we're using git dependencies.

You're right that for heapless-bytes and other dependencies where we don't use patch releases that much it's a benefit.

@robin-nitrokey
Copy link
Member Author

I still hope for a future where we don’t need the Git dependencies … or at least only in the runner, not in every crate using Trussed. Actually, it would make sense to move Path/PathBuf from littlefs2 into a separate crate. Maybe something like heapless-path? That would also mean that patches to the littlefs2 implementation would not affect the Trussed API directly.

robin-nitrokey added a commit to robin-nitrokey/trussed that referenced this issue Apr 3, 2024
This patch removes all re-exports of external types as discussed
in trussed-dev#155, except for those that are essential for using Trussed:
heapless_bytes::Bytes (only from the types module) and
littlefs2::path::{Path, PathBuf}.

Also, the cbor_smol re-exports in the main module are kept until we have
a better mechanism for that.

trussed-dev#155
robin-nitrokey added a commit to robin-nitrokey/trussed that referenced this issue Apr 3, 2024
As discussed in trussed-dev#155, this patch removes re-exports from types that are
defined inside the crate.  This means that all types that are defined in
this crate are now only visible under one path.

trussed-dev#155
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

No branches or pull requests

2 participants