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

propagate critical-section feature selection into portable-atomic; other minor updates for v1.20.0 #260

Merged

Conversation

brodycj
Copy link
Contributor

@brodycj brodycj commented Sep 2, 2024

(updated)

CI build now fixed with TEMPORARY WORKAROUND in TEST_BETA task.


ORIGINAL RATIONALE RE: propagate critical-section feature selection into portable-atomic

Normally just using critical-section should be good enough for targets like super-outdated thumbv6m-none-eabi target. But in rustls/rustls#2088 I would like to preserve existing use of alloc option, in which case I had to add explicit portable-atomic dependency with its critical-section feature enabled.

IMO it would be nice if I didn't have to add explicit sub-depencencies with this option. This is my proposal to enable critical-section for portable-atomic if portable-atomic is wanted.

Please let me know if you have any other idea or perspective concerning this.

@matklad
Copy link
Owner

matklad commented Sep 3, 2024

Yes, this makes sense! Could you:

  • bump the minor version in Cargo.toml
  • add an entry to the changelog
  • fix the CI (I think you need to update the Cargo.lock.msrv which we used to test old versions of Rust)

@brodycj brodycj force-pushed the propagate-critical-section-to-portable-atomic branch from f8c6b87 to 419e813 Compare September 3, 2024 14:13
@brodycj
Copy link
Contributor Author

brodycj commented Sep 3, 2024

I just force-pushed an update to update the Unreleased section of CHANGELOG.md, will try to fix the CI, update the minor version, and add the minor version heading to CHANGELOG.md next. Thanks!

@brodycj brodycj marked this pull request as draft September 3, 2024 14:28
@brodycj brodycj marked this pull request as ready for review September 3, 2024 14:29
@brodycj brodycj force-pushed the propagate-critical-section-to-portable-atomic branch from 49a0fee to c5a13fd Compare September 3, 2024 14:29
@brodycj brodycj changed the title propagate critical-section feature selection into portable-atomic DRAFT: propagate critical-section feature selection into portable-atomic - DRAFT WIP Sep 3, 2024
@brodycj brodycj changed the title DRAFT: propagate critical-section feature selection into portable-atomic - DRAFT WIP DRAFT: propagate critical-section feature selection into portable-atomic etc. - DRAFT WIP Sep 3, 2024
@brodycj brodycj force-pushed the propagate-critical-section-to-portable-atomic branch from b41e317 to ec4b3de Compare September 3, 2024 16:43
@brodycj brodycj changed the title DRAFT: propagate critical-section feature selection into portable-atomic etc. - DRAFT WIP DRAFT: propagate critical-section feature selection into portable-atomic; minor cleanup; etc. - DRAFT WIP Sep 3, 2024
@brodycj brodycj force-pushed the propagate-critical-section-to-portable-atomic branch from ec4b3de to c63d9d0 Compare September 3, 2024 17:02
CHANGELOG.md Outdated Show resolved Hide resolved
@brodycj brodycj force-pushed the propagate-critical-section-to-portable-atomic branch from ec93899 to dc093ad Compare September 3, 2024 18:53
@brodycj brodycj changed the title DRAFT: propagate critical-section feature selection into portable-atomic; minor cleanup; etc. - DRAFT WIP v1.20.0: propagate critical-section feature selection into portable-atomic; other fixes Sep 3, 2024
@brodycj brodycj force-pushed the propagate-critical-section-to-portable-atomic branch from d3da558 to e9bbdf0 Compare September 3, 2024 19:35
@brodycj

This comment was marked as outdated.

@brodycj

This comment was marked as resolved.

@brodycj brodycj force-pushed the propagate-critical-section-to-portable-atomic branch from e9bbdf0 to f5f45b0 Compare September 9, 2024 21:13
@brodycj

This comment was marked as resolved.

