-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Make list of native artifacts to link more compact. #40076
Conversation
Before: ``` note: link against the following native artifacts when linking against this static library note: the order and any duplication can be significant on some platforms, and so may need to be preserved note: library: util note: library: dl note: library: rt note: library: pthread note: library: gcc_s note: library: c note: library: m note: library: rt note: library: util Finished dev [unoptimized + debuginfo] target(s) in 330.48 secs ``` After: ```note: link against the following native artifacts when linking against this static library note: the order and any duplication can be significant on some platforms, and so may need to be preserved note: library: util, dl, rt, pthread, gcc_s, c, m, rt, util Finished dev [unoptimized + debuginfo] target(s) in 330.48 secs ```
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
This is the more conservative change. I’d rather remove this note entirely (and provide some other way to get that info, on demand). It shows up at every edit-recompile-test cycle, whereas it’s probably only useful when configuring a build system which is much less frequently. |
I vaguely remember someone parsing this list for build system integration (e.g. relying on one lib per line), but I may also be misremembering. @rillian was that you? |
Well, r=me on the code anyhow, obviously though there is a bit of a 'policy' decision here. This seems like a pretty suboptimal way to output info meant for tool consumption though! |
If we want to output info meant for tool consumption, shouldn't the tool be using json output anyway? |
Yes, that was me. I'm not currently using it for anything important, but appreciate the heads up that I'll need to update the script. I also opened #31875 about adding an |
@alexcrichton so do we have any suggestion for what that might look like? |
@nikomatsakis yes (all that was missing really was tests) |
I agree with @alexcrichton if something like #31875 is merged, I’d rather remove this note entirely rather than only make it more compact. |
#31875 has merged. Where does that leave this PR? |
Oh, my mistake. It is closed. Well -- same question! Where does our discussion leave this PR? Go push on something like #31875 first? |
@nikomatsakis that's my assumption, yes. |
OK, then I'm going to close until then. |
To be honest it’s a bit disappointing to see an incremental improvement ready to be merged being rejected solely because a better solution might be possible, even though the better solution not here yet, takes more work, and no-one is doing that work in the foreseeable future. As far as I can tell this PR does not prevent doing something like #31875 later. |
As I mentioned before, this is not rejected purely because a better solution may be possible. It's because there may be tools relying on this and if we change it then it breaks the tools with no other solution. |
Before:
After: