Skip to content

fix rustdoc: omit link flags for rustdoc - #2467

Merged
krasimirgg merged 6 commits into
bazelbuild:mainfrom
bgeron:rustdoc-without-linker-flags
Feb 21, 2024
Merged

fix rustdoc: omit link flags for rustdoc#2467
krasimirgg merged 6 commits into
bazelbuild:mainfrom
bgeron:rustdoc-without-linker-flags

Conversation

@bgeron

@bgeron bgeron commented Feb 8, 2024

Copy link
Copy Markdown
Contributor

In our project, rust_doc fails because it is generating -l arguments to
rustdoc, and rustdoc does not support -l arguments.

In my understanding, these arguments are only relevant for calling the
linker at some point, and it won't be called for rustdoc anyway.

With this PR, our project can build with rust_doc.

In our project, rust_doc fails because it is generating -l arguments to
rustdoc, and rustdoc does not support -l arguments.

In my understanding, these arguments are only relevant for calling the
linker at some point, and it won't be called for rustdoc anyway.
@krasimirgg
krasimirgg self-requested a review February 12, 2024 15:26
@krasimirgg

Copy link
Copy Markdown
Collaborator

There are some CI failures that suggest that for some actions in rustdoc mode at least, the linking step or something in the if ("link" in emit and crate_info.type not in ["rlib", "lib"]): section is necessary.
It seems that the rustdoc param should already account for not invoking linking since 9442aed. Naively, could you adapt the code to filter out the necessary -l flags depending on the existing rustdoc param instead?

Also could you add an example integration test about the failure you're seeing.

I couldn't find a build configuration with nonempty `cc_toolchain.dynamic_runtime_lib` or `cc_toolchain.static_runtime_lib`, so the new filter there is not covered by unit tests.
@bgeron bgeron changed the title fix rustdoc: remove functionality for adding linker flags fix rustdoc: omit link flags for rustdoc Feb 16, 2024
@bgeron

bgeron commented Feb 16, 2024

Copy link
Copy Markdown
Contributor Author

Thanks for looking at this! I adapted the code correspondingly, to omit -l flags in a more fine-grained way. I think the point of that flag is rustdoc will still compile binaries for running doctests. I don't understand it 100% though, but this patch is much more minimal, the tests are now passing, and our repo is building.

Most of the new patch is just threading variables through. But we now omit -l flags in three circumstances that before all failed for different reasons:

  • rustdoc for bin, linking to cc_library. Before, we only had rustdoc for a lib that links to a cc_library.
  • Rust build script emitting -llibrary.
  • Nonempty cc_toolchain.dynamic_runtime_lib or cc_toolchain.static_runtime_lib.

I made regression tests for the first two. But I didn't know how to for the third one; common platforms seem to have empty cc_toolchain.dynamic_runtime_lib and cc_toolchain.static_runtime_lib. I think it's okay to not cover this unlikely case with a test for now.

@krasimirgg krasimirgg left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thank you!

@krasimirgg
krasimirgg added this pull request to the merge queue Feb 21, 2024
Merged via the queue into bazelbuild:main with commit e404e51 Feb 21, 2024
qtica added a commit to qtica/rules_rust that referenced this pull request Apr 1, 2024
In our project, rust_doc fails because it is generating `-l` arguments
to
rustdoc, and rustdoc does not support `-l` arguments.

In my understanding, these arguments are only relevant for calling the
linker at some point, and it won't be called for rustdoc anyway.

With this PR, our project can build with rust_doc.

---------

Co-authored-by: Krasimir Georgiev <krasimir@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants