Make VaArgSafe dyn-incompatible

[theemathas]

View all comments

This is done just in case we want to later do stuff like adding a Sized supertrait.

See discussion at #t-libs > Should `VaArgSafe` require `Sized`?

r? @clarfonthey

[clarfonthey]

Wow, it's exactly what we talked about

@bors r+ rollup note="Beta nominated"

[rust-bors]

📌 Commit 16529d7 has been approved by clarfonthey

It is now in the queue for this repository.

[JonathanBrouwer]

💔 I suspect this PR failed tests as part of a rollup
@bors r-

After fixing the problem, consider running a try job for the failed job before re-approving.

Link to failure: #162375 (comment)

[rust-bors]

This pull request was unapproved.

This PR was contained in a rollup (#162375), which was unapproved.

View changes since this unapproval

[theemathas]

@bors try x86_64-gnu-stdlib-semver-check

@rustbot ready

[rust-bors]

Unknown argument "x86_64-gnu-stdlib-semver-check". Did you mean to use @bors jobs=<jobs>|parent=<parent>? Run @bors help or go to https://bors.rust-lang.org/help to see available commands.

[theemathas]

@bors try jobs=x86_64-gnu-stdlib-semver-check

[rust-bors]

💔 Test for 55d8e1d failed: CI
A workflow was considered to be a failure because it took only 585s. The minimum duration for CI workflows is configured to be 600s.

[theemathas]

I'm going to assume that's a "job failed successfully" and that this PR is ready for approval.

[zachs18]

Skipping stdlib semver check, because src/bootstrap/stdlib-semver-check-stamp was modified (link) Seems like the thing the try job is meant to run... didn't actually run? Is that supposed to happen?

---

Also the relevant failure from the rollup was

--- failure trait_no_longer_dyn_compatible: trait no longer dyn compatible ---

Description:
Trait is no longer dyn compatible, which breaks `dyn Trait` usage.
        ref: https://doc.rust-lang.org/stable/reference/items/traits.html#object-safety
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/trait_no_longer_dyn_compatible.ron

Failed in:
  trait VaArgSafe in file library/core/src/ffi/va_list.rs:313

Which is intentional here.

[theemathas]

@zachs18 I did in fact modify stdlib-semver-check-stamp.

[clarfonthey]

Huh, had no idea that job was relevant now, and that we were actually blocking on it.

[theemathas]

@clarfonthey That job is using cargo-semver-checks to make sure that we're not making breaking changes to the stable API of the stdlib. See https://rustc-dev-guide.rust-lang.org/tests/stdlib-semver-check.html

This PR is doing exactly that: a breaking change to a "stable" API. This API is only "stable" in beta rust though, so it's fine to do this change.

By modifying the stdlib-semver-check-stamp file, I am telling this job to skip this check. It seems like skipping this check causes the job to finish quickly, which ran into this configured minimum job running time.

Not sure what to do here...

[theemathas]

Asked in https://rust-lang.zulipchat.com/#narrow/channel/219381-t-libs/topic/Breakages.20detected.20by.20cargo-semver-checks/near/622157091

[theemathas]

As per zulip, this should be fine to merge as-is as long as it's rollup=never.

[clarfonthey]

To be clear: I knew this was going to be added, but had no idea we had actually done it.

@bors r+ rollup=never p=1 note="API change to backport"

[rust-bors]

📌 Commit 246105d has been approved by clarfonthey

It is now in the queue for this repository.

[rust-bors]

☀️ Test successful - CI
Approved by: clarfonthey
Duration: 3h 10m 42s
Pushing 752b9bf to main...

[github-actions]

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing 656a9da (parent) -> 752b9bf (this PR)

Test differences

Show 4 test diffs

4 doctest diffs were found. These are ignored, as they are noisy.

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 752b9bf8798c2ffc1d3fe2b804c04454366fc6d6 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

1. test-x86_64-gnu-stable: 1h 38m -> 2h 32m (+53.9%)
2. test-x86_64-gnu-stdlib-semver-check: 15m 50s -> 8m 47s (-44.5%)
3. dist-x86_64-illumos: 1h 23m -> 1h 53m (+36.2%)
4. test-x86_64-gnu-gcc-core-tests: 17m 48s -> 11m 25s (-35.9%)
5. test-x86_64-gnu-aux: 1h 56m -> 2h 34m (+32.3%)
6. dist-x86_64-msvc-alt: 2h 13m -> 2h 55m (+31.8%)
7. test-x86_64-gnu-tools: 1h 9m -> 49m 7s (-29.2%)
8. dist-x86_64-solaris: 1h 18m -> 1h 41m (+28.0%)
9. test-x86_64-gnu-llvm-21: 56m 12s -> 40m 45s (-27.5%)
10. dist-sparcv9-solaris: 1h 41m -> 1h 13m (-27.4%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

[rust-timer]

Finished benchmarking commit (752b9bf): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

Results (primary -5.8%, secondary 4.8%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.8%	[2.0%, 7.7%]	2
Improvements ✅ (primary)	-5.8%	[-5.8%, -5.8%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-5.8%	[-5.8%, -5.8%]	1

Cycles

Results (primary -0.7%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.8%	[0.4%, 1.2%]	5
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.0%	[-4.3%, -0.5%]	6
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.7%	[-4.3%, 1.2%]	11

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 478.991s -> 481.658s (0.56%)
Artifact size: 403.45 MiB -> 403.47 MiB (0.00%)