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

Rule to build a fully static library #1920

Open
asimshankar opened this issue Oct 10, 2016 · 88 comments · May be fixed by #16954
Open

Rule to build a fully static library #1920

asimshankar opened this issue Oct 10, 2016 · 88 comments · May be fixed by #16954
Assignees
Labels
help wanted Someone outside the Bazel team could own this P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Rules-CPP Issues for C++ rules type: feature request

Comments

@asimshankar
Copy link
Contributor

Among the multiple "release" outputs of my project, I'd like to include a single static library, which packages the objects of all dependencies into it. However, from what I can tell, bazel cannot build such targets.

The following target produces a single .so containing all symbols from //product/foo and //product/bar.:

cc_binary(
  name = "libproduct.so",
  linkshared = 1,
  deps = [
     "//product/foo",
     "//product/bar",
  ],
)

I believe there is no way to generate a similar static library (libproduct.a).
Is that something that will be of interest to add?

(#492 seems somewhat related, but not exactly)

@steven-johnson
Copy link
Contributor

+1; my project has similar hard requirement and similar problem.

@hermione521
Copy link
Contributor

Is this what you are looking for?
https://www.bazel.io/versions/master/docs/be/c-cpp.html#cc_binary.linkstatic

@asimshankar
Copy link
Contributor Author

@hermione521 : I don't think so.

If I try to create the .a using a cc_binary rule like so:

cc_binary(
  name = "libproduct.a",
  linkstatic = 1,
  deps = [
    "//product/foo",
    "//product/bar",
  ],
)

Then the output isn't an object archive.

And my understanding of linkstatic=1 in cc_library rules is that it means that the symbols in the library will be statically linked into the binary that depends on it, not that the library itself will include symbols of its transitive dependencies.

@hermione521
Copy link
Contributor

Ah, I'm sorry. Let's ask @lberki

@lberki
Copy link
Contributor

lberki commented Oct 13, 2016

No, we don't support that at the moment :( We were considering implementing something like that with @mhlopko , but we have other priorities at the moment.

A workaround would be to use a genrule to package the static library like this:

cc_library(name="a")
cc_library(name="b")
genrule(name="g", srcs=[":a", ":b"], cmd="$(AR) $(locations :a) $(locations :b)")

I took a bit of artistic freedom since $(locations) will return both a static and a dynamic library and that's not exactly how ar should be called, but it should work as long as you don't need symbols from transitive dependencies.

@asimshankar
Copy link
Contributor Author

Thanks, though I do want to include symbols from transitive dependencies
(for example, building a static library for tensorflow (today, there is a shared library))

@steven-johnson
Copy link
Contributor

This is a pretty major problem for us, too. I'll try the genrule workaround suggested above, but it makes me mighty nervous.

@ulfjack
Copy link
Contributor

ulfjack commented Oct 14, 2016

cc_binary with .so is currently special, but cc_binary doesn't handle .a. One of the downsides of special-casing the specific filename is that it's not cross-platform compatible. One option would be to introduce a new cc_export rule with a type={DYNAMIC,STATIC} attribute that links the transitive closure into a dynamic or static library respectively.

I'm a bit concerned about the increased likelihood of accidental ODR violations, which requires a globally consistent partition into cc_export rules. That's not a problem per se, except when different people want different partitions. Internally, we have everything in a single repository, and we actually have requests to support multiple different overlapping partitioning schemes. Externally, you usually don't have a choice. Whatever upstream decides is what you get.

@lberki
Copy link
Contributor

lberki commented Oct 14, 2016

@ulfjack: there are a bunch of options. One is a cc_export rule you suggested, another would be an output group on regular cc_binary rules that would put all the .o files in its transitive closure into a single .a .

I was thinking that the partitioning should be the responsibility of whoever is writing the cc_binary rule(s). I'm not particularly worried about coordination problems if there are multiple such rules with multiple authors because people should know what symbols a library they use contains and library authors should know what symbols they want to export. Do you think that's too optimistic?

@ulfjack
Copy link
Contributor

ulfjack commented Oct 14, 2016

I think it's very easy to mess up the dependencies. Consider for example a workspace that has two cc_library and two cc_export rules, with each of the cc_export rules depending on exactly one library. Now an owner of one of the lower-level libraries isn't aware of the cc_export going on in some completely different part of the workspace, and adds a dependency on the other library. Everything keeps working just fine, except that the application blows up at runtime, because it contains two copies of some symbols. They may not be exported symbols, so neither the static linker nor the dynamic linker would be able to check.

@lberki
Copy link
Contributor

lberki commented Oct 14, 2016

I understand that. But who is in a better situation to sort these things out if not the owner of the top-level rule?

@ulfjack
Copy link
Contributor

ulfjack commented Oct 14, 2016

I'm not saying that they aren't. But I think it's a problem that it's a runtime error rather than a compile-time error. If we can make it a compile-time error, then I think that may be an acceptable solution for now. If the burden on the top-level owners is too large, or if it causes users to create duplicate (multiplicate) rule hierarchies, then we can still see if we can come up with a better solution. We certainly need to document it carefully because it's easy to shoot yourself in the foot.

I can think of two ways of making it a compile-time error. 1. Use constraints to annotate every rule that goes into a specific cc_export rule. 2. Look for all cc_export rules in the transitive closure of a cc_binary or cc_test and check that their corresponding transitive closures are non-overlapping.

2 seems like it could be expensive; 1 would be optional.

I'd like to keep the option open of statically linking everything even if there are intermediate cc_export rules, so maybe cc_export isn't a good name for that. Maybe we should call it cc_package or cc_transitive_library. Then at the cc_binary level we can decide whether we cut the binary into individual .so's at the cc_transitive_library level or whether to link everything statically after all.

@ulfjack
Copy link
Contributor

ulfjack commented Oct 14, 2016

Consider that many debian packages come with both .so and .a which allows developers to decide after the fact whether to link a specific library statically or dynamically.

@steven-johnson
Copy link
Contributor

Related to my previous comment: I have put together a utility script for myself that does the necessary hackery with with the .a files to produce a statically-linked output for my purposes; unfortunately, this introduced me to #1740, in which a cc_library() has different outputs depending on compilation mode (in -c opt it also outputs a .pic.a library).

@ahyangyi
Copy link

@steven-johnson
I am the creator of issue #1740.
Yes, I also have some homebaked rule that builds a fully static shared library (and with extra functionalities such as linking with a version script).

Currently I use a hack: if a cc_library outputs at least some .pic.a library, I use them, otherwise I use all .a library. It somehow worked in all combinations of settings I encounter, so I do recommend you to do something else if you need it to work immediately.

However if we get better support from Official Bazel I can simplify things. You know, I am the sole maintainer of our Bazel workspace, since it contains too much cryptic workarounds that work around cryptic workarounds ...

@steven-johnson
Copy link
Contributor

if a cc_library outputs at least some .pic.a library, I use them, otherwise I use all .a library

Thanks, I'll give that a try. On the whole, Bazel is great, but the number of workarounds necessary for various outside-of-Google necessities of life is still disappointing.

@asimshankar
Copy link
Contributor Author

@lberki @ulfjack : Just wanted to check in on this. Would you guys have the bandwidth to address this soon?

@lberki
Copy link
Contributor

lberki commented Jan 30, 2017

@mhlopko is OOO for a long while, so I don't think we can get to this soon-ish :(

@hlopko
Copy link
Member

hlopko commented Feb 24, 2017

So I'm back :) We have no design ready for this (and other linking related requests), but I'll try to push this forward. We really have to polish our linking story. Sadly, I expect it will take a few weeks to fully implement considering my current load.

@lababidi
Copy link

@mhlopko how can we help? where should we begin?

@hlopko
Copy link
Member

hlopko commented Mar 23, 2017

I don't think there is anything you could do right now. I'm discussing the design internally and soon I'll publish a design doc to bazel-discuss. Then I'll start implementing.

@ulfjack
Copy link
Contributor

ulfjack commented Mar 23, 2017

How about publishing the doc now, and having the discussion on bazel-discuss?

@damienmg damienmg assigned hlopko and unassigned lberki Mar 30, 2017
@damienmg damienmg added P1 I'll work on this now. (Assignee required) type: feature request labels Mar 30, 2017
fmeum added a commit to fmeum/bazel that referenced this issue Sep 9, 2022
The behavior is analogous to that of Args.add_joined.

Work towards bazelbuild#1920
@shareefj
Copy link

shareefj commented Dec 7, 2022

Hi all,

I modified @Danvil's example to not use MRI scripts and hence support libs with "+" in the name. I'm using this to build a grpc++ static library.

https://gist.github.com/shareefj/4e314b16148fded3a8ec874e71b07143

@TheButlah
Copy link

Hi all,

I modified @Danvil's example to not use MRI scripts and hence support libs with "+" in the name. I'm using this to build a grpc++ static library.

https://gist.github.com/shareefj/4e314b16148fded3a8ec874e71b07143

Any interest in submitting that as a PR to bazel?

copybara-service bot pushed a commit that referenced this issue Dec 16, 2022
Returning a list of strings is needed to get correct `uniquify` semantics when a value may expand to multiple strings.

Work towards #1920

Closes #17008.

PiperOrigin-RevId: 495868960
Change-Id: I1f0ec560fd783846e916a07bb524dd2179f7c535
@fmeum
Copy link
Collaborator

fmeum commented Jan 9, 2023

I am working on this and will submit a design doc soon.

ShreeM01 added a commit that referenced this issue Jan 26, 2023
…17306)

Returning a list of strings is needed to get correct `uniquify` semantics when a value may expand to multiple strings.

Work towards #1920

Closes #17008.

PiperOrigin-RevId: 495868960
Change-Id: I1f0ec560fd783846e916a07bb524dd2179f7c535

Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im>
hvadehra pushed a commit that referenced this issue Feb 14, 2023
Returning a list of strings is needed to get correct `uniquify` semantics when a value may expand to multiple strings.

Work towards #1920

Closes #17008.

PiperOrigin-RevId: 495868960
Change-Id: I1f0ec560fd783846e916a07bb524dd2179f7c535
@fmeum
Copy link
Collaborator

fmeum commented Feb 24, 2023

The design is under review, feedback is welcome: https://docs.google.com/document/d/1jN0LUmp6_-rV9_f-Chx-Cs6t_5iOm3fziOklzBGjGIg/edit?usp=sharing

@derived-coder
Copy link

derived-coder commented Mar 24, 2023

Any update on this proposal? Are you somewhere blocked?

@fmeum
Copy link
Collaborator

fmeum commented Mar 24, 2023

I'm not blocked, I just need to find some time to update the proposal, taking the remaining comments into account. I had to shift priority to another design doc, but this one comes right after.

copybara-service bot pushed a commit that referenced this issue Apr 25, 2023
Includes an update of bazel_skylib to 1.3.0 as the (very old) version 1.0.3 wasn't compatible with rules_testing.

