-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Add a regression test for #29122 (fixed in #29134) #29151
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
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
@bors r+ rollup |
📌 Commit 19041cd has been approved by |
I suspect this won't work on Windows, but let's be optimistic and try it before disabling.
@bors: r- this caused rollup failure |
I've fixed the pretty failures and I think the test should be working on OS X now. |
…ichton I suspect this won't work on Windows, but let's be optimistic and try it before disabling.
I do not understand this new error. There is rustc output indicating a successful compilation of a staticlib, but then the expected staticlib does not exist.
|
all: | ||
$(RUSTC) library.rs | ||
mkdir $(bad_dir) | ||
mv $(call STATICLIB,library) $(bad_dir) |
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.
The STATICLIB
macro here is expanding to library.lib on Windows when a staticlib output always produces libfoo.a (e.g. liblibrary.a in this case)
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.
The reference could use an update, then. (I'm not going to do it because, as shown here, I don't understand what's going on well enough to write something useful.)
I think at this point I'm just going to disable the test on Windows, if no one objects. I expect that the linking command won't work there anyway, and it seems like there might be several more cycles of this before I even figure out how to attempt to link a Windows native library. Someone who can do local testing and actually knows how to develop for Windows can fix it if they want to.
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.
Oh weird! Yeah that documentation should probably be updated!
Could this just hardcode liblibrary.a for now to work on Windows as well? In general we prefer to have tests run everywhere whenever possible.
Modified to be a no-op on Windows. |
b41963e
to
e6626a8
Compare
(Moving to the main comment thread from line comments.) Alright, try this version. I should probably point out that if this does work on Windows and actually tests what it's supposed to then it would seem I've created a file whose name can't be stored in a |
Thanks! Could you also squash this down to one commit? |
19664fd
to
0dce92a
Compare
Squashed. |
…ichton I suspect this won't work on Windows, but let's be optimistic and try it before disabling.
I suspect this won't work on Windows, but let's be optimistic and try it before disabling.
@bors: r- I think this caused a legitimate failure |
@bors: force |
⛄ The build was interrupted to prioritize another pull request. |
I wasn't sure |
Can we not roll this up in the future? It's already caused three rollups to fail. |
@bors: rollup- |
Not much to go on. The failed command is a grep, so in failing it swallowed all the debugging output. |
This may actually be a bug in MinGW's If that's the case where this just unfortunately doesn't work on Windows then could you add a comment as such before ignoring it? |
0dce92a
to
a1c8431
Compare
This look good? |
I suspect this won't work on Windows, but let's be optimistic and try it before disabling.
I suspect this won't work on Windows, but let's be optimistic and try it before disabling.