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.

Sign up for GitHub

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

Jump to bottom

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

Closed
oquenchil opened this issue Jan 4, 2019 · 33 comments
Closed

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

oquenchil opened this issue Jan 4, 2019 · 33 comments
Assignees
@oquenchil
Labels
incompatible-change Incompatible/breaking change P1 I'll work on this now. (Assignee required) team-Rules-CPP Issues for C++ rules

Comments

@oquenchil
Copy link
Contributor

oquenchil commented Jan 4, 2019 •
edited
Loading

Flag: --incompatible_disable_legacy_cc_provider
Available since: 0.22 (January 2019 release)
Will be flipped in: 0.25 (March 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
@oquenchil oquenchil added P1 I'll work on this now. (Assignee required) team-Rules-CPP Issues for C++ rules Bazel 1.0 labels 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 hlopko added incompatible-change Incompatible/breaking change migration-ready labels Jan 4, 2019
@hlopko
Copy link
Member

hlopko commented Jan 4, 2019

CC @lberki

bazel-io pushed a commit that referenced this issue Jan 4, 2019
@oquenchil
C++:Add a flag to disable the legacy C++ provider.
a60b6ce 
Working towards #7036
RELNOTES: None.
PiperOrigin-RevId: 227838975
@jayconrod
Copy link
Contributor

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?

@jayconrod jayconrod mentioned this issue Jan 8, 2019
Closed
@hlopko
Copy link
Member

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 jayconrod mentioned this issue Jan 8, 2019
Merged
@jayconrod
Copy link
Contributor

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
Copy link
Member

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?

@keith keith mentioned this issue Jan 8, 2019
Closed
@jayconrod
Copy link
Contributor

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.

@hlopko hlopko mentioned this issue Jan 9, 2019
Closed
bazel-io pushed a commit that referenced this issue Jan 9, 2019
@oquenchil
C++: Shim to replace old C++ API provider
3976d58 
Working towards #7036

RELNOTES:none
PiperOrigin-RevId: 228522635
@hlopko
Copy link
Member

hlopko commented Jan 11, 2019 •
edited
Loading

We have some issues with the doc generation, in the meantime, here are docs:

https://docs.bazel.build/versions/master/skylark/lib/CcInfo.html
https://docs.bazel.build/versions/master/skylark/lib/CompilationContext.html
https://docs.bazel.build/versions/master/skylark/lib/LinkingContext.html
https://docs.bazel.build/versions/master/skylark/lib/LibraryToLink.html
https://docs.bazel.build/versions/master/skylark/lib/cc_common.html#create_compilation_context
https://docs.bazel.build/versions/master/skylark/lib/cc_common.html#create_linking_context
https://docs.bazel.build/versions/master/skylark/lib/cc_common.html#create_library_to_link

@dslomov
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
Copy link
Contributor Author

I added migration instructions .

@Profpatsch Profpatsch mentioned this issue Jan 14, 2019
Closed
@keith
Copy link
Member

keith commented Jan 14, 2019 •
edited
Loading

@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
Copy link
Member

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

This was referenced Jan 14, 2019
Closed
Closed
@jmillikin
Copy link
Contributor

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
Copy link
Contributor Author

oquenchil commented Jan 25, 2019 •
edited
Loading

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

@mfarrugi mfarrugi mentioned this issue Feb 8, 2019
Closed
@hlopko hlopko mentioned this issue Feb 19, 2019
Closed
bazel-io pushed a commit that referenced this issue Mar 1, 2019
@oquenchil @copybara-github
C++: Flips default to True for --incompatible_disable_legacy_cc_provider
972e73e 
Tracking issue: #7036

RELNOTES:none
PiperOrigin-RevId: 236281588
@katre
Copy link
Member

katre commented Mar 1, 2019

This issue was tagged as "breaking-change-0.24" but does not appear ready to be flipped in the 0.24.0 release. If this is incorrect please comment on that issue and discuss with me.

bazel-io pushed a commit that referenced this issue Mar 1, 2019
@oquenchil @copybara-github
Automated rollback of commit 972e73e.
c19a161 
*** Reason for rollback ***

Breaking project downstream.

*** Original change description ***

C++: Flips default to True for --incompatible_disable_legacy_cc_provider

Tracking issue: #7036

RELNOTES:none
PiperOrigin-RevId: 236320142
This was referenced Mar 5, 2019
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@meteorcloudy
Copy link
Member

@oquenchil Looks like the only failing projects are rules_rust and TensorFlow. TensorFlow is already disabled in downstream, if you want to flip the flag for catching the 0.25.0 cut, you can send a PR to disable rules_rust as well.
For example bazelbuild/continuous-integration@69cf7ec

@hlopko
Copy link
Member

hlopko commented Mar 27, 2019

I'll flip it later this week if nothing new shows up.

bazel-io pushed a commit that referenced this issue May 1, 2019
@dkelmer
Release 0.25.0 (2019-05-01)
2ab3866 
Baseline: 0366246

Cherry picks:

   + 3f7f255:
     Windows: fix native test wrapper's arg. escaping
   + afeb8d0:
     Flip --incompatible_windows_escape_jvm_flags
   + 4299b65:
     Sort DirectoryNode children to ensure validity.
   + 231270c:
     Conditionally use deprecated signature for initWithContentsOfURL
   + 75a3a53:
     Add http_archive entries for testing with various JDK versions.
   + 4a6354a:
     Now that ubuntu1804 uses JDK 11, remove explicit
     ubuntu1804_java11 tests.
   + ae102fb:
     Fix wrong name of ubuntu1804_javabase9 task.
   + 0020a97:
     Remove @executable_path/Frameworks from rpaths
   + 130f86d:
     Download stderr/stdout to a temporary FileOutErr

Incompatible changes:

  - (Starlark rules) The legacy "py" provider can no longer be passed
    to or produced by native Python rules; use
    [PyInfo](https://docs.bazel.build/versions/master/skylark/lib/PyIn
    fo.html) instead. See
    [#7298](#7298) for more
    information.
  - (Python rules) The `default_python_version` attribute of the
    `py_binary` and `py_test` rules has been renamed to
    `python_version`. Also, the `--force_python` flag has been
    renamed to `--python_version`. See
    [#7308](#7308) for more
    information.
  - (Python rules) The python version now changes to whatever version
    is specified in a `py_binary` or `py_test`'s `python_version`
    attribute, instead of being forced to the value set by a command
    line flag. You can temporarily revert this change with
    `--incompatible_allow_python_version_transitions=false`. See
    [#7307](#7307) for more
    information.
  - --incompatible_disable_third_party_license_checking` is enabled
    by default
  - Introduced --incompatible_use_python_toolchains, which supersedes
    --python_top/--python_path. See #7899 and #7375 for more
    information.
  - Python 3 is now the default Python version (for `py_binary` and
    `py_test` targets that don't specify the `python_version`
    attribute). Targets that are built for Python 3 will no longer
    have their output put in a separate `-py3` directory; instead
    there is now a separate `-py2` directory for Python 2 targets.
    See #7359 and #7593 for more information.
  - objc_library resource attributes are now disabled by default.
    Please migrate them to data instead. See
    #7594 for more info.
  - Flip --incompatible_windows_escape_jvm_flags to true. See
    #7486

New features:

  - genrules now support a $(RULEDIR) variable that resolves to the
    directory where the outputs of the rule are put.
  - Added --incompatible_windows_native_test_wrapper flag: enables
    using the Bash-less test wrapper on Windows. (No-op on other
    platforms.)

Important changes:

  - incompatible_use_jdk11_as_host_javabase: makes JDK 11 the default
    --host_javabase for remote jdk
    (#7219)
  - Makes genquery somepath output deterministic.
  - Tristate attributes of native rules now reject True/False (use
    1/0)
  - Rollback of "Tristate attributes of native rules now reject
    True/False (use 1/0)"
  - Tristate attributes of native rules now reject True/False (use
    1/0)
  - Added -incompatible_do_not_split_linking_cmdline flag. See #7670
  - Tristate attributes of native rules now temporarily accept
    True/False again
  - `--incompatible_disable_legacy_crosstool_fields` has been flipped
    (#6861)
    `--incompatible_disable_expand_if_all_available_in_flag_set` has
    been flipped (#7008)
  - `--incompatible_disable_legacy_crosstool_fields` has been flipped
    (#6861)
    `--incompatible_disable_expand_if_all_available_in_flag_set...
    RELNOTES: None.
  - --incompatible_no_transitive_loads is enabled by default.
  - Makes TreeArtifact deterministic.
  - --incompatible_no_transitive_loads is enabled by default.
  - Android NDK C++ toolchain is now configured in Starlark. This
    should be a backwards compatible change, but in case of bugs
    blame unknown commit.
  - `--incompatible_disable_legacy_crosstool_fields` has been flipped
    (#6861)
    `--incompatible_disable_expand_if_all_available_in_flag_set` has
    been flipped (#7008)
  - --incompatible_no_transitive_loads is enabled by default.
  - --incompatible_bzl_disallow_load_after_statement is enabled
  - Added `--incompatible_require_ctx_in_configure_features`, see
    #7793 for details.
  - Flag --incompatible_merge_genfiles_directory is flipped. This
    removes the directory `bazel-genfiles` in favor of `bazel-bin`.
  - previously deprecated flag --experimental_remote_spawn_cache was
    removed
  - `--incompatible_disallow_load_labels_to_cross_package_boundaries`
    is enabled by default
  - Fix an issue where the Android resource processor did not surface
    errors from aapt2 compile and link actions.
  - --incompatible_no_attr_license is enabled by default
  - `--incompatible_disable_crosstool_file` has been flipped
    (#7320)
  - A new flag `--incompatible_string_join_requires_strings` is
    introduced. The sequence argument of `string.join` must contain
    only string elements.
  - --incompatible_symlinked_sandbox_expands_tree_artifacts_in_runfile
    s_tree has been flipped
  - Incompatible flag `--incompatible_disable_legacy_cc_provider` has
    been flipped (see #7036
    for details).
  - Don't drop the analysis cache when the same --define flag is set
    multiple times and the last value is the same (e.g. if the
    current invocation was run with "--define foo=bar" and the
    previous one was run with "--define foo=baz --define foo=bar").
  - The --incompatible_disable_genrule_cc_toolchain_dependency flag
    has been flipped (see
    #6867 for details).
  - Incompatible change
    `--incompatible_remove_cpu_and_compiler_attributes_from_cc_toolcha
    in` has been flipped (see
    #7075 for details).
  - --noexperimental_java_coverage is a no-op flag.
  - --experimental_java_coverage/--incompatible_java_coverage flag was
    removed. See #7425.
  - incompatible_use_toolchain_providers_in_java_common: pass
    JavaToolchainInfo and JavaRuntimeInfo providers to java_common
    APIs instead of configured targets
    (#7186.)
  - --incompatible_remote_symlinks has been flipped. The remote
    caching and execution protocol will now represent symlinks in
    outputs as such. See
    #7917 for more details.
  - Bazel is now ~20MiB smaller, from unbundling the Android rules'
    runtime dependencies.

This release contains contributions from many people at Google, as well as Andreas Herrmann, Andrew Suffield, Andy Scott, Benjamin Peterson, Ed Baunton, George Gensure, Ian McGinnis, Ity Kaul, Jingwen Chen, John Millikin, Keith Smiley, Marwan Tammam, Mike Fourie, Oscar Bonilla, perwestling, petros, Robert Sayre, Ryan Beasley, silvergasp, Stanimir Mladenov, Travis Cline, Vladimir Chebotarev, ??.
@jamiesnape jamiesnape mentioned this issue May 3, 2019
Merged
dkelmer pushed a commit that referenced this issue May 6, 2019
@dkelmer
Release 0.25.0 (2019-05-01)
f44bc47 
Baseline: 0366246

Cherry picks:

   + 3f7f255:
     Windows: fix native test wrapper's arg. escaping
   + afeb8d0:
     Flip --incompatible_windows_escape_jvm_flags
   + 4299b65:
     Sort DirectoryNode children to ensure validity.
   + 231270c:
     Conditionally use deprecated signature for initWithContentsOfURL
   + 75a3a53:
     Add http_archive entries for testing with various JDK versions.
   + 4a6354a:
     Now that ubuntu1804 uses JDK 11, remove explicit
     ubuntu1804_java11 tests.
   + ae102fb:
     Fix wrong name of ubuntu1804_javabase9 task.
   + 0020a97:
     Remove @executable_path/Frameworks from rpaths
   + 130f86d:
     Download stderr/stdout to a temporary FileOutErr

Incompatible changes:

  - (Starlark rules) The legacy "py" provider can no longer be passed
    to or produced by native Python rules; use
    [PyInfo](https://docs.bazel.build/versions/master/skylark/lib/PyIn
    fo.html) instead. See
    [#7298](#7298) for more
    information.
  - (Python rules) The `default_python_version` attribute of the
    `py_binary` and `py_test` rules has been renamed to
    `python_version`. Also, the `--force_python` flag has been
    renamed to `--python_version`. See
    [#7308](#7308) for more
    information.
  - (Python rules) The python version now changes to whatever version
    is specified in a `py_binary` or `py_test`'s `python_version`
    attribute, instead of being forced to the value set by a command
    line flag. You can temporarily revert this change with
    `--incompatible_allow_python_version_transitions=false`. See
    [#7307](#7307) for more
    information.
  - --incompatible_disable_third_party_license_checking` is enabled
    by default
  - Introduced --incompatible_use_python_toolchains, which supersedes
    --python_top/--python_path. See #7899 and #7375 for more
    information.
  - Python 3 is now the default Python version (for `py_binary` and
    `py_test` targets that don't specify the `python_version`
    attribute). Targets that are built for Python 3 will no longer
    have their output put in a separate `-py3` directory; instead
    there is now a separate `-py2` directory for Python 2 targets.
    See #7359 and #7593 for more information.
  - objc_library resource attributes are now disabled by default.
    Please migrate them to data instead. See
    #7594 for more info.
  - Flip --incompatible_windows_escape_jvm_flags to true. See
    #7486

New features:

  - genrules now support a $(RULEDIR) variable that resolves to the
    directory where the outputs of the rule are put.
  - Added --incompatible_windows_native_test_wrapper flag: enables
    using the Bash-less test wrapper on Windows. (No-op on other
    platforms.)

Important changes:

  - incompatible_use_jdk11_as_host_javabase: makes JDK 11 the default
    --host_javabase for remote jdk
    (#7219)
  - Makes genquery somepath output deterministic.
  - Tristate attributes of native rules now reject True/False (use
    1/0)
  - Rollback of "Tristate attributes of native rules now reject
    True/False (use 1/0)"
  - Tristate attributes of native rules now reject True/False (use
    1/0)
  - Added -incompatible_do_not_split_linking_cmdline flag. See #7670
  - Tristate attributes of native rules now temporarily accept
    True/False again
  - `--incompatible_disable_legacy_crosstool_fields` has been flipped
    (#6861)
    `--incompatible_disable_expand_if_all_available_in_flag_set` has
    been flipped (#7008)
  - `--incompatible_disable_legacy_crosstool_fields` has been flipped
    (#6861)
    `--incompatible_disable_expand_if_all_available_in_flag_set...
    RELNOTES: None.
  - --incompatible_no_transitive_loads is enabled by default.
  - Makes TreeArtifact deterministic.
  - --incompatible_no_transitive_loads is enabled by default.
  - Android NDK C++ toolchain is now configured in Starlark. This
    should be a backwards compatible change, but in case of bugs
    blame unknown commit.
  - `--incompatible_disable_legacy_crosstool_fields` has been flipped
    (#6861)
    `--incompatible_disable_expand_if_all_available_in_flag_set` has
    been flipped (#7008)
  - --incompatible_no_transitive_loads is enabled by default.
  - --incompatible_bzl_disallow_load_after_statement is enabled
  - Added `--incompatible_require_ctx_in_configure_features`, see
    #7793 for details.
  - Flag --incompatible_merge_genfiles_directory is flipped. This
    removes the directory `bazel-genfiles` in favor of `bazel-bin`.
  - previously deprecated flag --experimental_remote_spawn_cache was
    removed
  - `--incompatible_disallow_load_labels_to_cross_package_boundaries`
    is enabled by default
  - Fix an issue where the Android resource processor did not surface
    errors from aapt2 compile and link actions.
  - --incompatible_no_attr_license is enabled by default
  - `--incompatible_disable_crosstool_file` has been flipped
    (#7320)
  - A new flag `--incompatible_string_join_requires_strings` is
    introduced. The sequence argument of `string.join` must contain
    only string elements.
  - --incompatible_symlinked_sandbox_expands_tree_artifacts_in_runfile
    s_tree has been flipped
  - Incompatible flag `--incompatible_disable_legacy_cc_provider` has
    been flipped (see #7036
    for details).
  - Don't drop the analysis cache when the same --define flag is set
    multiple times and the last value is the same (e.g. if the
    current invocation was run with "--define foo=bar" and the
    previous one was run with "--define foo=baz --define foo=bar").
  - The --incompatible_disable_genrule_cc_toolchain_dependency flag
    has been flipped (see
    #6867 for details).
  - Incompatible change
    `--incompatible_remove_cpu_and_compiler_attributes_from_cc_toolcha
    in` has been flipped (see
    #7075 for details).
  - --noexperimental_java_coverage is a no-op flag.
  - --experimental_java_coverage/--incompatible_java_coverage flag was
    removed. See #7425.
  - incompatible_use_toolchain_providers_in_java_common: pass
    JavaToolchainInfo and JavaRuntimeInfo providers to java_common
    APIs instead of configured targets
    (#7186.)
  - --incompatible_remote_symlinks has been flipped. The remote
    caching and execution protocol will now represent symlinks in
    outputs as such. See
    #7917 for more details.
  - Bazel is now ~20MiB smaller, from unbundling the Android rules'
    runtime dependencies.

This release contains contributions from many people at Google, as well as Andreas Herrmann, Andrew Suffield, Andy Scott, Benjamin Peterson, Ed Baunton, George Gensure, Ian McGinnis, Ity Kaul, Jingwen Chen, John Millikin, Keith Smiley, Marwan Tammam, Mike Fourie, Oscar Bonilla, perwestling, petros, Robert Sayre, Ryan Beasley, silvergasp, Stanimir Mladenov, Travis Cline, Vladimir Chebotarev, ??.
@soonho-tri soonho-tri mentioned this issue May 6, 2019
Closed
@JingyuQian JingyuQian mentioned this issue Aug 22, 2019
Merged
luca-digrazia pushed a commit to luca-digrazia/DatasetCommitsDiffSearch that referenced this issue Sep 4, 2022
Flip --incompatible_disable_legacy_cc_provider
7d1718a 
    Fixes bazelbuild/bazel#7036.

    RELNOTES: Incompatible flag `--incompatible_disable_legacy_cc_provider` has been flipped (see bazelbuild/bazel#7036 for details).
    PiperOrigin-RevId: 240971173
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@oquenchil oquenchil

Labels
incompatible-change Incompatible/breaking change P1 I'll work on this now. (Assignee required) team-Rules-CPP Issues for C++ rules
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
9 participants
@jayconrod @keith @katre @jmillikin @hlopko @meteorcloudy @dslomov @aehlig @oquenchil