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

update littlefs2-sys bindgen dependency to 0.60 #2

Merged
merged 2 commits into from
Dec 13, 2022

Conversation

mykmelez
Copy link

Per rust-lang/rust-bindgen#2030, this seems to interact poorly with other bindgen dependents that also specify features = ["runtime"] (and perhaps other dependents that don't specify default-features = "false" at all).

Per rust-lang/rust-bindgen#2030, this seems to interact poorly with other bindgen dependents that also specify `features = ["runtime"]` (and perhaps other dependents that don't specify `default-features = "false"` at all).
Copy link

@nathan-whittington nathan-whittington left a comment

Choose a reason for hiding this comment

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

This works for me.

@mykmelez mykmelez changed the title don't specify default-features = false for bindgen dependency update littlefs2-sys bindgen dependency to 0.60 Dec 13, 2022
@mykmelez mykmelez requested a review from codysch December 13, 2022 19:28
@mykmelez
Copy link
Author

mykmelez commented Dec 13, 2022

Per the recommendation from @codysch in Slack, I've updated this crate's bindgen dependency to 0.60 instead of removing default-features = false. That's the version of bindgen we use elsewhere in JOSL, and it enables cargo xtask build -t esp32s3 -p example-hummingbird-littlefs2 to succeed for me locally.

This change is a little less upstreamable than updating the bindgen dependency to the latest available version of bindgen (0.63), which is presumably what the maintainers would do if they were doing it themselves (and perhaps what they will request if we try to upstream this change).

If we instead update this crate's bindgen dependency to 0.63, then we'd need to either update the bindgen dependencies in JOSL crates to the same version or add an unused build-dependency on bindgen 0.63 in some JOSL crate to ensure feature unification when building this crate along with JOSL crates for esp32s3. I haven't done that for now.

@codysch, are you ok with this approach?

Copy link

@codysch codysch left a comment

Choose a reason for hiding this comment

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

I'm ok with this, though we should really update our bindgen to the latest everywhere at some point.

@mykmelez
Copy link
Author

Agreed about updating to the latest bindgen everywhere.

Also worth noting, da42d4e suggests default features were disabled for bindgen to avoid enabling log/std, presumably for non-build dependencies, which is a problem that can now be solved by enabling the v2 Cargo resolver. So perhaps we can upstream a change that updates bindgen to 0.63, removes default-features = false, and sets resolver = "2" in the [package].

@mykmelez mykmelez merged commit 871a86b into 0.1.6-june Dec 13, 2022
@mykmelez mykmelez deleted the myk/nathan/JSW-41451 branch December 13, 2022 19:46
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.

3 participants