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

Compiletest: add proc-macro header #133540

Merged
merged 2 commits into from
Nov 28, 2024
Merged

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Nov 27, 2024

This adds a proc-macro header to simplify using proc-macros, and to reduce boilerplate. This header works similar to the aux-build header where you pass a path for a proc-macro to be built.

This allows the force-host, no-prefer-dynamic headers, and crate_type attribute to be removed. Additionally it uses --extern like aux_crate (allows implicit extern crate in 2018) and --extern proc_macro (to place in the prelude in 2018).

This also includes a secondary change which defaults the edition of proc-macros to 2024. This further reduces boilerplate (removing extern crate proc_macro;), and allows using modern Rust syntax. I was a little on the fence including this. I personally prefer it, but I can imagine it might be confusing to others. EDIT: Removed

Some tests were changed so that when there is a chain of dependencies A→B→C, that the @ proc-macro is placed in B instead of A so that the --extern flag works correctly (previously it depended on -L to find C). I think this is better to make the dependencies more explicit. None of these tests looked like the were actually testing this behavior.

There is one test that had an unexplained output change: tests/ui/macros/same-sequence-span.rs. I do not know why it changed, but it didn't look like it was particularly important. Perhaps there was a normalization issue?

This is currently not compatible with the rustdoc build-aux-docs header. It can probably be fixed, I'm just not feeling motivated to do that right now.

Implementation steps

This adds a proc-macro header to make it easier to depend on a
proc-macro, and remove some of the boilerplate necessary.
@rustbot rustbot added A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 27, 2024
@rustbot
Copy link
Collaborator

rustbot commented Nov 27, 2024

Some changes occurred in src/tools/compiletest

cc @jieyouxu

Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

Thanks. I'm very much in favor of reducing proc-macro boilerplate but changing the default edition for proc-macro tests as far as I can tell regresses our test coverage -- this is the one thing I'm not sure about. i.e.

This allows the force-host, no-prefer-dynamic headers, and crate_type attribute to be removed. Additionally it uses --extern like aux_crate (allows implicit extern crate in 2018) and --extern proc_macro (to place in the prelude in 2018).

This is great, thank you for the effort!

This also includes a secondary change which defaults the edition of proc-macros to 2024. This further reduces boilerplate (removing extern crate proc_macro;), and allows using modern Rust syntax. I was a little on the fence including this. I personally prefer it, but I can imagine it might be confusing to others.

This, however, I hesistant about. This means that test writers by default won't remember to cover older editions.

Very cool regardless.

src/tools/compiletest/src/runtest.rs Show resolved Hide resolved
src/tools/compiletest/src/runtest.rs Outdated Show resolved Hide resolved
@jieyouxu

This comment was marked as outdated.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 27, 2024
@jieyouxu jieyouxu added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 27, 2024
@jieyouxu
Copy link
Member

The compiletest change LGTM, I'll review the test diff tmrw.

Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

I manually looked through the tests, and AFAICT the transformations are correct.

@jieyouxu
Copy link
Member

Thank you!

@ehuss would you like to update the rustc-dev-guide docs for proc-macro auxiliaries, or do you want me to?

In any case,

@bors r+ rollup=iffy

@bors
Copy link
Contributor

bors commented Nov 28, 2024

📌 Commit f94142b has been approved by jieyouxu

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 28, 2024
@jieyouxu
Copy link
Member

@bors p=1 (conflict-prone)

@jieyouxu jieyouxu added the A-test-infra Area: test infrastructure (may span bootstrap/compiletest/more) label Nov 28, 2024
@jieyouxu
Copy link
Member

Actually I'll update the dev-guide docs.

@jieyouxu
Copy link
Member

jieyouxu commented Nov 28, 2024

Something else came to my mind, actually: if we support transitive builds, i.e. auxiliary can itself declare auxiliaries, technically we can have a cycle... right? What happens then? Does compiletest explode? i.e. if aux A -> aux B but aux B -> aux A... Do we just, infinitely try to compile each other until we run out of disk space?

Whereas the previous scheme disallows auxiliaries from declaring auxiliaries themselves AFAIK, which is also what ui_test does AFAIK, to prevent such build cycles from being possible in the first place.

@jieyouxu
Copy link
Member

@bors r- (one question)

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Nov 28, 2024
@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 28, 2024
@jieyouxu
Copy link
Member

I agree that properly supporting transitive builds is something that would be desirable, but that might require proper cycle detection.

@ehuss
Copy link
Contributor Author

ehuss commented Nov 28, 2024

I agree that properly supporting transitive builds is something that would be desirable, but that might require proper cycle detection.

That's nothing new, is it? This PR isn't changing that. Transitive builds have always been supported with aux-build and aux-crate, and compiletest has always had a stack overflow if it has a cycle.

@jieyouxu
Copy link
Member

That's nothing new, is it? This PR isn't changing that. Transitive builds have always been supported with aux-build and aux-crate, and compiletest has always had a stack overflow if it has a cycle.

Hm, I think you're right, I might be misremembering as in ui_test may have introduced the restriction to fix this bug in compiletest 😅

@bors r+

@bors
Copy link
Contributor

bors commented Nov 28, 2024

📌 Commit f94142b has been approved by jieyouxu

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 28, 2024
@bors
Copy link
Contributor

bors commented Nov 28, 2024

⌛ Testing commit f94142b with merge a2545fd...

@bors
Copy link
Contributor

bors commented Nov 28, 2024

☀️ Test successful - checks-actions
Approved by: jieyouxu
Pushing a2545fd to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 28, 2024
@bors bors merged commit a2545fd into rust-lang:master Nov 28, 2024
7 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Nov 28, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (a2545fd): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.9% [-1.9%, -1.9%] 1
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (secondary -3.3%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.3% [-3.3%, -3.3%] 1
All ❌✅ (primary) - - 0

Cycles

Results (primary -2.5%, secondary 2.3%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.3% [2.3%, 2.3%] 1
Improvements ✅
(primary)
-2.5% [-2.5%, -2.5%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -2.5% [-2.5%, -2.5%] 1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 792.194s -> 791.904s (-0.04%)
Artifact size: 335.92 MiB -> 335.89 MiB (-0.01%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-compiletest Area: The compiletest test runner A-test-infra Area: test infrastructure (may span bootstrap/compiletest/more) A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants