Skip to content
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

fix rustdoc: omit link flags for rustdoc #2467

Merged
merged 6 commits into from
Feb 21, 2024

Conversation

bgeron
Copy link
Contributor

@bgeron bgeron commented Feb 8, 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.

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
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
Copy link
Contributor Author

bgeron commented Feb 16, 2024

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.

Copy link
Collaborator

@krasimirgg krasimirgg left a comment

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
3 checks passed
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.

None yet

2 participants