-
Notifications
You must be signed in to change notification settings - Fork 12.1k
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
Migrate run-make/issue-14500
to new rmake.rs
format
#125047
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
/// Pass a codegen option. | ||
pub fn codegen_option(&mut self, option: &str) -> &mut Self { | ||
self.cmd.arg("-C"); | ||
self.cmd.arg(option); | ||
self | ||
} |
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.
I think specific helpers for specific codegen options are better, i.e. .lto()
over .codegen_options("lto")
or something.
tests/run-make/issue-14500/rmake.rs
Outdated
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.
Please take the opportunity to rename this test to something that's not as opaque as issue-14500
. Maybe reachable-extern-fn-available-lto
?
tests/run-make/issue-14500/rmake.rs
Outdated
use run_make_support::{cc, extra_c_flags, run, rustc, tmp_dir}; | ||
|
||
fn main() { | ||
let libbar_path = tmp_dir().join("libbar.a"); |
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.
I don't recall exactly, do we have a static library name helper? If not, can you add one?
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.
There is already one! I should use it.
@rustbot author |
This comment was marked as outdated.
This comment was marked as outdated.
@rustbot author |
Here is the part that made me suspicious (in
ifeq ($(UNAME),Darwin)
STATICLIB = $(TMPDIR)/lib$(1).a
else
ifdef IS_WINDOWS
ifdef IS_MSVC
STATICLIB = $(TMPDIR)/$(1).lib
else
STATICLIB = $(TMPDIR)/lib$(1).a
endif
else
STATICLIB = $(TMPDIR)/lib$(1).a
endif
endif |
You're right, I completely forgot about msvc being special again... There's technically no standard here, but AFAICT msvc uses |
Thanks! @bors r+ rollup |
…mpiler-errors Rollup of 7 pull requests Successful merges: - rust-lang#119838 (style-guide: When breaking binops handle multi-line first operand better) - rust-lang#124844 (Use a proper probe for shadowing impl) - rust-lang#125047 (Migrate `run-make/issue-14500` to new `rmake.rs` format) - rust-lang#125080 (only find segs chain for missing methods when no available candidates) - rust-lang#125088 (Uplift `AliasTy` and `AliasTerm`) - rust-lang#125100 (Don't do post-method-probe error reporting steps if we're in a suggestion) - rust-lang#125118 (Use new utility functions/methods in run-make tests) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#125047 - Oneirical:test5, r=jieyouxu Migrate `run-make/issue-14500` to new `rmake.rs` format Part of rust-lang#121876 and the associated [Google Summer of Code project](https://blog.rust-lang.org/2024/05/01/gsoc-2024-selected-projects.html). Note: I find suspicious that `libbar.a` is hardcoded and is not using the `STATICLIB` call to adapt to Windows platforms. Is this intentional? If not, this will need to be changed.
Part of #121876 and the associated Google Summer of Code project.
Note: I find suspicious that
libbar.a
is hardcoded and is not using theSTATICLIB
call to adapt to Windows platforms. Is this intentional? If not, this will need to be changed.