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

WIP: Re-organising repo #169

Closed
wants to merge 10 commits into from
Closed

WIP: Re-organising repo #169

wants to merge 10 commits into from

Conversation

thejpster
Copy link
Contributor

@thejpster thejpster commented Nov 19, 2019

This PR re-organises the embedded-hal repo into a workspace of related crates. There's a top-level embedded-hal crate which pulls in the stable items from the other crates, at stable revisions.

DO NOT MERGE. This PR is a work in progress.

The goal for this PR is to allow unstable features to be developed at a different pace to stable releases. By moving the traits into their own crates, they can have new unstable features added as and when required, which can be published to crates.io. If you want a stable HAL, you should simply continue to use the top-level embedded-hal crate which simply pulls in only the stable items from some recent release of each of the sub-crates.

For example, embedded-hal might contain:

pub mod serial {
    pub use embedded_hal_serial::Write as Write;
}

Here the Write trait is being re-exported from the embedded_hal_serial. Some new experimental AsyncWrite trait could be added to embedded_hal_serial, causing a minor, or even major, version bump of that crate, but users of the top-level embedded-hal trait wouldn't notice until we elected to up-version the dependency in the top-level crate.

@rust-highfive
Copy link

r? @therealprof

(rust_highfive has picked a reviewer for you, use r? to override)

@thejpster
Copy link
Contributor Author

Ok, so most of the modules are in place. I just need to bring over all the old embedded-hal documentation.

@little-arhat
Copy link
Contributor

Hi @thejpster, could you please add motivation/goals for this change to PR description? Thanks!

Comment on lines +14 to +28
[dependencies.embedded-hal-delay]
path = "../embedded-hal-delay"
# version = "1.0"

[dependencies.embedded-hal-digital]
path = "../embedded-hal-digital"
# version = "1.0"

[dependencies.embedded-hal-i2c]
path = "../embedded-hal-i2c"
# version = "1.0"

[dependencies.embedded-hal-serial]
path = "../embedded-hal-serial"
# version = "1.0"
Copy link
Member

Choose a reason for hiding this comment

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

How these crates will be updated in terms of visibility from embedded-hal? Will embedded-hal have version bump each time one of the child crates is updated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

embedded-hal will only bump version when they want to do a release. The underlying crates can be released as often as the authors feel is necessary.

  • Using embedded-hal-serial is 'unstable' (although it follows semver, things move fast)
  • Using embedded-hal is 'stable' (it picks up changes from embedded-hal-XXX relatively slowly and with an eye on backwards compatibility)

Copy link
Member

Choose a reason for hiding this comment

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

What is the intended use for these not-yet-stable releases? I believe that hal providers will use stable versions anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New HALs mainly, or proposed breaking changes. You can depend your chip crate or driver crate on the bleeding edge if necessary, or on the stable release.

Think of HEAD as nightly, a release of embedded-hal-XXX as beta, and embedded-hal as stable.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, in this model, driver that implements the bleeding edge interface will be incompatible with the existing ecosystem without numerous compatibility layers. I don't know if such driver will be useful.

@thejpster
Copy link
Contributor Author

So, I'm confused by the Travis build. The script seems to attempt to run tests on the embedded targets - which won't work as running tests needs std.

@thejpster
Copy link
Contributor Author

So, I'm confused by the Travis build. The script seems to attempt to run tests on the embedded targets - which won't work as running tests needs std.

Ah, I messed up when I 'cleaned' up script.sh. It was only running the tests on 'nightly' before, which worked because the only nightly build was on x86-64. I've changed the script to only run the tests on 'x86-64' instead.

@Disasm
Copy link
Member

Disasm commented Nov 26, 2019

I slightly concerned about having everything in monorepo due to the potential problems with semver-trick and numerous auxiliary branches. Maybe these projects should live in submodules. Problems may happen with CI too due to the potential CI breakage.

@ryankurte
Copy link
Contributor

ryankurte commented Dec 3, 2019

hmm, the outcome (being able to develop / test / bump individual drivers) is really nice, but, my feeling is that this is a significant increase in complexity, opportunities for incompatibility (as @Disasm mentioned), and management overhead? also if this is something we're v convinced is an improvement, i think it probably worth an RFC?

I slightly concerned about having everything in monorepo due to the potential problems with semver-trick and numerous auxiliary branches. Maybe these projects should live in submodules.

afaik the big projects projects that started with separate repos have susbsequently moved to monorepos for similar (nightmarish compatibility) reasons. i'm not convinced that either of these approaches are improvements over the current one :-/

for some other rust-embedded things we have the approach of experiment independently and merge once we've worked out what works, would this be possible alternative for experimenting with traits and breaking changes? i think with some documentation we could achieve the same flexibility without additional complexity in this crate (and i’m happy to put together a more in-depth counter proposal if it’s useful).

(edit: see also "Accepting new additions" in #163)

@thejpster thejpster mentioned this pull request Jan 7, 2020
@thejpster
Copy link
Contributor Author

We agreed on 10 Mar 2020 to abandon this approach. Thank you for the comments!

@thejpster thejpster closed this Mar 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Review is incomplete T-hal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants