Compact display of static lib dependencies

[kornelski]

Fixes #33173

Instead of displaying one dependency per line, I've changed the format to display them all in one line.

As a bonus they're in format of linker flags (-lfoo), so the output can be copy&pasted if one is actually going to link as suggested.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pnkfelix (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.

[aidanhs]

Thanks for the PR @pornel! We'll check in now and again to make sure this gets reviewed soon.

[retep998]

As a bonus they're in format of linker flags (-lfoo), so the output can be copy&pasted if one is actually going to link as suggested.

If you're going to do it in a way that can be copy pasted to the linker, then please make it adjust the syntax based on what the linker for the current target is. Trying to pass -lfoo to link.exe is only going to end in sadness.

[kornelski]

Which platforms actually need this message?

I vaguely remember that MSVC tracks dependencies even for static libraries. Does it work with Rust too? If so, I'll just hide the whole message for MSVC.

[retep998]

@pornel Rust does not currently emit the appropriate DEFAULTLIB directives into the static library, so the message is still necessary on msvc. My point is rather that the syntax is wrong. When invoking link.exe you're supposed to specify libraries by passing their lliteral name, such as foo.lib, rather than -lfoo.

[kornelski]

OK, I've added MSVC syntax.

[carols10cents]

friendly ping @pnkfelix !

[pnkfelix]

@pornel am I correct in inferring that this is only addressing the first of @ubsan's three points from #33173 ?

The three points I will transcribe here for ease of reference:

1. it's not in a useful output format. It would be more useful if each of the note: library:s was replaced with -l, and it was moved to one line.
2. It's not optional. I have to see this every time I compile. Which is a lot.
3. It's to stderr. These are not errors. I can't even >/dev/null

---

Assuming the above inference is correct...

Addressing point 2 may be more work than is warranted for this particular PR; I'd rather land a small PR that has an obvious benefit than wait a long time for the perfect one.

Point 3 seems important. So that leads me to ask: Is there an easy way to change the output port so that it goes to stdout rather than stderr?

---

On further review of the overall context here, it seems like there is was prior work (#31471, #31875) to address this in a somewhat more complete way...

Anyway, I'm hesitant to just land this as is, even though it seems like an obvious improvement, given that there might be hacks in the wild that are relying on the old format...

[kornelski]

Yes, it's intended to address only the first point.

there might be hacks in the wild that are relying on the old format...

What can I do about this?

[pnkfelix]

@pornel wrote:

What can I do about this [hacks in the wild that are relying on the old format.]?

Oh sorry. I don't actually expect you to try to solve that problem (which strikes me as insolvable)

that was more me thinking out loud about what to do here.

I'm going to nominate this for discussion amongst the compiler team. Sorry for the hassle, and thanks for your patience @pornel!

[pnkfelix]

I would like to see @alexcrichton potentially chime in here, since it seems like he was helping coordinate efforts on #31471 and #31875 to some degree, and thus he might have a more informed opinion than I do about the right path here.

[pnkfelix]

Nominating for discussion at ... well, probably the dev-tools meeting is the most appropriate one for this. (And someone from that team should feel free to steal this bug from me.)

(But I'm also tagging as T-compiler just in case that is the more appropriate team to handle this, or if you prefer, as a worst-case fallback if the dev-tools team does not get to to it before the next compiler triage meeting.)

[alexcrichton]

This was proposed in #40076 which I had concerns about leading to it being closed.

[nrc]

We discussed this quite a bit in the dev tools meeting this week, but did not quite come to a conclusion. There was general agreement, that improvement here is a good thing, but also agreement that we should not break current users. We seemed to talk at cross-purposes somewhat, but here is my understanding about the constraints:

• we do not need to keep the current output, if there is a way for current users to get some more tool-friendly output
• A set of flags seems to fit the bill for tool-friendly, but I'm not sure we 100% agreed on that
• In my view, that means that this PR is OK - it makes a list of flags and makes these available by default on stderr.

However I would be wrong if:

• this PR is not making a list of usable flags
• a list of usable flags is not considered tool-friendly
• the default output (stderr) is not considered tool-friendly (c.f., writing to a file).

The alternative we discussed was:

• output to a specified file by using an --emit flag
• output a list of flags (I think that is the same as in this PR)
• display nothing on stderr by default.

IMO, this PR is a fine alternative to that proposal (I don't see that the alternative is more backwards compatible than the current PR), but I might be wrong.

cc @michaelwoerister, @alexcrichton is my summary correct? Where do we differ in opinion?

[kornelski]

For back-compat the format could be hacked somewhat:

library: foo -lbaz -lbar

assuming current tools search for library: prefix and then pass -l<rest of the line> to the linker without caring about spaces. However, this is ugly and worse in all cases other than (speculative) back compat.

[alexcrichton]

@nrc your summary sounds right to me, thanks for writing that up! I would just want to avoid a "halfway" situation where we change the output and then later change it again with a more "official" output. I figure that if we change it once we should just leave it for good.

I'd personally have a goal here that there's some invocation you can use to have the compiler not emit anything by default, as a quiet compiler is typically synonymous with a happy compiler.

[kornelski]

OK, could the message be hidden when Cargo is doing the linking? i.e. show libraries when the static lib is a top-level project. Don't show libraries to link with when the static lib is built as a dependency.

[retep998]

But the staticlib crate type is never built as a dependency because cargo always builds dependencies as rlibs, except for plugins which are built as dylibs.

[kornelski]

Ah, to clarify: I had in mind building static libs in build.rs via the gcc crate.

If I have project-foo that depends on foo-rs that depends on foo-sys that builds C files as a static lib using the gcc crate, then building of the top-level project-foo still outputs these instructions, which isn't necessary.

[retep998]

If foo-sys builds a static library using the gcc crate it is linked with kind=static then it already won't be printed out in the list of libraries to link to when creating a staticlib.

[bors]

⌛ Testing commit 2920658 with merge b5bc21651233404e6103ca5553fa8132e775e2ca...

[bors]

💔 Test failed - status-travis

[kennytm]

Missing an & before format!.

[00:14:11] error[E0308]: mismatched types
[00:14:11]    --> /checkout/src/librustc_trans/back/link.rs:702:33
[00:14:11]     |
[00:14:11] 702 |         sess.note_without_error(format!("native-static-libs: {}", &lib_args.join(" ")));
[00:14:11]     |                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected &str, found struct `std::string::String`
[00:14:11]     |
[00:14:11]     = note: expected type `&str`
[00:14:11]                found type `std::string::String`
[00:14:11]     = help: try with `&format!("native-static-libs: {}", &lib_args.join(" "))`
[00:14:11]     = note: this error originates in a macro outside of the current crate
[00:14:11] 
[00:14:15] error: aborting due to previous error
[00:14:15] 
[00:14:15] error: Could not compile `rustc_trans`.

[nrc]

@bors: r+

[bors]

📌 Commit 2354089 has been approved by nrc

[bors]

⌛ Testing commit 2354089 with merge 22d6598...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: nrc
Pushing 22d6598 to master...

[mssun]

https://github.com/pornel/rust/blob/2354089ece7d03f997caf8a9f6ad99d235c8dacb/src/librustc/session/config.rs#L1296

https://github.com/pornel/rust/blob/2354089ece7d03f997caf8a9f6ad99d235c8dacb/src/librustc/session/config.rs#L1642

Should it be native-static-libs or native-static-deps? The flag is not consistent.

I guess it has been merged already. I tried today with native-static-deps. but It failed.

[kornelski]

BTW, I'm having doubts about this solution. I'm wondering whether printing a note: … is the right approach for this at all.

Perhaps this information should be written to a file in the same directory as the static library? This way users wouldn't have to parse rustc/cargo output, and the library could be used properly even by tools that didn't observe the compilation.

I've started discussion topic here: https://internals.rust-lang.org/t/exposing-static-lib-dependencies/5925

[mssun]

@pornel I did have an issue on passing this flags to cargo (rust-lang/cargo#4510)

[mssun]

@pornel Also, I still didn't manage to get this flag work.

If I understand it correctly, the following compilation command should output bz2 right?

$ rustc main.rs -L native=. -l static=bz2 --print native-static-libs

But there is nothing in the stdout.

[kornelski]

@mssun No, this doesn't just echo the flags. This specifies flags non-Rust build must use when you create a static library (i.e. it's for --crate-type=staticlib only).

[mssun]

@pornel OK, I get it. Thank you so much.

[Michael-F-Bryan]

Is there some way to hide these messages altogether? The note explicitly says "this list will not be printed by default", yet every time I compile it's printed (rustc 1.23.0-nightly (bd0e45a32 2017-11-06)).

Combine that with cargo-watch to recompile every time you save and that ends up spamming your terminal with a lot of unnecessary noise... It'd be nice if I could set some environment variable to acknowledge that you may need to add the static libs and prevent the message from printing.