-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Filter libtool warning about table of contents #15325
Conversation
The warning is unlikely to indicate a real issue, and Bazel already silences parallel warnings with `-no_warning_for_no_symbols`. For more context, see bazelbuild#4057
(Tagging @keith, since this came out of our discussion.) |
2> >(grep -v "the table of contents is empty (no object file members in the"` | ||
`" library define global symbols)$" >&2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this grep not matching cause an unexpected failure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you could also be less strict here if you wanted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When running, I'm seeing things succeed fine when the grep doesn't match. Does that resolve the concern?
(I think your concern is a non-zero exit code from grep leading to a non-zero exit code of the script.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I kinda like the full thing to self-document the warning, but can remove if you want.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that resolves the concern, I was worried grep would still error. it looks like it's because of the -v
that it doesn't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I think so. I was concerned about that, too, when writing
tools/objc/libtool.sh
Outdated
@@ -32,15 +32,22 @@ if [ -z ${MY_LOCATION+x} ]; then | |||
fi | |||
fi | |||
|
|||
WRAPPER="${MY_LOCATION}/xcrunwrapper.sh" | |||
function libtool() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably worth naming this something slightly different to avoid confusion that we're calling libtool
directly, maybe libtool_func
(or ideally something better)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would invoke_libtool
be okay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good
All checks are green and pass on the first round. I'll make that change Are we all good to go, then, @keith? Thanks for being so lightning fast and always a delight to work with. |
In response to @keith's good feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep I think this is safe to filter out! Thanks for the positivity! Someone from google will still have to review and land
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Wahoo! Thanks @oquenchil x2 |
@bazel-io flag |
@brentleyjones, could I ask what the tags, like yours above, mean? I keep seeing them and am curious :) |
@cpsauer I'm flagging them as desired to get into 5.2. If the release manager agrees they do the fork command to create an issue to track the inclusion. After that issue is made someone (preferably the author) can make a cherry-pick PR. I think now that the branch exists the author can just make the PR though. Since I might not be able to make the PR immediately, I like to flag ones I think would be great candidates for the next release. |
Got it! Thanks for a great explanation, @brentleyjones. Honored to have made your cut :) |
@bazel-io fork 5.2.0 |
The warning is unlikely to indicate a real issue, and Bazel already silences parallel warnings with `-no_warning_for_no_symbols`. For more context, see #4057 Fixes #4057 Closes #15325. PiperOrigin-RevId: 446659148 Co-authored-by: Christopher Sauer <[email protected]>
The warning is unlikely to indicate a real issue, and Bazel already silences parallel warnings with `-no_warning_for_no_symbols`. For more context, see #4057 Fixes #4057 Closes #15325. PiperOrigin-RevId: 446659148 Co-authored-by: Christopher Sauer <[email protected]>
The warning is unlikely to indicate a real issue, and Bazel already silences parallel warnings with
-no_warning_for_no_symbols
.For more context, see #4057
Fixes #4057