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

incompatible_disable_legacy_cc_provider: Deprecate old C++ Starlark provider API #7036

Open
oquenchil opened this Issue Jan 4, 2019 · 30 comments

Comments

@oquenchil
Copy link
Contributor

oquenchil commented Jan 4, 2019

Flag: --incompatible_disable_legacy_cc_provider
Available since: 0.22 (January 2019 release)
Will be flipped in: 0.24 (February 2019 release)
Tracking issue: #4570

This is a tracking issue for the removal of the legacy C++ Starlark provider.

We are deprecating this old API in favor of the new C++ Starlark API that will be fully supported in the future and will provide more functionality than this deprecated API.

Migration instructions

Replacements:

Old API New API
dep.cc.transitive_headers dep[CcInfo].compilation_context.headers
dep.cc.defines dep[CcInfo].compilation_context.defines.to_list()
dep.cc.system_include_directories dep[CcInfo].compilation_context.system_includes.to_list()
dep.cc.include_directories dep[CcInfo].compilation_context.includes.to_list()
dep.cc.quote_include_directories dep[CcInfo].compilation_context.quote_includes.to_list()
dep.cc.link_flags dep[CcInfo].linking_context.user_link_flags
dep.cc.libs get_libs_for_static_executable(dep)
dep.cc.compile_flags get_compile_flags(dep)

For the methods get_libs_for_static_executable and get_compile_flags, see as an example: https://gist.github.com/oquenchil/7e2c2bd761aa1341b458cc25608da50c

@oquenchil oquenchil self-assigned this Jan 4, 2019

@hlopko hlopko changed the title Deprecate old C++ Starlark provider API incompatible_disable_legacy_cc_provider: Deprecate old C++ Starlark provider API Jan 4, 2019

@hlopko

This comment has been minimized.

Copy link
Contributor

hlopko commented Jan 4, 2019

CC @lberki

bazel-io pushed a commit that referenced this issue Jan 4, 2019

C++:Add a flag to disable the legacy C++ provider.
Working towards #7036
RELNOTES: None.
PiperOrigin-RevId: 227838975
@jayconrod

This comment has been minimized.

Copy link
Collaborator

jayconrod commented Jan 8, 2019

Flipping this flag breaks rules_go. We need to be able to compile against and link C/C++/ObjC code. We need most of the information in this provider, such as defines, include_directories, transitive_headers, link_flags...

Is there a replacement? What's the migration plan?

@hlopko

This comment has been minimized.

Copy link
Contributor

hlopko commented Jan 8, 2019

Yup we'know go needs replacement. Pedro will write solid migration docs once he's back from vacation. If I had to steal comment from his unsubmitted cl, the rough plan is:

dep.cc.transitive_headers -> dep[CcInfo].compilation_context.headers
dep.cc.defines -> dep[CcInfo].compilation_context.defines.to_list()
dep.cc.system_include_directories -> dep[CcInfo].compilation_context.system_includes.to_list()
dep.cc.include_directories -> dep[CcInfo].compilation_context.includes.to_list()
dep.cc.quote_include_directories -> dep[CcInfo].compilation_context.quote_includes.to_list()
dep.cc.link_flags = dep[CcInfo].linking_context.user_link_flags
dep.cc.libs = get_libs_for_static_executable(dep)
dep.cc.compile_flags = get_compile_flags(dep)

get_libs_for_static_executable and get_compile_flags will be provided such they return what legacy provider returned. We expect at least users be surprised by what was returned and prefer to use custom logic for get_libs_for_static_executable and use C++ toolchain api for get_compile_flags.

@jayconrod

This comment has been minimized.

Copy link
Collaborator

jayconrod commented Jan 8, 2019

Cool, thanks for sharing those instructions. It doesn't look like CcInfo or ProtoInfo are documented yet. Are those new in Bazel 0.22.0?

Basically, I'm wondering when I should do the migration in rules_go, and how big the support window of Bazel versions can be. Since Starlark resolves symbols eagerly, and CcInfo and ProtoInfo need to be used directly (not through reflection), I think releases before migration will have a maximum Bazel version of 0.23.0, and releases after migration will have a minimum version of Bazel 0.22.0. Does that sound right?

@hlopko

This comment has been minimized.

Copy link
Contributor

hlopko commented Jan 8, 2019

(Where do you see ProtoInfo?)

Pedro will know if CcInfo was in 0.21 or sooner (ping @oquenchil).

What you can do in these cases is to use versions.bzl in the repository rule to pick the right version of the analysis time bzl file depending on the bazel version. We do that in rules_rust (this is the PR, but look for the state at head, I generated many bugs there).

Does that solve your issue?

@jayconrod

This comment has been minimized.

Copy link
Collaborator

jayconrod commented Jan 8, 2019

I found ProtoInfo mentioned in #6901. Seems like both providers are deprecated in 0.22.0.

About using versions.bzl and the repository rule, that would definitely work, thanks for pointing it out in rules_rust. It's unclear to me if the additional compatibility is worth the maintenance cost though. Will have to think about it.

bazel-io pushed a commit that referenced this issue Jan 9, 2019

C++: Shim to replace old C++ API provider
Working towards #7036

RELNOTES:none
PiperOrigin-RevId: 228522635
@dslomov

This comment has been minimized.

Copy link
Contributor

dslomov commented Jan 11, 2019

Please update this bug with information about what is breaking and (at least roughly) what is the migration path

@oquenchil

This comment has been minimized.

Copy link
Contributor Author

oquenchil commented Jan 14, 2019

I added migration instructions .

@keith

This comment has been minimized.

Copy link
Contributor

keith commented Jan 14, 2019

@oquenchil I think there's a depo for the new version of system_include_directories and include_directories. It looks like they shoudl be system_includes and includes on CompilationContext respectively

@keith

This comment has been minimized.

Copy link
Contributor

keith commented Jan 14, 2019

It also looks like this legacy shim file isn't in the 0.22 RC https://github.com/bazelbuild/bazel/tree/release-0.22.0/tools/cpp is this something that's going to be released with this or not? Otherwise we can't migrate until 0.23

@jmillikin

This comment has been minimized.

Copy link
Contributor

jmillikin commented Jan 15, 2019

The docstring for linking_context in https://docs.bazel.build/versions/master/skylark/lib/CcInfo.html has a typo. It is CcLinkingContext, and should be LinkingContext to match https://docs.bazel.build/versions/master/skylark/lib/LinkingContext.html.

Alternatively, prefix the context types with Cc so the user doesn't have to what what kind of compilation it's contexting.

@oquenchil

This comment has been minimized.

Copy link
Contributor Author

oquenchil commented Jan 15, 2019

Thank you very much @keith, I fixed the table. Regarding the shim, it's not really necessary, it's just there for your inspiration. The old API isn't completely correct.

The first method get_compile_flags doesn't return the compilation command line that the C++ toolchain API would (for example, on Windows using MSVC it will still use GCC style flags). Therefore, it's better to use the C++ toolchain API.

For the second method get_libs_for_static_executable, the artifacts are correct for a static executable but if you use the new API directly you can customize for your own rule depending on what type of libraries you support. For example get_libs_for_static_executable may return dynamic libraries when you expect only static libraries.

If you are sure that the logic that you want is what is in the shim, then you can just copy over what is here to your own rules: https://github.com/bazelbuild/bazel/blob/master/tools/cpp/legacy_cc_starlark_api_shim.bzl

@jmillikin thank you for pointing that out. We are aware of it, this is due to a deprecated class that is still lying around. The documentation is auto-generated, that class will go away with the next release so the docs should be fixed.

@aehlig

This comment has been minimized.

Copy link
Member

aehlig commented Jan 15, 2019

It also looks like this legacy shim file isn't in the 0.22 RC https://github.com/bazelbuild/bazel/tree/release-0.22.0/tools/cpp is this something that's going to be released with this or not?

Our policy is to not cherry-pick for features. So you will have to delay the migration.

@dslomov

This comment has been minimized.

Copy link
Contributor

dslomov commented Jan 15, 2019

It also looks like this legacy shim file isn't in the 0.22 RC https://github.com/bazelbuild/bazel/tree/release-0.22.0/tools/cpp is this something that's going to be released with this or not?

Our policy is to not cherry-pick for features. So you will have to delay the migration.

Does this mean we need to delay the breaking change beyond 0.23 @oquenchil @hlopko ?

@oquenchil

This comment has been minimized.

Copy link
Contributor Author

oquenchil commented Jan 15, 2019

No need to postpone. As I commented in #7036 (comment), people don't need to rely on the bzl file.

@oquenchil

This comment has been minimized.

Copy link
Contributor Author

oquenchil commented Jan 15, 2019

I deleted the file from Bazel. I created a gist instead that people can copy.

bazel-io pushed a commit that referenced this issue Jan 15, 2019

Automated rollback of commit 3976d58.
*** Reason for rollback ***

No longer needed. We won't maintain this:
https://gist.github.com/oquenchil/7e2c2bd761aa1341b458cc25608da50c

*** Original change description ***

C++: Shim to replace old C++ API provider

Working towards #7036

RELNOTES:none
PiperOrigin-RevId: 229358490
@jmillikin

This comment has been minimized.

Copy link
Contributor

jmillikin commented Jan 16, 2019

I'm trying out the CcInfo provider and got a Java stack trace from what seemed like a minimal "compile C source to object code" rule. Filed as #7136

@oquenchil

This comment has been minimized.

Copy link
Contributor Author

oquenchil commented Jan 16, 2019

Hey John, did that code use to work with the old API? It seems that the extension of the static library that you are passing isn't one of .pic.a, .a or .lib. It should definitely not crash but I'd need to know if this code used to work with the old API. If it doesn't work with the old API, the incompatible change can stay for this release and the crash would be fixed the release after.

@jmillikin

This comment has been minimized.

Copy link
Contributor

jmillikin commented Jan 16, 2019

If it doesn't work with the old API, the incompatible change can stay for this release and the crash would be fixed the release after.

I haven't tried it with the old API. This particular crash (incorrect and possibly glob()-supplied param crashing the entire Bazel process) seems like it's worth cherrypicking a fix for.

@hlopko

This comment has been minimized.

Copy link
Contributor

hlopko commented Jan 16, 2019

To be honest I'm not clear on this corner of the cherry-pick policy. @aehlig, do we allow cherrypicking fixes for the new functionality introduced in the candidate?

@aehlig

This comment has been minimized.

Copy link
Member

aehlig commented Jan 16, 2019

@aehlig, do we allow cherrypicking fixes for the new functionality introduced in the candidate?

Our policy is clear here: we only cherry-pick fixes for regressions with respect to previous releases. The new functionality wasn't working (because it wasn't present) in 0.21, so it being broken in 0.22 is not a reason for a cherry pick.

@keith

This comment has been minimized.

Copy link
Contributor

keith commented Jan 18, 2019

I think there's another issue here related to the migration window. It looks like quote_includes doesn't exist in 0.21, so for some projects we won't be able to migrate until after the release

@hlopko

This comment has been minimized.

Copy link
Contributor

hlopko commented Jan 21, 2019

That's expected, the migration window for this flag is 0.22 (released hopefully this week) and it will be flipped in 0.23 (released late February) = 1 release. If you want, you can make your change forward compatible by using Skylib's versions.bzl (see #7036 (comment)).

@jayconrod

This comment has been minimized.

Copy link
Collaborator

jayconrod commented Jan 22, 2019

@hlopko FYI, I don't think the versions.bzl trick will work for all projects. In particular, I don't think it will work for rules_go without a breaking change in the WORKSPACE boilerplate.

The Starlark interpreter resolves symbols eagerly, so there can't be references to CcInfo in .bzl files that may be loaded by older versions of Bazel, even if those references are never evaluated. So as you suggested above, we can load compatibility functions from a .bzl file in a repository rule which uses versions.bzl to select an implementation.

The catch, for rules_go at least, is that we load all our public symbols from one file, //go:def.bzl, including our repository rules. That means .bzl files reachable from //go:def.bzl can't reference anything in external repositories, since nothing has been declared at the time those files are loaded.

If I were starting over today, I'd put all the repository rules in a separate file, //go:deps.bzl, so that everything reachable from //go:def.bzl could assume the necessary external repos have already been declared. This is what Gazelle does in order to use Skylib. Making this change now would require users to update their WORKSPACE to continue using old versions of Bazel.

It's not clear to me if that migration is worth making. The rules_go API isn't changing much anymore, so I'm hoping it will be easy enough for devs to update to newer rules_go major versions in order to get support for newer Bazel versions.

@hlopko

This comment has been minimized.

Copy link
Contributor

hlopko commented Jan 22, 2019

Then there is an option of checking in https://github.com/bazelbuild/bazel-skylib/blob/master/lib/versions.bzl in rules_go (or the part that is used). Or I missed your point?

@jayconrod

This comment has been minimized.

Copy link
Collaborator

jayconrod commented Jan 22, 2019

@hlopko We actually have versions.bzl checked in already, that's not the issue.

I started prototyping this a couple days ago. I wanted to have a compat.bzl file which would provide several constants and wrapper functions for different versions of Bazel. This would be loaded as @io_bazel_rules_go_compat//:compat.bzl. The io_bazel_rules_go_compat workspace would be declared in go_rules_dependencies (where we declare other dependencies) using a repository rule. The repository rule would use versions.bzl to select one of several .bzl files to be copied to compat.bzl.

The problem was that in WORKSPACE, we need to load @io_bazel_rules_go//go:def.bzl into order to get go_rules_dependencies, and @io_bazel_rules_go_compat//:compat.bzl is loaded transitively from @io_bazel_rules_go//go:def.bzl, before the io_bazel_rules_compat repo can be declared. So we get a cyclic load error.

@hlopko

This comment has been minimized.

Copy link
Contributor

hlopko commented Jan 24, 2019

Oh, is it because you load @rules_go//go:def.bzl from both WORKSPACE and BUILD files? Yeah I didn't think about that. I'm curious (for rules_cc), was this a conscious decision? What were the reasons to not use 2 separate bzl files?

@jayconrod

This comment has been minimized.

Copy link
Collaborator

jayconrod commented Jan 24, 2019

@hlopko "It was like that when I got here." :)

Seriously though, rules_go used to be that all definitions could be in //go:def.bzl, and it was only ~300 lines of code. Eventually, we moved stuff into private directories, but //go:def.bzl was always the single thing that people could load to get public definitions.

When I pulled Gazelle out into its own repository, I added separate //:deps.bzl (for WORKSPACE definitions) and //:def.bzl (for other definitions). That was necessary to add a Skylib dependency. Maybe rules_go should move to that model for newer releases. It depends on how many breaking changes are expected in the future and how long the support window needs to be.

@oquenchil

This comment has been minimized.

Copy link
Contributor Author

oquenchil commented Jan 25, 2019

Delaying one more release because of bug in Windows. Fixed in cae1e816e5e1142fbd4aefdd29bffb2cbad71fa8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment