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

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

Merged
merged 1 commit into from Nov 22, 2022

Conversation

taks
Copy link
Contributor

@taks taks commented Oct 8, 2022

Copy link
Owner

@Rahix Rahix left a comment

Choose a reason for hiding this comment

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

Hi, thanks for the PR!

I'm a bit hesitant about this as it is technically a semver-breaking change. I am not sure how to best handle this... The only solution I see would be to declare that the eh-alpha feature is not following semver so users must pin shared-bus with a =0.2.x dependency. But we didn't do this from the get-go so there will be projects out there who'd get broken by introducing this now...

What do you think about this?

@taks
Copy link
Contributor Author

taks commented Oct 9, 2022

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
Copy link
Owner

Rahix commented Oct 9, 2022

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
Copy link
Contributor Author

taks commented Oct 9, 2022

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
Copy link
Owner

Rahix commented Oct 9, 2022

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
Copy link
Contributor Author

taks commented Oct 10, 2022

I created a test branch using #38 (comment) 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 #38 (comment)

@Rahix
Copy link
Owner

Rahix commented Oct 10, 2022

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
Copy link
Contributor Author

taks commented Oct 10, 2022

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
Copy link
Owner

Rahix commented Oct 11, 2022

Just for the record, this was discussed in the embedded rust working group meeting today: rust-embedded/wg#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
Copy link
Contributor Author

taks commented Oct 12, 2022

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
Copy link

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
Copy link
Owner

Rahix commented Nov 22, 2022

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...

Copy link
Owner

@Rahix Rahix left a comment

Choose a reason for hiding this comment

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

@taks, thanks for providing this changeset!

@Rahix Rahix merged commit c09023d into Rahix:main Nov 22, 2022
@Rahix
Copy link
Owner

Rahix commented Nov 22, 2022

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

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 this pull request may close these issues.

None yet

3 participants