Unexpected issue in Ninja command output for rlib artifacts

[David Turner]

I would like to add a new .gn variable named add_rlib_link_libraries to unbreak the Fuchsia build, which cannot use upstream GN at the moment.

Because recent changes to the way the Ninja command for Rlib artifacts is generated break our remote builds, so we are stuck with an older version of GN right now :-/

I have uploaded https://gn-review.googlesource.com/c/gn/+/15760 which contains more information about the problem, but in a nutshell:

1) The Ninja command generated by Ninja for Rlib artifacts was recently changed in [1] to omit linked libraries from the Ninja command

2) This broke Fuchsia local builds, so a fix was submitted in [2] to add missing -Cnative=<libdir> arguments which are necessary for the rustc compiler to find now-implicit linked libraries.

3) Unfortunately, our remote builds are still broken, because we process the Ninja command to find all input files to send to remote builders. I.e. we really need the -Clink-arg=libfoo.a arguments to appear on the command line.

So the new add_rlib_link_libraries variable would restore the old behavior. My CL sets its default value to true, in order to be able to roll the newest GN into the Fuchsia tree, then we can add `add_rlib_link_libraries = true` to the Fuchsia .gn file, then the GN default value can be changed to false.

Let me know if this sounds too crazy, or if you have a better idea on how to solve this.

Thanks

- Digit (for the Fuchsia team)

[Aaron Wood]

I'd like to stress the urgency on this because the inability to roll a newer GN version into Fuchsia also blocks my own work (in Fuchsia) that is blocked on https://gn-review.googlesource.com/c/gn/+/15720 landing.

--
To unsubscribe from this group and stop receiving emails from it, send an email to gn-dev+un...@chromium.org.

[Dirk Pranke]

It seems unlikely to me that the intent of the original CL was to introduce a breaking change that we would have to address with a configuration variable. I would have hoped that if it was, we would've had some public discussion of this on the list first. 

If I am correct and introducing a breaking change was not the intent, I think we should revert the original change and then have the discussion about what to do.

Unless I am mistaken, we don't have any configuration options that affect the build file generation right now. We have historically tried quite hard to avoid such things. It may be that Rust is presenting us with some heretofore unseen and unavoidable challenges, but I think we should assess that when we are not feeling rushed by someone being broken. 

-- Dirk

[Dirk Pranke]

Also, in looking at this a bit more, it looked like one part of the change (the `{{rustdeps}}` part) might've been more optional the other (the `-Lframework=` part), so if a full revert would break something else, maybe we can just do a partial revert?

-- Dirk

[David Turner]

On Wed, Jul 26, 2023 at 5:32 AM Dirk Pranke <dpr...@google.com> wrote:

Also, in looking at this a bit more, it looked like one part of the change (the `{{rustdeps}}` part) might've been more optional the other (the `-Lframework=` part), so if a full revert would break something else, maybe we can just do a partial revert?

Yes, from the original bug, it looks like the breaking CL tried to fix two separate issues, so a full revert would re-introduce a problem with framework_dirs.
I have uploaded another patchset on the CL that simply restores the linker arguments into rlib Ninja commands, but keeps the framework dirs fix
https://gn-review.git.corp.google.com/c/gn/+/15760/3 

Please take a look, Takuto already accepted the second patchset, but I would prefer if everyone is ok with the third one.

[Dana Jansens]

Yeah the removal of the link-args from rlib compilation steps (similar to source set compilation for C++) was something that helped me debug by removing "pointless" noise from the command line.

The noise is not pointless however for Fuchsia's RBE. Given the strong distaste for config options I can stomach growing the command lines back unconditionally, there's no side effects I am aware of outside of much longer command lines for humans to have to try to parse when debugging stuff.

[David Turner]

Agreed. The CL has now been submitted and I confirm that the newest GN binary was successfully rolled into our source tree. Thanks everyone.

[Dirk Pranke]

Thanks for clearing it up!

-- Dirk

[Dana Jansens]

On Wed, Jul 26, 2023 at 3:59 PM Dirk Pranke <dpr...@google.com> wrote:

Thanks for clearing it up!

+1, sorry for the pain!

[Tyler Mandry]

Sorry, I did not realize this would break us. Is this about rustc looking for native libs when there are #[link] directives in a crate? If so then I think we should be able to get away with only passing "direct" native deps (for some definition of direct)  on the command line, rather than all transitive ones.

I'd also prefer if we could remove the -Lnative option again, since search directories have caused a lot of pain in my experience.