src/imp_std.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
xtask/src/main.rs Outdated Show resolved Hide resolved
@brodycj brodycj changed the title v1.20.0: propagate critical-section feature selection into portable-atomic; other fixes [DRAFT] v1.20.0: propagate critical-section feature selection into portable-atomic; other fixes - DRAFT WIP Sep 13, 2024
@brodycj brodycj changed the title [DRAFT] v1.20.0: propagate critical-section feature selection into portable-atomic; other fixes - DRAFT WIP [DRAFT] propagate critical-section feature selection into portable-atomic; other minor updates for v1.20.0 - DRAFT WIP Sep 13, 2024
@brodycj brodycj force-pushed the propagate-critical-section-to-portable-atomic branch from f5f45b0 to dde6ef2 Compare September 13, 2024 15:05
src/lib.rs Outdated Show resolved Hide resolved
@brodycj brodycj force-pushed the propagate-critical-section-to-portable-atomic branch 2 times, most recently from dfacda0 to b8b5bf4 Compare September 13, 2024 15:45
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@brodycj brodycj changed the title [DRAFT] propagate critical-section feature selection into portable-atomic; other minor updates for v1.20.0 - DRAFT WIP propagate critical-section feature selection into portable-atomic; other minor updates for v1.20.0 Sep 13, 2024
@brodycj

This comment was marked as outdated.

@brodycj brodycj changed the title propagate critical-section feature selection into portable-atomic; other minor updates for v1.20.0 [DRAFT] propagate critical-section feature selection into portable-atomic; other minor updates for v1.20.0 - DRAFT WIP Sep 13, 2024
@brodycj brodycj force-pushed the propagate-critical-section-to-portable-atomic branch from 1a04e4a to d1bf3db Compare September 13, 2024 18:30
@brodycj brodycj force-pushed the propagate-critical-section-to-portable-atomic branch from ff24c4c to dd6b5c2 Compare September 13, 2024 18:36
@brodycj brodycj changed the title [DRAFT] propagate critical-section feature selection into portable-atomic; other minor updates for v1.20.0 - DRAFT WIP propagate critical-section feature selection into portable-atomic; other minor updates for v1.20.0 Sep 13, 2024
@brodycj
Copy link
Contributor Author

brodycj commented Sep 13, 2024

@matklad I think this should be ready (yet again), with a TEMPORARY WORKAROUND in I made in the TEST_BETA task. Please let me know if you need anything else for this. Thanks!

// TEMPORARY WORKAROUND for Rust compiler issue ref:
// - https://github.com/rust-lang/rust/issues/129352
// - https://github.com/matklad/once_cell/issues/261
let _e = sh.push_env("RUSTFLAGS", "-A unreachable_patterns");
Copy link
Owner

Choose a reason for hiding this comment

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

Ahh, I didn't realize this percolated all the way up to the beta! But, anyway, you've found the perfect solution here, which places the fine-grained work-around where it belongs, thanks a bunch for this!

Copy link
Owner

@matklad matklad left a comment

Choose a reason for hiding this comment

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

Looks awesome to me now, thanks for persevering here!

@matklad matklad merged commit 72f7c2e into matklad:master Sep 13, 2024
1 check passed
@matklad
Copy link
Owner

matklad commented Sep 13, 2024

published at https://crates.io/crates/once_cell/1.20.0

@brodycj
Copy link
Contributor Author

brodycj commented Sep 13, 2024

🎉

@kaspar030
Copy link

  • propagate critical-section feature selection into portable-atomic

This breaks our use case. portable-atomic treats critical-section and unsafe-assume-single-core as mutually exclusive. E.g., we depend on esp-rs, which for some platforms (e.g., esp32c2, c3) explicitly enables the latter.

Could we make this optional?

kaspar030 added a commit to kaspar030/RIOT-rs that referenced this pull request Sep 16, 2024
@brodycj brodycj deleted the propagate-critical-section-to-portable-atomic branch September 18, 2024 17:23
@brodycj
Copy link
Contributor Author

brodycj commented Sep 18, 2024

I would like to state my assumption that despite issue #264, all other changes from this PR should be considered valid and should remain in master (and be part of the next release):

  • cargo fmt updates that would be needed to pass cargo fmt --check
  • fix default-features flag for parking_lot_core in dependencies (in response to a warning message I noticed)
  • CI-related updates:
    • add cargo test --workspace to beginning of TEST task (as discussed above, this is needed to ensure we check formatting in CI workflow)
    • temporary workaround for Rust compiler bug in RUSTFLAGS, as was needed for TEST_BETA task to succeed

matklad please comment in case this is mistaken ... apologies for commenting on an already merged & closed PR!

P.S. This also updated Cargo.lock, assume we want to keep this update.

elenaf9 pushed a commit to elenaf9/RIOT-rs that referenced this pull request Sep 27, 2024
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.

CI: detect cargo fmt issues
3 participants