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

rustdoc_test: Apply prefix stripping to proc_macro dependencies. #1952

Merged
merged 3 commits into from
Sep 10, 2024

Conversation

konkers
Copy link
Contributor

@konkers konkers commented May 2, 2023

Without stripping the prefix, rustdoc can not find the proc macro shared library.

It's not entirely clear to me why //test/unit/rustdoc:lib_with_proc_macro passes without this. When I run bazel test //test/unit/rustdoc:rustdoc_test_suite --subcommands I don't see any "action 'Generating Rustdoc test runner for.." lines so perhaps that path is not tested?

Copy link
Collaborator

@UebelAndre UebelAndre left a comment

Choose a reason for hiding this comment

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

Thanks! Seems reasonable. I can look at the existing test but what would fail before this change and can that be added as a regression test?

konkers added a commit to konkers/bazel-rustdoctest-proc-macro-failure that referenced this pull request Jul 10, 2023
@konkers
Copy link
Contributor Author

konkers commented Jul 10, 2023

Thanks for the look. I created an stripped down example repo to illustrate the failure: https://github.com/konkers/bazel-rustdoctest-proc-macro-failure

@konkers
Copy link
Contributor Author

konkers commented Jul 10, 2023

I noticed in #1782 an additional change is also added:

        root = proc_macro_dep.crate_info.output.root.path
        if not root in roots:
            roots.append(root)

Perhaps that should be added to this PR.

konkers added a commit to konkers/bazel-rustdoctest-proc-macro-failure that referenced this pull request Jul 10, 2023
@armandomontanez
Copy link
Contributor

I spun up a test that covers this at konkers#1

@konkers
Copy link
Contributor Author

konkers commented Aug 29, 2024

I spun up a test that covers this at konkers#1

Merged into PR

@konkers konkers force-pushed the pr/rustdoc_test_proc_macro branch from 667d648 to 4503ed8 Compare August 29, 2024 23:02
@konkers konkers requested a review from UebelAndre August 29, 2024 23:14
konkers and others added 2 commits September 5, 2024 12:36
Without stripping the prefix, rustdoc can not find the proc macro
shared library.
Adds a test that reproduces the bug mentioned in rules_rust#1952.
@konkers konkers force-pushed the pr/rustdoc_test_proc_macro branch from b2ba394 to 3e6bb71 Compare September 5, 2024 19:43
@armandomontanez
Copy link
Contributor

@UebelAndre this is ready for another look

@krasimirgg
Copy link
Collaborator

@UebelAndre this is looking reasonable now, with a regression test in place.
@konkers just to clarify:

I noticed in #1782 an additional change is also added

This PR is good as-is, and doesn't require this?

Copy link
Collaborator

@UebelAndre UebelAndre left a comment

Choose a reason for hiding this comment

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

Thank you!

@krasimirgg krasimirgg added this pull request to the merge queue Sep 10, 2024
Merged via the queue into bazelbuild:main with commit 9ad0b00 Sep 10, 2024
4 checks passed
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.

4 participants