Rahix / shared-bus

Crate for sharing buses between multiple devices
Apache License 2.0
129 stars 33 forks source link

Migrate to embedded-hal 1.0.0-alpha.9 #38

Closed taks closed 2 years ago

taks commented 2 years ago

See https://github.com/rust-embedded/embedded-hal/releases/tag/v1.0.0-alpha.9

taks commented 2 years ago

Thank you for checking.

It is best to be able to choose alpha.8 or alpha.9. But, I tried the following setting to distinguish between alpha.8 and alpha.9.

[dependencies]
embedded-hal-alpha = { package = "embedded-hal", version = "^1.0.0-alpha.8", optional = true }
embedded-hal-alpha-9 = { package = "embedded-hal", version = "^1.0.0-alpha.9", optional = true }

[features]
eh-alpha = ["embedded-hal-alpha"]
eh-alpha-9 = ["embedded-hal-alpha-9"]

Even if I specified el-alpha feature, it was recognized as version “1.0.0-alpha.9”. Do you know how to distinguish between alpha.8 and alpha.9?

Rahix commented 2 years ago

Indeed, your approach might be the best solution for the time being.

Hm, does it work if you use = version requirements, maybe?

[dependencies]
embedded-hal-alpha = { package = "embedded-hal", version = "=1.0.0-alpha.8", optional = true }
embedded-hal-alpha-9 = { package = "embedded-hal", version = "=1.0.0-alpha.9", optional = true }
taks commented 2 years ago

Unfortunately, the settings you provided will result in the following error.

error: failed to select a version for `embedded-hal`.
    ... required by package `shared-bus v0.2.4 (/private/tmp/shared-bus)`
versions that meet the requirements `=1.0.0-alpha.9` are: 1.0.0-alpha.9

all possible versions conflict with previously selected packages.

  previously selected package `embedded-hal v1.0.0-alpha.8`
    ... which satisfies dependency `embedded-hal-alpha = "=1.0.0-alpha.8"` of package `shared-bus v0.2.4 (/private/tmp/shared-bus)`

failed to select a version for `embedded-hal` which could resolve this conflict
Rahix commented 2 years ago

Uff, that's weird. To all my knowledge, they shouldn't interfere when specified this way. I'll try to investigate this as well, I will let you know if I find something...

taks commented 2 years ago

I created a test branch using https://github.com/Rahix/shared-bus/pull/38#issuecomment-1272506961 settings. (https://github.com/taks/shared-bus/tree/embedded-hal-1.0.0-alpha.9-test)

The result is usable from the projects that used shared-bus library. (https://github.com/taks/shared-bus/tree/embedded-hal-1.0.0-alpha.9-test/test_projects) However, when I build the shared-bus library itself (with no features), I get the errors https://github.com/Rahix/shared-bus/pull/38#issuecomment-1272544763

Rahix commented 2 years ago

Hm, it feels like this is a bug in cargo. With just the following lines in an empty project, it is failing:

[dependencies]
embedded-hal = "=0.2.3"
ver2 = { package = "embedded-hal", version = "=0.2.4" }

while it works with

[dependencies]
embedded-hal = "=0.2.3"
ver2 = { package = "embedded-hal", version = "=0.1.0" }

It looks like this fails when the versions are semver-compatible to each other but = is used to pin both to a specific one.

Do you want to report it upstream or should I take care of it?

taks commented 2 years ago

I searched cargo issues and found a similar issue here. The conclusion of this issue is that this behavior is a specification. And, I found the dependency reoslution reference.

Rahix commented 2 years ago

Just for the record, this was discussed in the embedded rust working group meeting today: https://github.com/rust-embedded/wg/pull/640

The consensus was mostly that the alpha versions shouldn't be considered semver-compatible by cargo in the first place which would have avoided this problem. We'll have to talk to cargo folks to see what they think about this...

Still, I am surprised that there is no way to override this behavior when absolutely needed.

taks commented 2 years ago

Thank you for sharing. I agree with the alpha versions need to be considered incompatible.

Regarding this change, since v1.0.0-alpha.8 is still used by other libraries, I would like to leave the merge timing to you.

zRedShift commented 2 years ago

Hi @taks @Rahix, thanks for the work on the crate! Just wanted to chime in wrt release timing of this PR.

We're currently using this branch with esp-idf-hal (esp-hal also moved to alpha 9 and released its versions to crates.io 2 days ago), and on the official Matrix channel, we're get an influx of users using the crates.io published version of shared-bus, with the embedded-hal alpha 8/9 incompatibility issue. So it would be nice if this PR got merged...

Also, if you look at the charts at https://crates.io/crates/embedded-hal/1.0.0-alpha.8 and https://crates.io/crates/embedded-hal/1.0.0-alpha.9, you can see that the alpha.9 weekly downloads have already surpassed alpha.8.

Rahix commented 2 years ago

Hm, okay, I think I've made up my mind: As this is the embedded-hal alpha, we can accept breaking downstream code in a semver-compatible release. People should know what they are doing when they are using an alpha version. So I'll merge this change and release a new patch release.

If you have users on the alpha release, please encourage them to pin (use =#.#.#) all dependencies related to the alpha (like shared-bus). This way, the fallout of the situation will at least be limited when the next alpha version hits.

Of course this is still quite unfortunate but I don't see any other option that doesn't end in maintenance hell or depends on changing the cargo resolver as discussed previously...

Rahix commented 2 years ago

@zRedShift: https://github.com/Rahix/shared-bus/releases/tag/v0.2.5