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.

Sign up for GitHub

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

Jump to bottom

refactor: compiler invocations #12211

Merged
merged 7 commits into from
May 31, 2023
Merged

refactor: compiler invocations #12211

merged 7 commits into from
May 31, 2023

Conversation

weihanglo
Copy link
Member

What does this PR try to resolve?

Found some places needing minor refactors during reviewing #12205

How should we test and review this PR?

Commit by commit. Each has its own explanation.

The first four shouldn't be controversial. However, we need people to scrutinize the last two. All static rustflags additions were moved into a single functions. This shouldn't have any behavior change.

Additional information

After this, I'll do a follow-up to extract flags building process. It is expected to make it clearer and safer for building static and dynamic flags.

@rustbot
Copy link
Collaborator

rustbot commented May 31, 2023

r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-build-execution Area: anything dealing with executing the compiler A-build-scripts Area: build.rs scripts S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 31, 2023
@weihanglo
refactor: rename rustflags to profile_rustflags
b3c9aaf 
Apparently it was a bit confusing where `rustflags` is from
when reading this piece of code.
@weihanglo weihanglo force-pushed the refactor-compiler-invocation branch from d21e8a8 to 5256dec Compare May 31, 2023 18:16
@weihanglo weihanglo force-pushed the refactor-compiler-invocation branch from 5256dec to fb92ee5 Compare May 31, 2023 19:01
epage
epage reviewed May 31, 2023
View reviewed changes
tests/testsuite/profile_targets.rs
@@ -100,7 +100,7 @@ fn profile_selection_build() {
[FINISHED] dev [unoptimized + debuginfo] [..]
"
)
.with_stdout_does_not_contain("[..] -C debuginfo=0[..]")
.with_stderr_does_not_contain("[..] -C debuginfo=0[..]")
Copy link
Contributor

Choose a reason for hiding this comment

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

If these tests were testing the wrong thing, should that be its own commit?

This is also why I don't like testing for something not existing...

epage
epage reviewed May 31, 2023
View reviewed changes
tests/testsuite/build.rs
@@ -1334,13 +1334,13 @@ fn cargo_default_env_metadata_env_var() {
[COMPILING] bar v0.0.1 ([CWD]/bar)
[RUNNING] `rustc --crate-name bar bar/src/lib.rs [..]--crate-type dylib \
--emit=[..]link \
-C prefer-dynamic[..]-C debuginfo=2 \
-C prefer-dynamic[..]-C debuginfo=2 [..]\

This comment was marked as resolved.

Sign in to view

@weihanglo
refactor: only when -C debuginfo > 0 will spilt-debuginfo be passed
64a1f20 
It was unnecessary to pass `spilt-debuginfo` if there is no debuginfo.
Tests are touched here only for matching rustflags invocation stderr
in the original test suite.
@weihanglo
refactor: rename LinkType and move it to custom_build.rs
81adada 
This is for linker arguments from build scripts, so should live there.
@weihanglo
refactor: remove unnecessary mutability
aba1fdc
@weihanglo
refactor: consolidate rustc process building in one place
8ae29fb 
Make it clear by separating static and dynamic rustflags.
@weihanglo
refactor: consolidate rustdoc process building in one place
434509b 
Make it clear by separating static and dynamic rustdocflags.
@weihanglo
fix: should assert against stderr instead
d4067e4 
This was an overlook in rust-lang#12205
@weihanglo weihanglo force-pushed the refactor-compiler-invocation branch from fb92ee5 to d4067e4 Compare May 31, 2023 19:46
@epage
Copy link
Contributor

epage commented May 31, 2023

@bors r+

@bors
Copy link
Collaborator

bors commented May 31, 2023

📌 Commit d4067e4 has been approved by epage

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 May 31, 2023
@bors
Copy link
Collaborator

bors commented May 31, 2023

⌛ Testing commit d4067e4 with merge e755955...

@bors
Copy link
Collaborator

bors commented May 31, 2023

☀️ Test successful - checks-actions
Approved by: epage
Pushing e755955 to master...

@bors bors merged commit e755955 into rust-lang:master May 31, 2023
@weihanglo weihanglo deleted the refactor-compiler-invocation branch May 31, 2023 20:42
@weihanglo weihanglo mentioned this pull request Jun 3, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 4, 2023
@bors
Auto merge of rust-lang#112259 - weihanglo:update-cargo, r=weihanglo
d8095ac 
Update cargo

14 commits in f7b95e31642e09c2b6eabb18ed75007dda6677a0..b0fa79679e717cd077b7fc0fa4166f47107f1ba9
2023-05-30 19:25:02 +0000 to 2023-06-03 14:19:48 +0000
- Emit error when users try to use a toolchain via the `add` or `install` command (rust-lang/cargo#12226)
- Support "default" option for `build.jobs` (rust-lang/cargo#12222)
- Fix typo in changelog (rust-lang/cargo#12227)
- chore: Sort `-Z` flags match statement (rust-lang/cargo#12223)
- Update curl-sys (rust-lang/cargo#12218)
- Bump to 0.73.0; update changelog (rust-lang/cargo#12219)
- refactor: housekeeping for 1.70.0 (rust-lang/cargo#12217)
- nit: fix typo in changelog for 1.70 (rust-lang/cargo#12215)
- Remove a noop `.clone` (rust-lang/cargo#12213)
- refactor: compiler invocations (rust-lang/cargo#12211)
- cargo clean: use `remove_dir_all` (rust-lang/cargo#11442)
- Add a small note about indexes ignoring SemVer build metadata. (rust-lang/cargo#12206)
- Revert "chore: detect the channel a PR wants to merge into" (rust-lang/cargo#12204)
- Don't distinguish `Debuginfo::None` and `Debuginfo::Explicit(None)` (rust-lang/cargo#12205)

r? `@ghost`
RalfJung pushed a commit to RalfJung/miri that referenced this pull request Jun 11, 2023
@bors
Auto merge of #112259 - weihanglo:update-cargo, r=weihanglo
0b44f85 
Update cargo

14 commits in f7b95e31642e09c2b6eabb18ed75007dda6677a0..b0fa79679e717cd077b7fc0fa4166f47107f1ba9
2023-05-30 19:25:02 +0000 to 2023-06-03 14:19:48 +0000
- Emit error when users try to use a toolchain via the `add` or `install` command (rust-lang/cargo#12226)
- Support "default" option for `build.jobs` (rust-lang/cargo#12222)
- Fix typo in changelog (rust-lang/cargo#12227)
- chore: Sort `-Z` flags match statement (rust-lang/cargo#12223)
- Update curl-sys (rust-lang/cargo#12218)
- Bump to 0.73.0; update changelog (rust-lang/cargo#12219)
- refactor: housekeeping for 1.70.0 (rust-lang/cargo#12217)
- nit: fix typo in changelog for 1.70 (rust-lang/cargo#12215)
- Remove a noop `.clone` (rust-lang/cargo#12213)
- refactor: compiler invocations (rust-lang/cargo#12211)
- cargo clean: use `remove_dir_all` (rust-lang/cargo#11442)
- Add a small note about indexes ignoring SemVer build metadata. (rust-lang/cargo#12206)
- Revert "chore: detect the channel a PR wants to merge into" (rust-lang/cargo#12204)
- Don't distinguish `Debuginfo::None` and `Debuginfo::Explicit(None)` (rust-lang/cargo#12205)

r? `@ghost`
@ehuss ehuss added this to the 1.72.0 milestone Jun 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@epage epage epage left review comments

Assignees

@ehuss ehuss

Labels
A-build-execution Area: anything dealing with executing the compiler A-build-scripts Area: build.rs scripts S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.72.0
Development

Successfully merging this pull request may close these issues.
5 participants
@weihanglo @rustbot @epage @bors @ehuss