rules_cc icon indicating copy to clipboard operation
rules_cc copied to clipboard

Added cc_library_func(), a function for cc_library()-like semantics.

Open haberman opened this issue 5 years ago • 7 comments

This is a high-level API that sits on top of the sandwich to provide cc_library()-like semantics.

The goal is for this API to stay stable even as the underlying Sandwich APIs evolve into their final form. We can add version detection as necessary if/when the old APIs are deprecated.

Ideally this would be whitelisted to use the sandwich APIs in the next release of Bazel.

haberman avatar Apr 14 '19 01:04 haberman

The cc_library_func() function in this PR seems to work, but I did notice once difference from a real cc_library() (here libbar.a is from my custom rule):

$ cat bazel-out/k8-opt/bin/examples/my_c_rule/main-2.params
bazel-out/k8-opt/bin/examples/my_c_rule/_objs/main/main.o
--start-lib
bazel-out/k8-opt/bin/examples/my_c_rule/_objs/baz/baz.o
--end-lib
bazel-out/k8-opt/bin/examples/my_c_rule/libbar.a
--start-lib
bazel-out/k8-opt/bin/examples/my_c_rule/_objs/foo/foo.o
--end-lib
-lstdc++
-lm

The real cc_library() targets are linked by their .o files (surrounded in --start-lib/--end-lib) but my custom my_cc_library() is linked by a .a file. Did I do something wrong in my cc_library_func() function? Or is this difference coming from the underlying API?

haberman avatar Apr 14 '19 01:04 haberman

Why do you create your own static library action? Sandwich api will create one if needed.

Also, I could be convinced to have that function here in this repo, but with these prerequisites:

  1. We know it's good enough for cc_library. We don't want to maintain a separate function if we cannot use it or which we don't understand how our users are using.
  2. It's using the stable APIs. We don't have a backward compatibility story set for rules_cc yet, which some can interpret the wrong way which will lead to surprises when we break people. I.e. we don't want to provide a incremental migration story for this for 0.24.0 and 0.25, only for 0.26 and forward when the API will be stable.
  3. it's thoroughly tested.

And even after all these are fulfilled I'm thinking if adding a third layer to support is something we want to do and we understand implications of (we have rules as the first layer, then the Sandwich API as the second). There will eventually be a backwards compatibility policy (before we tell users to use this repo), which will protect users of the API from willy-nilly breaking changes and will offer migration instructions and a way to migrate incrementally.

Wdyt?

hlopko avatar Apr 15 '19 09:04 hlopko

Why do you create your own static library action? Sandwich api will create one if needed.

I didn't know that. I don't understand the sandwich API very well, because I couldn't find any docs that explain things like this. I just copy+pasted snippets from other projects and iterated through trial and error util I got it working.

How do I fix this? I think I still need a linking context somehow? As a guess I tried using cc_common.create_linking_context_from_compilation_outputs instead of doing all the archiver stuff, but that function doesn't appear to exist in 0.24.1, only in master.

It's using the stable APIs. [...] we don't want to provide a incremental migration story for this for 0.24.0 and 0.25, only for 0.26 and forward when the API will be stable.

My goal with this PR is to provide a stable API now, even though it is built on top of unstable APIs. I want the cc_library_func() function to do version detection like this, if/when it is necessary for new Bazel versions:

if not hasattr(cc_common, create_linking_context_from_compilation_outputs):
  # Bazel 0.24.1 code
elif <some predicate that detects Bazel 0.25>:
  # Bazel 0.25 code.
# etc.

This would let users like me start using it now, without requiring any user migration for future Bazel versions (only this function would need to change).

Wdyt?

I need this API now. The main question is whether the function goes in my repo (https://github.com/protocolbuffers/upb) or yours.

If it goes in your repo, users besides me can take advantage of it. I understand that you might want to avoid offering another public API (which is your decision to make). If your other users are like me, they will prefer cc_library_func() to cc_common.*. It would also let users start using it now, even though the full sandwich API won't be available for several more months.

I unfortunately don't have much more time to spend on this right now. I'm happy to add some more tests or fix bugs, but I don't have the time to do a full-scale cc_library() on top of this that supports all attributes and tests all the cases. I also can't wait for the full sandwich API to be available. If you're not able to accept this now, I understand, I'll just add it to my own repo instead.

haberman avatar Apr 16 '19 06:04 haberman

I'm sorry you had to struggle through this without proper docs. Docs will be provided for the stable API, but because the experimental API was changing a lot it didn't make sense to write docs yet.

You can always create a linking context by hand, the constructor function is stable - https://docs.bazel.build/versions/master/skylark/lib/cc_common.html#create_linking_context. You can also ignore the Sandwich API completely and create your own compilation and linking actions. C++ toolchain API and C++ providers API are all you need and those are both stable. Some usage is linked from https://docs.bazel.build/versions/master/integrating-with-rules-cc.html.

I think the most responsible way forward is to keep cc_library_func-like function in your repo in the meantime, and only move it to rules_cc when the code and test coverage are robust enough for users to use and for us to claim stability. I must say the idea of this helper function is growing on me, but only based on the stable APIs and coverage.

Thanks Josh for understanding!

hlopko avatar Apr 16 '19 07:04 hlopko

You can always create a linking context by hand, the constructor function is stable

But how do I create the LibraryToLink that I need for create_linking_context? The create_library_to_link function wants a single File for static_library and pic_static_library, which seems to imply that I have to compile all the .o files into a single .a file. How can I give it the list of .o files instead?

You can also ignore the Sandwich API completely and create your own compilation and linking actions.

Earlier you mentioned that I would be missing a lot of functionality if I did this, so I'd prefer not to go this route.

I think the most responsible way forward is to keep cc_library_func-like function in your repo in the meantime

Ok, sounds good.

I must say the idea of this helper function is growing on me

I'm glad. :)

haberman avatar Apr 16 '19 15:04 haberman

I'm not sure we need this, the API should stay fairly stable and I can imagine this function would just start adding more and more parameters until it matches the actual API. For example, it might be fairly common to need to pass a custom feature_configuration.

oquenchil avatar Jun 06 '19 15:06 oquenchil

Adding my two cents, if you have an aspect that wants to make a cc_library type thing next to each target it runs on (like generating code for each proto_library), you can't use native.cc_library in an aspect

kwasimensah avatar Oct 07 '19 17:10 kwasimensah

Closing stale PR. Starlark API is stable now.

oquenchil avatar Mar 13 '23 11:03 oquenchil