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

Ignore receiver_waker_size test since it's unstable #53

Merged
merged 1 commit into from
Feb 1, 2025

Conversation

faern
Copy link
Owner

@faern faern commented Jan 31, 2025

At first I just updated the sizes to match stable Rust 1.84. But I then realized the size of Thread was smaller again on nightly. So let's ignore the test for now and see what the standard library settles on, if anything stable at all. The standard library give no size guarantees about the struct we are testing the size of, so this test is inherently unstable! The problem lies in this library, not in std. I just want to have this test in place to catch library bugs that cause the ReceiverWaker to blow up in size unintentionally.

The Rust PR that reverted the size increase of std::thread::Thread, and made it go back to pre Rust 1.84 sizes is this one: rust-lang/rust#132654

@faern faern force-pushed the update-waker-size-test branch from d018418 to cbb0dfc Compare January 31, 2025 05:46
@faern
Copy link
Owner Author

faern commented Jan 31, 2025

So annoying that to run cargo test the benchmarking-only dependencies need to be built. And they in turn has a much higher MSRV than this library. Dev dependencies also can't be optional, so I can't run tests without criterion 🤷 🙃

Will work around this by simply not running the tests on the MSRV version of Rust for this library. Verifying it via just cargo build will have to do 😢

@faern faern force-pushed the update-waker-size-test branch from cbb0dfc to d49da88 Compare January 31, 2025 05:49
@faern faern mentioned this pull request Jan 31, 2025
@ada4a
Copy link
Contributor

ada4a commented Jan 31, 2025

So annoying that to run cargo test the benchmarking-only dependencies need to be built. And they in turn has a much higher MSRV than this library. Dev dependencies also can't be optional, so I can't run tests without criterion 🤷 🙃

Couldn't you do something like this?

[target.'cfg(not(test))'.dev-dependencies]
criterion = "0.5.1"

@faern faern force-pushed the update-waker-size-test branch from 144299c to e694a51 Compare February 1, 2025 10:58
@faern
Copy link
Owner Author

faern commented Feb 1, 2025

That is sadly not possible. Quoting https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#platform-specific-dependencies

The same applies to cfg(debug_assertions), cfg(test) and cfg(proc_macro). These values will not work as expected and will always have the default value returned by rustc --print=cfg. There is currently no way to add dependencies based on these configuration values.

@faern faern force-pushed the update-waker-size-test branch 2 times, most recently from c66a944 to e2a2cdb Compare February 1, 2025 11:29
@faern
Copy link
Owner Author

faern commented Feb 1, 2025

But I solved it with a hack similar to the loom stuff: A custom cfg. This hurts the developer experience for the benchmarks, since and it requires running benches in an awkward way, but at least it builds and runs.

I should note that Rust 1.65 is very old! And I'm by no means a strict MSRV advocate. I'd happily be supporting current - 2 or similar as my MSRV policy. It just irritates me that cargo handles this so clumsy. That benchmarking dependencies mess with the support possibilities of the main library.

@faern faern force-pushed the update-waker-size-test branch from 46bbcbf to 2ccf485 Compare February 1, 2025 11:57
@faern faern merged commit 2ccf485 into main Feb 1, 2025
11 checks passed
@faern faern deleted the update-waker-size-test branch February 1, 2025 11:58
@faern
Copy link
Owner Author

faern commented Feb 1, 2025

I moved the criterion over to #54, to not mix different tasks up in one PR. Now this very simple ignore-PR can pass cleanly.

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.

2 participants