-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Conversation
This PR modifies cc @jieyouxu |
@bors try |
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
There was a problem hiding this 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.
This comment has been minimized.
This comment has been minimized.
💔 Test failed - checks-actions |
Do we link dynamically on i686 but statically on x86_64? That's going to make things slightly more complicated. |
@bors try |
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
☀️ Try build successful - checks-actions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pog!
#[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"))] |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
This comment was marked as resolved.
This comment was marked as resolved.
Looks good as is, so |
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
…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
Ah, a proper test failure. So the good news is that we don't need all the |
The run-make-support library was changed cc @jieyouxu |
@bors try |
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
wtf |
This comment has been minimized.
This comment has been minimized.
💔 Test failed - checks-actions |
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. |
@bors try |
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
This comment has been minimized.
This comment has been minimized.
💔 Test failed - checks-actions |
@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 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 |
Thank you for trying! Knowing more specifically why it doesn't work is very useful! @bors r+ rollup |
…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
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
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