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

run-make: explaing why fmt-write-bloat is ignore-windows #128807

Merged
merged 2 commits into from
Aug 10, 2024

Conversation

ChrisDenton
Copy link
Member

@ChrisDenton ChrisDenton commented Aug 8, 2024

The trouble here is that libc doesn't exist on Windows. Well it kinda does but it isn't called that so we substitute a name that works. Ideally finding necessary libs for the platform would be done at a higher level but until then this should work.

try-job: x86_64-msvc
try-job: x86_64-mingw
try-job: i686-msvc
try-job: i686-mingw

@rustbot
Copy link
Collaborator

rustbot commented Aug 8, 2024

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 8, 2024
@rustbot
Copy link
Collaborator

rustbot commented Aug 8, 2024

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

@ChrisDenton
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Aug 8, 2024

⌛ Trying commit 06d09ad with merge 5811814...

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 8, 2024
run-make: run fmt-write-bloat on Windows

The trouble here is that libc doesn't exist on Windows. Well it kinda does but it isn't called that so we substitute a name that works. Ideally finding necessary libs for the platform would be done at a higher level but until then this should work.

try-job: x86_64-msvc
try-job: i686-msvc
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, r=me if try job comes back green.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Aug 8, 2024

💔 Test failed - checks-actions

@bors bors 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 Aug 8, 2024
@ChrisDenton
Copy link
Member Author

warning LNK4098: defaultlib 'msvcrt' conflicts with use of other libs

Do we link dynamically on i686 but statically on x86_64? That's going to make things slightly more complicated.

@ChrisDenton
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Aug 8, 2024

⌛ Trying commit 67518d0 with merge 81ac031...

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 8, 2024
run-make: run fmt-write-bloat on Windows

The trouble here is that libc doesn't exist on Windows. Well it kinda does but it isn't called that so we substitute a name that works. Ideally finding necessary libs for the platform would be done at a higher level but until then this should work.

try-job: x86_64-msvc
try-job: i686-msvc
@bors
Copy link
Contributor

bors commented Aug 8, 2024

☀️ Try build successful - checks-actions
Build commit: 81ac031 (81ac03195aa96073b58ec4de9fa295c33a0f08ed)

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.

Pog!

Comment on lines 8 to 10
#[cfg_attr(not(target_env = "msvc"), link(name = "c"))]
#[cfg_attr(all(target_env = "msvc", target_feature = "crt-static"), link(name = "libcmt"))]
#[cfg_attr(all(target_env = "msvc", not(target_feature = "crt-static")), link(name = "msvcrt"))]
Copy link
Member

Choose a reason for hiding this comment

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

Remark: this is not obvious, at all lol

Copy link
Member Author

Choose a reason for hiding this comment

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

Aye. I do think this logic should ideally either be in run-make-support or better yet compiletest could have something like a NO_STD_EXTRA_ARGS variable that any test can use. Or at least I think that's better than individual tests needing to figure it out. But I'm not entirely sure how best to approach that.

Copy link
Member

Choose a reason for hiding this comment

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

Good point. I opened #128821 so this doesn't get lost in a review comment once the PR is merged.

@jieyouxu

This comment was marked as resolved.

@jieyouxu
Copy link
Member

jieyouxu commented Aug 8, 2024

Looks good as is, so
@bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 8, 2024

📌 Commit 67518d0 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 Aug 8, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 8, 2024
run-make: run fmt-write-bloat on Windows

The trouble here is that libc doesn't exist on Windows. Well it kinda does but it isn't called that so we substitute a name that works. Ideally finding necessary libs for the platform would be done at a higher level but until then this should work.

try-job: x86_64-msvc
try-job: i686-msvc
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 8, 2024
…iaskrgr

Rollup of 3 pull requests

Successful merges:

 - rust-lang#128795 (Update E0517 message to reflect RFC 2195.)
 - rust-lang#128804 (run-make: enable msvc for redundant-libs)
 - rust-lang#128807 (run-make: run fmt-write-bloat on Windows)

r? `@ghost`
`@rustbot` modify labels: rollup
@compiler-errors
Copy link
Member

#128819 (comment)

@bors r-

@ChrisDenton
Copy link
Member Author

Symbol { name: "__imp__OutputDebugStringA@4", address: 4264468, size: 0, kind: Data, section: Section(SectionIndex(6)), scope: Linkage, weak: false, flags: None } contains Debug
thread 'main' panicked at C:\a\rust\rust\tests\run-make\fmt-write-bloat\rmake.rs:33:5:
assertion failed: !any_symbol_contains(&bin_name("main"), &panic_syms)

Ah, a proper test failure. So the good news is that we don't need all the #[link] shenanigans. The other news is that I've found the reason why we were ignoring __imp_ symbols. Apparently it is because of i686-mingw (but not any other target).

@rustbot
Copy link
Collaborator

rustbot commented Aug 8, 2024

The run-make-support library was changed

cc @jieyouxu

@ChrisDenton
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Aug 8, 2024

⌛ Trying commit a33003b with merge 7dd8f8b...

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 8, 2024
run-make: run fmt-write-bloat on Windows

The trouble here is that libc doesn't exist on Windows. Well it kinda does but it isn't called that so we substitute a name that works. Ideally finding necessary libs for the platform would be done at a higher level but until then this should work.

try-job: x86_64-msvc
try-job: x86_64-mingw
try-job: i686-msvc
try-job: i686-mingw
@jieyouxu
Copy link
Member

jieyouxu commented Aug 8, 2024

The other news is that I've found the reason why we were ignoring __imp_ symbols. Apparently it is because of i686-mingw (but not any other target).

wtf

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Aug 8, 2024

💔 Test failed - checks-actions

@ChrisDenton
Copy link
Member Author

ChrisDenton commented Aug 8, 2024

Ok, this is not going to be as easy as I thought. Maybe I should try compiling it as a staticlib. However, I'd need to make sure it's still testing everything correctly. EDIT: Hmm... that doesn't really work. The original issue seems quite sensitive to the optimization used when in a staticlib.

@ChrisDenton
Copy link
Member Author

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 8, 2024
run-make: run fmt-write-bloat on Windows

The trouble here is that libc doesn't exist on Windows. Well it kinda does but it isn't called that so we substitute a name that works. Ideally finding necessary libs for the platform would be done at a higher level but until then this should work.

try-job: x86_64-msvc
try-job: x86_64-mingw
try-job: i686-msvc
try-job: i686-mingw
@bors
Copy link
Contributor

bors commented Aug 8, 2024

⌛ Trying commit d8969b0 with merge 442f40b...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Aug 8, 2024

💔 Test failed - checks-actions

@ChrisDenton ChrisDenton changed the title run-make: run fmt-write-bloat on Windows run-make: explaing why fmt-write-bloat is ignore-windows Aug 9, 2024
@ChrisDenton
Copy link
Member Author

@jieyouxu ok, I'm going to admit defeat here. At least for the time being. I have updated the test to explain why this test in particular is ignore-windows. I've also kept the changes to not link libc on Windows, even though Windows is ignore, in case it helps someone else who looks at this in the future.

Anyway, the tl;dr version of the comment is that to properly run this test on Windows MSVC we need to parse the debug information from a .pdb file. On Windows MINGW we (currently) can't stop a load of random runtime stuff being included so on that platform at least we'd need to be more specific than e.g. "Debug" when looking for symbols.

@jieyouxu
Copy link
Member

Thank you for trying! Knowing more specifically why it doesn't work is very useful!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 10, 2024

📌 Commit ef90df6 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 Aug 10, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 10, 2024
…llaumeGomez

Rollup of 8 pull requests

Successful merges:

 - rust-lang#128273 (Improve `Ord` violation help)
 - rust-lang#128807 (run-make: explaing why fmt-write-bloat is ignore-windows)
 - rust-lang#128903 (rustdoc-json-types `Discriminant`: fix typo)
 - rust-lang#128905 (gitignore: Add Zed and Helix editors)
 - rust-lang#128908 (diagnostics: do not warn when a lifetime bound infers itself)
 - rust-lang#128909 (Fix dump-ice-to-disk for RUSTC_ICE=0 users)
 - rust-lang#128910 (Differentiate between methods and associated functions in diagnostics)
 - rust-lang#128923 ([rustdoc] Stop showing impl items for negative impls)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a7e188a into rust-lang:master Aug 10, 2024
6 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Aug 10, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 10, 2024
Rollup merge of rust-lang#128807 - ChrisDenton:bloat, r=jieyouxu

run-make: explaing why fmt-write-bloat is ignore-windows

The trouble here is that libc doesn't exist on Windows. Well it kinda does but it isn't called that so we substitute a name that works. Ideally finding necessary libs for the platform would be done at a higher level but until then this should work.

try-job: x86_64-msvc
try-job: x86_64-mingw
try-job: i686-msvc
try-job: i686-mingw
@ChrisDenton ChrisDenton deleted the bloat branch August 10, 2024 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants