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.

Sign up for GitHub

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

Jump to bottom

Filter libtool warning about table of contents #15325

Closed
wants to merge 2 commits into from
Closed

Filter libtool warning about table of contents #15325

wants to merge 2 commits into from

Conversation

cpsauer
Copy link
Contributor

@cpsauer cpsauer commented Apr 23, 2022 •
edited
Loading

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

@cpsauer
Filter libtool warning about table of contents
3e9ab96 
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
@cpsauer cpsauer mentioned this pull request Apr 23, 2022
Closed
@cpsauer
Copy link
Contributor Author

cpsauer commented Apr 23, 2022

(Tagging @keith, since this came out of our discussion.)

keith
keith reviewed Apr 23, 2022
View reviewed changes
tools/objc/libtool.sh
Comment on lines +38 to +39
2> >(grep -v "the table of contents is empty (no object file members in the"`
`" library define global symbols)$" >&2)
Copy link
Member

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?

Copy link
Member

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

Copy link
Contributor Author

@cpsauer cpsauer Apr 23, 2022 •
edited
Loading

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.)

Copy link
Contributor Author

@cpsauer cpsauer Apr 23, 2022 •
edited
Loading

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.)

Copy link
Member

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?

Copy link
Contributor Author

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() {
Copy link
Member

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)

Copy link
Contributor Author

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?

Copy link
Member

Choose a reason for hiding this comment

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

sounds good

@cpsauer
Copy link
Contributor Author

cpsauer commented Apr 23, 2022 •
edited
Loading

All checks are green and pass on the first round. I'll make that change libtool -> invoke_libtool presently.

Are we all good to go, then, @keith?

Thanks for being so lightning fast and always a delight to work with.

@cpsauer
libtool -> invoke_libtool
a5a8955 
In response to @keith's good feedback.
keith
keith approved these changes Apr 23, 2022
View reviewed changes
Copy link
Member

@keith keith left a 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

@sgowroji sgowroji added team-Rules-CPP Issues for C++ rules awaiting-review PR is awaiting review from an assigned reviewer labels Apr 25, 2022
@sgowroji sgowroji requested a review from oquenchil April 25, 2022 04:10
oquenchil
oquenchil approved these changes May 5, 2022
View reviewed changes
Copy link
Contributor

@oquenchil oquenchil left a comment

Choose a reason for hiding this comment

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

LGTM

@cpsauer
Copy link
Contributor Author

cpsauer commented May 5, 2022

Wahoo! Thanks @oquenchil x2

@bazel-io bazel-io closed this in fa1081c May 5, 2022
@brentleyjones
Copy link
Contributor

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label May 5, 2022
@cpsauer
Copy link
Contributor Author

cpsauer commented May 5, 2022

@brentleyjones, could I ask what the tags, like yours above, mean? I keep seeing them and am curious :)

@brentleyjones
Copy link
Contributor

@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.

@cpsauer
Copy link
Contributor Author

cpsauer commented May 5, 2022

Got it! Thanks for a great explanation, @brentleyjones. Honored to have made your cut :)

@ckolli5
Copy link

ckolli5 commented May 9, 2022

@bazel-io fork 5.2.0

@bazel-io bazel-io mentioned this pull request May 9, 2022
Closed
@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label May 9, 2022
@ckolli5 ckolli5 mentioned this pull request May 10, 2022
Merged
ckolli5 added a commit that referenced this pull request May 10, 2022
@ckolli5 @cpsauer
Filter libtool warning about table of contents (#15448)
7c52cb6 
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 <cpsauer@users.noreply.github.com>
meteorcloudy pushed a commit that referenced this pull request May 10, 2022
@ckolli5 @cpsauer @meteorcloudy
Filter libtool warning about table of contents (#15448)
027e9b2 
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 <cpsauer@users.noreply.github.com>
@ShreeM01 ShreeM01 removed the awaiting-review PR is awaiting review from an assigned reviewer label Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keith keith keith approved these changes

@oquenchil oquenchil oquenchil approved these changes

Assignees
No one assigned
Labels
team-Rules-CPP Issues for C++ rules
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cc_library warning: file has no symbols
8 participants
@cpsauer @brentleyjones @ckolli5 @keith @oquenchil @bazel-io @sgowroji @ShreeM01