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: respect error_format config #2166

Merged
merged 3 commits into from
Sep 28, 2023

Conversation

goffrie
Copy link
Contributor

@goffrie goffrie commented Sep 21, 2023

This makes it easier to make a test ensuring rustdoc doesn't emit warnings.

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.

Hi, thanks for the PR! Can you provide a bit more context here as well as a unit test of sorts?

@goffrie
Copy link
Contributor Author

goffrie commented Sep 21, 2023

The context is that we have automation using the output of rustdoc that uses json error format. We used to use extra rustc flags to pass --error-format=json but that broke since #1275 now unconditionally passes --error-format=human to rustdoc (and it's an error to specify error-format twice on the rustc/rustdoc command line). I figured that the cleaner solution would be to respect the global error_format configuration instead of trying to revert to the old behaviour of not passing anything.

I'll see when I can find some time to work on a unit test, but this isn't urgent to merge since we run a patched version.

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.

In https://github.com/bazelbuild/rules_rust/blob/0.27.0/test/unit/rustdoc/rustdoc_unit_test.bzl you should be able to add a test that hard codes some value for //:error_format that you should be able to assert made it's way into the resulting action.

Does that seem reasonable?

@goffrie
Copy link
Contributor Author

goffrie commented Sep 27, 2023

Thanks for the pointer! I've added a test - is that what you had in mind?

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.

This is perfect, thank you!

@UebelAndre UebelAndre merged commit ac5aa1e into bazelbuild:main Sep 28, 2023
2 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.

2 participants