Work towards #1920

Closes #18182.

PiperOrigin-RevId: 527021012
Change-Id: I80ebff1a1aa656974ad6a1bb1809d8f919a2fa22
fweikert pushed a commit to fweikert/bazel that referenced this issue May 25, 2023
Includes an update of bazel_skylib to 1.3.0 as the (very old) version 1.0.3 wasn't compatible with rules_testing.

Work towards bazelbuild#1920

Closes bazelbuild#18182.

PiperOrigin-RevId: 527021012
Change-Id: I80ebff1a1aa656974ad6a1bb1809d8f919a2fa22
@louis-shao
Copy link

@shareefj I tried your rule, and I got an error on the ar command saying "malformed archive". If I change the ar options from rcT to rcs, the ar command succeeds, but when I try to link against the resulted .a file, it say no index. Do you guys have any idea? I'm running it on linux and I was using this method to compile the openxla library.

@cameron-martin
Copy link
Contributor

cameron-martin commented Oct 11, 2023

If I understand the proposal correctly, this archives the whole dependency graph beneath deps. This is fairly easily implementable in user-space so isn't that useful to me. What isn't easily implementable, since it requires usage of starlark in bazel that isn't exposed to the user, is a rule that selectively archives the dependency graph beneath it based on what isn't already included in other referenced static and shared libraries.

I added a comment to this effect on the design doc, but it looks like I missed the boat before it was approved.

@shareefj
Copy link

@louis-shao Sorry, I gave up on Bazel. Use CMake - much easier. :-)

@fmeum
Copy link
Collaborator

fmeum commented Oct 11, 2023

@cameron-martin Your comment wasn't missed and the proposal hasn't been finalized yet, I just got caught up in other projects that I believe are of higher priority right now. I am still planning to get back to it this year.

@fmeum
Copy link
Collaborator

fmeum commented Mar 18, 2024

I have a PR out with the full implementation of the proposal: #16954

If you can, please build a version of Bazel from this branch and test cc_static_library.

@joelberkeley
Copy link

joelberkeley commented Mar 30, 2024

I can't see mention in the proposal of collating the corresponding headers so I can compile against the static library. Will that be part of cc_static_library?

@jiawen
Copy link

jiawen commented Mar 30, 2024

I can't see mention in the proposal of collating the corresponding headers so I can compile against the static library. Will that be part of cc_static_library?

I imagine that's outside the scope of of this rule. The purpose of the rule is to correctly build a static library with the least amount of surprise given the legacy of how static libraries work and all the edge cases that come with transitive dependencies.

If you're talking about packaging, it seems straightforward to declare a filegroup of headers that you want to form the library's public interface.

@joelberkeley
Copy link

joelberkeley commented Mar 31, 2024

understood, though would having separate rules for archives and headers not create duplication, along with all the problems that come with that? For example, it would be easy to add a header to the filegroup without adding it to the archive. I'm trying to understand the purpose of cc_static_library without collating the associated headers. Apologies if I'm asking basic questions, I find this stuff pretty confusing

@fmeum
Copy link
Collaborator

fmeum commented Mar 31, 2024

@joelberkeley Could you explain how you would want cc_static_library to collate headers?

@joelberkeley
Copy link

joelberkeley commented Mar 31, 2024

I don't know bazel internals so I can't speak for the implementation, but as for the API, the cc_library docs say

For cc_library rules, headers in hdrs comprise the public interface of the library and can be directly included both from the files in hdrs and srcs of the library itself as well as from files in hdrs and srcs of cc_* rules that list the library in their deps. Headers in srcs must only be directly included from the files in hdrs and srcs of the library itself. When deciding whether to put a header into hdrs or srcs, you should ask whether you want consumers of this library to be able to directly include it. This is roughly the same decision as between public and private visibility in programming languages.

Headers in hdrs could be collated, using the same directory structure as they appear in the source, to a dedicated directory, perhaps include. I notice there's no hdrs argument in the cc_static_library in the tests, so I guess it's not a parameter to the rule. I guess one could re-export those hdrs in rules the cc_static_library has in its deps, though that would assume one wants it to all be part of the public API. Future work could make that configurable (so only some parts are re-exported).

Before I get too into this, I really don't want to get in the way of useful functionality. Perhaps header collation could be added in a future PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Someone outside the Bazel team could own this P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Rules-CPP Issues for C++ rules type: feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.