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

rustls_version() integration test #434

Merged
merged 5 commits into from
Jun 19, 2024
Merged

Conversation

cpu
Copy link
Member

@cpu cpu commented Jun 12, 2024

Figuring out something to do about the manually sync'd RUSTLS_CRATE_VERSION constant in build.rs has been on my mind for a while.

As explained in the new build.rs comment I was originally thinking about populating the const automatically, but I decided against that because it seemed like it would require brittle by-hand parsing, or taking a new dep (unfortunately, it can't be a development dependency if used in build.rs). Both seemed like bad options so I landed on adding an integration test to catch if we forget to update the constant. This has the added advantage of also replacing an existing looser unit test with one that matches the full expected value from rustls_version() without adding any new hardcoded constants to maintain.

@cpu cpu self-assigned this Jun 12, 2024
@@ -38,6 +38,7 @@ crate-type = ["lib", "staticlib"]

[dev-dependencies]
regex = "1.9.6"
toml = { version = "0.6.0", default-features = false, features = ["parse"] }
Copy link
Member Author

Choose a reason for hiding this comment

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

note: the version here, and in the Cargo.lock was arranged based on this crate's MSRV. The toml stack of crates (toml, toml-edit, serde_spanned, etc) have aggressive MSRV reqs for newer versions. (Another reason I didn't want them to be primary dependencies!)

@cpu
Copy link
Member Author

cpu commented Jun 19, 2024

@ctz If you had a couple min to spare I'd love a +1 on this to get it out of my PR queue. Thanks!

tests/rustls_version.rs Outdated Show resolved Hide resolved
tests/rustls_version.rs Show resolved Hide resolved
@@ -3,6 +3,11 @@ use std::io::Write;
use std::{env, fs, path::PathBuf};

// Keep in sync with Cargo.toml.
//
// We don't populate this automatically from the Cargo.toml at build time
Copy link
Member

Choose a reason for hiding this comment

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

Shower thought:

If the rust crate's build.rs did println!("cargo:version_number={}", env!("CARGO_PKG_VERSION")); then we could simply use env!("DEP_RUSTLS_VERSION_NUMBER") here.

Haven't tried that, and it's probably a worse overall solution than what we have in this PR, and also I should have learned my lesson by now rather than relying on underused parts of cargo.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ooo, I forgot that build.rs can forward information like that. Seems worth playing with but we'd need a rustls release with that magic in-place to be usable here so I think I'll leave it for a "One Day" follow-up.

@cpu cpu force-pushed the cpu-rustls-version-test branch from 7418eeb to bcb733a Compare June 19, 2024 19:28
cpu added 4 commits June 19, 2024 15:31
This uses a dev-dependency on `toml` to parse `Cargo.toml`, matching
pieces of metadata found there with expected values from calling
`rustls_version()`.

Notably this will help catch a failure to keep the `build.rs`
`RUSTLS_CRATE_VERSION` constant in-sync with the Rustls dependency in
`Cargo.toml`.
This will help avoid a `clippy::items_after_test_module` finding after
the stand-alone `test_rustls_version` unit test is removed.
This is now redundant with the more specific `tests/rustls_version.rs`
integration test.
This commit adds some commentary to the `build.rs`
`RUSTLS_CRATE_VERSION` const, justifying why we keep it in-sync
manually.

Initially I was thinking about deducing the `RUSTLS_CRATE_VERSION` in
the `build.rs` script automatically by parsing `Cargo.toml`, but after
thinking through the sensitivity of `build.rs`, and realizing the
parsing would require a new _non-development_ dependency, I abandoned
the idea.

The recently added integration test will catch anyone forgetting to sync
the const and avoids extra deps or complexity in the build script.
@cpu cpu force-pushed the cpu-rustls-version-test branch from bcb733a to 17fe35d Compare June 19, 2024 19:31
@cpu cpu merged commit c74c0a5 into rustls:main Jun 19, 2024
25 checks passed
@cpu cpu deleted the cpu-rustls-version-test branch June 19, 2024 19:36
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