chore: Move over final neqo-crypt changes

[larseggert]

I think with this landed, we're ready for cutting 0.0.1 and landing mozilla/neqo#3399.

[Copilot]

Pull request overview

Prepares the crate for the upcoming 0.0.1 cut and alignment with upstream neqo changes by tightening build-script behavior across platforms and adding targeted regression tests for constants/error codes.

Changes:

• Adjust build.rs to better handle CI vs local environments, improve macOS libclang selection, and use Cargo target cfg vars for cross-compilation correctness.
• Add tests validating mozpkix error-code sequencing/values and Epoch -> usize conversions.
• Minor lint-attribute adjustment so dead_code expectations don’t conflict with test-only usage.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
src/err.rs	Makes dead_code expectation conditional and adds a test asserting mozpkix error code values/sequencing.
src/constants.rs	Adds a small unit test ensuring Epoch converts into the expected usize values.
build.rs	Improves libclang setup (notably macOS), fixes rerun-if-* directives, and uses CARGO_CFG_TARGET_* for target-aware linking/bindgen flags.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[github-actions]

Clean, well-motivated PR that fixes several real bugs in the build script and adds macOS Xcode toolchain detection.

Key improvements:

• Cross-compilation correctness: env::consts::OS → CARGO_CFG_TARGET_OS / CARGO_CFG_TARGET_ARCH in all target-dependent code paths (static_link, dynamic_link, build_bindings, setup_standalone). setup_clang() correctly retains env::consts::OS since it configures the host toolchain.
• Bug fixes in cargo directives: The old code had println!("rerun-if-env-changed=...") missing the cargo: prefix (lines were silently ignored), cargo:rerun-if-env-changed emitting path values instead of env var names (nss.display(), nssdist.display()), and inert cargo:include= output (no links key in Cargo.toml). All fixed.
• setup_clang() hoisted to main(): Previously only called from setup_standalone(), so the pkg_config() path never configured libclang for bindgen. Now runs unconditionally.
• macOS Xcode detection: Addresses the clang-sys issue where Homebrew LLVM lacks SDK include paths. Fallback chain with clear cargo warnings is well-structured.
• eprintln! → cargo:warning=: Warnings now surface through Cargo's reporting instead of being lost in stderr noise.

The new tests in constants.rs and err.rs are appropriate — they lock down the Epoch ordinal mapping and the mozpkix error code sequence, both of which are load-bearing for correctness.

[Not-Nik]

Maybe the tests from aead_null.rs from 4e35fb3c688acf31714e4ca1617f858a91f27daf should also move over to aead.rs into the #[cfg(feature = "disable-encryption")] mod recprot.

[github-actions]

Solid PR. The previous automated review covered the major correctness points well; a few additional observations:

setup_clang() now runs for all build paths (including gecko)

This is the right call — pkg_config() previously never got libclang setup. The early return on CI is safe for current infra but note that any CI environment (including self-hosted macOS runners) that sets CI=true without pre-configuring LIBCLANG_PATH will silently skip Xcode detection and hit a confusing bindgen error. Consider swapping the CI and LIBCLANG_PATH checks so an explicit LIBCLANG_PATH is always honored first — reads more clearly as "use what you're told, then trust CI, then auto-detect":

if env::var("LIBCLANG_PATH").is_ok() {
    return;
}
if env::var("CI").is_ok() {
    return;
}

pkg_config() doesn't verify --cflags --libs exit status

Pre-existing, but since this function is being cleaned up: if pkg-config --cflags --libs nss fails, stdout is empty, split_whitespace() yields nothing, and the function returns Ok(vec![]) — leading to a confusing bindgen error with no include paths. A .status.success() check after the output()? call would surface the real problem.

Test naming

mozpkix_error_codes_are_negative actually asserts sequential ordering from a base value (-16384 + i), which is a stronger invariant than negativity. mozpkix_error_codes_are_sequential would better describe what the test catches on failure.

rerun-if-changed additions are correct

min_version.txt and src/min_version.rs are transitively included via include_str! — rustc tracks these for recompilation, but the explicit directives ensure the build script is also re-run (distinct from recompiled) when values change. Good addition.

[github-actions]

Summary

Two well-scoped commits: the first fixes several real bugs in build.rs (cross-compilation correctness via CARGO_CFG_TARGET_*, broken cargo: directive prefixes, inert cargo:include= output, setup_clang() now covering all build paths), and the second adds unit tests for the disable-encryption null AEAD and existing constants/error code invariants.

The build script changes have already been thoroughly reviewed and approved. The new AEAD tests in the second commit are well-structured — they cover the roundtrip happy path, empty-plaintext edge case, and three distinct error conditions (too-short input, corrupted tag, all-zero data rejection). The Epoch and mozpkix tests lock down load-bearing ordinal/sequential invariants.

One concern: the AEAD tests are gated on #[cfg(feature = "disable-encryption")] but no CI workflow exercises this feature during test execution. clippy.yml covers it via --feature-powerset but with --no-dev-deps, so #[cfg(test)] modules are excluded. These tests exist but are never run. See inline comment for details.

Previous review observations about CI/LIBCLANG_PATH check ordering and pkg_config() missing a .status.success() check are still valid and not repeated here.

[github-actions]

Good PR — the build script fixes are well-motivated and the cross-compilation correctness (CARGO_CFG_TARGET_* over env::consts::OS) is the right call throughout. Hoisting setup_clang() to main() so the pkg_config() path also gets libclang configuration is an important fix. The cargo: directive prefix fixes and removal of inert cargo:include= output are solid cleanup.

The new tests for Epoch ordinals, mozpkix error code sequencing, and the null AEAD are appropriate — they lock down load-bearing invariants.

Two observations not already covered by prior reviews:

---

Warning

build.rs — CI vs LIBCLANG_PATH check ordering (line 75)

A self-hosted macOS CI runner that sets CI=true but does not pre-configure LIBCLANG_PATH will silently skip the Xcode detection and hit a confusing bindgen error. Swapping these two checks so an explicit LIBCLANG_PATH is always honored first is more defensive — "use what you are told, then trust CI, then auto-detect":

if env::var("LIBCLANG_PATH").is_ok() {
    return;
}
// In CI, the environment is already configured correctly.
if env::var("CI").is_ok() {
    return;
}

---

Note

src/aead.rs — encrypt_in_place bounds-check gap (null AEAD line 322 vs real AEAD lines 152-154)

The new test suite covers encrypt_in_place with a correctly-sized buffer (line 400) but does not test the edge case where data.len() < self.expansion(). The real encrypt_in_place returns Err(SEC_ERROR_BAD_DATA) for undersized buffers, but the disable-encryption version does data.len() - self.expansion() — an arithmetic underflow that panics.

Consider adding either a guard matching the real implementation, or a test documenting this discrepancy:

#[test]
#[should_panic]
fn encrypt_in_place_too_small() {
    let a = aead();
    let mut buf = vec![0u8; a.expansion() - 1];
    let _ = a.encrypt_in_place(0, b"", &mut buf);
}