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 experimental cc_static_library rule #16954

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Dec 7, 2022

RELNOTES: The new cc_static_library rule produces a static library that bundles given targets and all their transitive dependencies. It has to be enabled via --experimental_cc_static_library.

Implements https://docs.google.com/document/d/1jN0LUmp6_-rV9_f-Chx-Cs6t_5iOm3fziOklzBGjGIg/edit

Fixes #1920

@fmeum fmeum force-pushed the 1920-cc-static-library branch 11 times, most recently from fbcb331 to a42ff75 Compare December 8, 2022 16:57
@fmeum fmeum force-pushed the 1920-cc-static-library branch 2 times, most recently from 0f9622b to f260501 Compare April 4, 2023 13:06
@fmeum fmeum force-pushed the 1920-cc-static-library branch 5 times, most recently from 799d1e8 to dce3fe7 Compare April 26, 2023 14:53
@aminya
Copy link

aminya commented Nov 6, 2023

@fmeum I made a pull request for your fork. Could you merge fmeum#1 to this branch so that it is updated? It is behind 2114 commits.

#1920

@aminya aminya mentioned this pull request Nov 6, 2023
@aminya
Copy link

aminya commented Jan 3, 2024

@fmeum Any update on my comments?

#16954 (comment)

@Algomorph
Copy link

Algomorph commented Mar 7, 2024

@aminya and @fmeum any updates on the status / timeline for this? We (as in, my company) are considering moving off Bazel partially or completely and lack of transitive static libraries is one of the major factors in making such decisions. We need to understand our timeline and resource allocation should be and that hinges on this MR + issue thread.

@fmeum
Copy link
Collaborator Author

fmeum commented Mar 8, 2024

Sorry for the lack of response, other efforts kept needing priority over the one. I'm planning to get back to it in April.

@Algomorph If you really just want to build transitive static libraries, then you could really use any of the Starlark rules proposed on the original issue without being blocked on me. Do you have any special requirements that only my design would address?

@Algomorph
Copy link

Algomorph commented Mar 8, 2024

@fmeum thanks for getting back to me so promptly and sharing the plan. April will probably work for us. I got the general sense from reading through the thread that people are running into issues with the proposed rules, but granted, I haven't personally tried any of them yet. We will review further at our next planning session and reevaluate, perhaps even assist -- if we can allocate the resources for it.

@aminya
Copy link

aminya commented Mar 8, 2024

We (as in, my company) are considering moving off Bazel partially or completely and lack of transitive static libraries

Unfortunately, one of my dependencies uses Bazel, and I am stuck in the walled garden of Bazel without a way out. Having static libraries is the only way that allows us to build the dependency in isolation and use it in other projects without the fragile and bulky alternative of whole archive shared libraries.
RobotLocomotion/drake#16882

Do you have any special requirements that only my design would address?

I tried all the proposed rules, and only this PR seems to partially work for me except for the errors I mentioned here
#16954 (comment)

I'm planning to get back to it in April.

@fmeum
Could you at least update the branch via fmeum#1?

@fmeum
Copy link
Collaborator Author

fmeum commented Mar 8, 2024

Could you at least update the branch via fmeum#1?

@aminya Sorry, I didn't want to merge it directly due to its size and then forgot about it. I just rebased the current PR, please let me know if anything broke due to that.

# (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=35201).
DUPLICATE_SYMBOLS=$(
"%{nm}" -A -g -P %{nm_extra_args} "$1" |
sed -E -e 's/.*\[([^][]+)\]: (.+) ([A-TX-Z]) [a-f0-9]+ [a-f0-9]+/\1: \3 \2/g' -e t -e d |
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this sed be simplified?

My suggestions:

  • the last part [a-f0-9]+ [a-f0-9]+ of the matcher is not used in the output, so it could be removed
  • \[([^][]+)\] is quite complex, is the occurence of [ or ] in any other strings even possible? if not, then do just \[(.*)\]. It's easier to understand.
  • shouldn't sed -E -e <> -e t -e d do the same as sed -E <>? If that's the case remove -es.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. We need to match this part so that it is consumed and replaced, otherwise it would end up in the sed output. I could use another wildcard, but using two wildcards around a part we need to match exactly seemed too brittle to me.
  2. It may appear in file names, which is why i wanted this match to be as precise as possible. It is only relevant in edge cases though (and maybe not even there).
  3. t and d additionally drop unmatched lines, which is required to drop some symbol types that should be ignored. Default sed doesn't do that as far as I know.

src/main/starlark/builtins_bzl/common/cc/cc_helper.bzl Outdated Show resolved Hide resolved
@fmeum fmeum requested review from comius and removed request for a team, oquenchil, gregestren, fweikert, Wyverald and meteorcloudy July 5, 2024 21:10
@fmeum fmeum added awaiting-review PR is awaiting review from an assigned reviewer and removed awaiting-user-response Awaiting a response from the author labels Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review PR is awaiting review from an assigned reviewer team-Rules-CPP Issues for C++ rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule to build a fully static library
6 participants