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

Add support for libtool over ar #309

Closed
wants to merge 1 commit into from

Conversation

keith
Copy link
Member

@keith keith commented Aug 27, 2019

On macOS libtool is preferred for creating static archives over ar. If
libtool is set as the static linker, we need to pass different flags.

Fixes bazelbuild/bazel#9258

On macOS libtool is preferred for creating static archives over ar. If
libtool is set as the static linker, we need to pass different flags.
@meteorcloudy
Copy link
Contributor

@irengrig Can you review this? It's need to make downstream green again. ;)

@buchgr
Copy link

buchgr commented Aug 30, 2019

Irina is out of office for the next two weeks.

@meteorcloudy
Copy link
Contributor

@hlopko @oquenchil Does this PR looks safe to you? We need some CMake/C++ expert.

@irengrig
Copy link
Contributor

irengrig commented Sep 1, 2019

Hi, sorry for the late response, I am having a vacation (and turned off the notifications).
Thank you so much for working on this problem and submitting the pull request!

The documentation for the used flags is here: https://cmake.org/cmake/help/v3.15/variable/CMAKE_LANG_ARCHIVE_CREATE.html?highlight=cmake_%20lang%20_archive_create
As you can see, there are also 2 more related flags, for appending to the static archive, and finishing the process.
I guess we could set those into empty values, if we believe that <>_CREATE flag is enough.

The code looks reasonable. The reference to libtool cli: https://www.gnu.org/software/libtool/manual/html_node/Link-mode.html#Link-mode
However, I am not sure about <LINK_FLAGS>, whether the value will be compatible.

Also, the CI has passed, but it runs against released Bazel, not against the master branch.
It would be cool if @meteorcloudy could check that the fix indeed helps, and then merge this PR.

Thank you all again!

@keith
Copy link
Member Author

keith commented Sep 1, 2019

Hmm I guess it's up to the cmake configs to know if those other flags are necessary. I can try to add the appropriate ones for those (although I'm not sure libtool supports those).

Good point about LINK_FLAGS. Maybe we should remove those. Alternatively do you know if there's a way to make cmake smarter about the static archiver it's using? AFAICT there is not.

I think we should try this with bazel HEAD against envoy since it uses a ton of cmake libraries.

@irengrig
Copy link
Contributor

irengrig commented Sep 1, 2019

Hmm I guess it's up to the cmake configs to know if those other flags are necessary. I can try to add the appropriate ones for those (although I'm not sure libtool supports those).

Well, I can google :) I think I rather add a test in a separate PR that the user can still pass the value for those flags to override the value that we put automatically, so that there is a workaround for any situation.

Good point about LINK_FLAGS. Maybe we should remove those. Alternatively do you know if there's a way to make cmake smarter about the static archiver it's using? AFAICT there is not.

Probably, each library can have this in CMakeLists.txt, but not outside the scripts. We can only manipulate the flags/environment variables.

I think we should try this with bazel HEAD against envoy since it uses a ton of cmake libraries.
Good idea!

@meteorcloudy
Copy link
Contributor

I tested locally, this change doesn't look like to fix the issue. You can get a Bazel binary from HEAD at the following url to reproduce.

https://storage.googleapis.com/bazel-builds/artifacts/macos/20b056ae8506e489933cf236b9401cddad90afac/bazel

irengrig added a commit to irengrig/rules_foreign_cc that referenced this pull request Sep 9, 2019
@keith
Copy link
Member Author

keith commented Sep 9, 2019

#315

@keith keith closed this Sep 9, 2019
@keith keith deleted the ks/libtool-over-ar branch September 9, 2019 16:29
@irengrig
Copy link
Contributor

@keith thank you again for working on it! I used the changes in this OR as the basis for the fix in #315.

Your fix was actually correct, the problem was in on eof the rules_foreign_cc tests with configure rule.
However, I changed the fix to be "less hard coded", to get the command line for linking from Bazel's cc_common interface.
That is the reason I submitted my own PR, based on your fix.

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.

rules_foreign_cc broken in downstream
4 participants