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

Print warnings when allowlist doesn't match anything #2252

Merged
merged 5 commits into from
Aug 18, 2022

Conversation

pvdrz
Copy link
Contributor

@pvdrz pvdrz commented Aug 10, 2022

This PR introduces a way to store warning messages inside Bindings so they can be emitted later. This allows bindgen to use cargo:warnining=MESSAGE in build scripts to show warnings and still produce warning log entries (which means this PR would fix #2167).

We still have some questions about this that we would like to resolve before promoting this to a non-draft PR:

  • Should we emit log entries AND build scripts warnings at the same time? or should we favor one over the other?
    Apparently cargo won't print warnings for crates.io dependencies unless -vv is passed (Mechanism to unconditionally emit warnings from build scripts cargo#3777). I decided to float the warning messages all the way up and add a Bindings::take_warnings method so the user can print them directly in the build script. I also added a print_warnings macro to ease this process.

  • Is there any functionality in the test suite to capture stdout/stderr to be able to test that the warnings are being printed correctly?

cc @amanjeev

Edit: As pointed out by @kulp (thanks!), the warning log entries are still emitted..

@pvdrz pvdrz marked this pull request as ready for review August 12, 2022 18:36
@pvdrz pvdrz changed the title Store warnings and emit them later Print warnings when allowlist doesn't match anything Aug 12, 2022
Copy link
Member

@kulp kulp left a comment

Choose a reason for hiding this comment

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

This seems reasonable to me, and if @emilio does not see a problem maybe we can merge it soon. You could rebase it down to a few commits that all pass the tests, or we could squash it when merging.

@pvdrz pvdrz force-pushed the allowlist-warnings branch from 937d294 to 731da2f Compare August 12, 2022 20:40
@pvdrz
Copy link
Contributor Author

pvdrz commented Aug 12, 2022

This seems reasonable to me, and if @emilio does not see a problem maybe we can merge it soon. You could rebase it down to a few commits that all pass the tests, or we could squash it when merging.

Thanks for your review :) I addressed your comments and rebased the PR so all commits pass the tests individually

Copy link
Contributor

@emilio emilio left a comment

Choose a reason for hiding this comment

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

Looks sensible, but I think we can do a couple simplifications.

@pvdrz pvdrz requested a review from emilio August 16, 2022 17:28
@pvdrz
Copy link
Contributor Author

pvdrz commented Aug 16, 2022

@emilio I've addressed your comments. I'll squash them after your next review so you can still see the diff

Copy link
Contributor

@emilio emilio left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@emilio emilio merged commit ebb1ce9 into rust-lang:master Aug 18, 2022
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.

allowlist entries like allowlist_function show no warning when they don't match anything.
3 participants