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

Platform/toolchain support for C++ rules #6516

Closed
gregestren opened this issue Oct 25, 2018 · 20 comments
Closed

Platform/toolchain support for C++ rules #6516

gregestren opened this issue Oct 25, 2018 · 20 comments
Assignees
@katre
Labels
P2 We'll consider working on this in future. (Assignee optional) stale Issues or PRs that are stale (no activity for 30 days) team-Configurability platforms, toolchains, cquery, select(), config transitions type: feature request

Comments

@gregestren
Copy link
Contributor

Tracking issue on Bazel Configurability Roadmap

Goal:

  • C++ toolchains can be defined with Bazel's generic toolchain machinery
  • C++ rules read these toolchains
  • C++ rules resolve properly against platforms
@gregestren gregestren added team-Configurability platforms, toolchains, cquery, select(), config transitions team-Rules-CPP Issues for C++ rules labels Oct 25, 2018
@hlopko
Copy link
Member

hlopko commented Oct 26, 2018

Before I leave for 3 weeks of vacation, let me say I'm not aware of any work needed now that 683c302 is submitted. John, do you know about anything?

@katre
Copy link
Member

katre commented Oct 26, 2018

Testing, that's all. Enjoy your vacation!

@hlopko
Copy link
Member

hlopko commented Nov 28, 2018

I'm back! :)

Could you give me a list of behaviors/features that I should test? I don't know where to start :)

@katre
Copy link
Member

katre commented Nov 28, 2018

The big areas we've had problems with were around finding the correct CROSSTOOL file for a cc_toolchain. @nlopezgi might have more ideas, but honestly I'd just start taking any cc rules integration tests, and try running them with toolchain resolution enabled for cc toolchains, to see what happens.

@nlopezgi
Copy link
Contributor

I'd like to see tests that run with custom values for some of the ENV variables that cc_config uses. Most of the times we have seen issues is because we set custom values for some of these ENV variables in our rules (https://github.com/bazelbuild/bazel-toolchains/blob/master/rules/environments.bzl#L30).

@nlopezgi
Copy link
Contributor

This is a bigger ask: I would like to see changes that impact crosstool/c_toolchain compatibility to be guarded behind --incompatible flags. Let me expand that: Currently, when there are changes to the CROSSTOOL / c_toolchain, there is no guarding them behind an --incopatible flag (afaik). For users that have hand built or checked in versions of a CROSSTOOL / c_toolchain, they have no way of preventing Bazel changes in release X+1 from breaking them.
For example, a recent change to how toolchains are selected using as key only e.g., "k8" instead of e.g., "k8|clang" could have been guarded behind an --incompatible flag for ~1 bazel release. That way, I could have tested that with version X my c_toolchain that did not have the "k8|clang" would work, but if I use --incompatible_someting_something it would break.

@nlopezgi
Copy link
Contributor

nlopezgi commented Nov 29, 2018 •
edited
Loading

The other problem I had in the past is that adding the
enabled_toolchain_types=@bazel_tools//tools/cpp:toolchain_type
made it so that the OPT toolchain was always selected and linkopt flags were not passed (#5291)
(not sure if this is relevant to this thread or if it has been fixed already)

@hlopko
Copy link
Member

hlopko commented Nov 30, 2018

Hi Nick!

  1. tests for cc_configure, absolutely, but since we plan a big rewrite of cc_configure because Rewrite CROSSTOOL in Starlark #5380, we will only add tests during the rewrite. I think it's independent from this issue.
  2. we (C++ rules) already do that, and we did that before it was cool. This is the flag for the change that broke you: Disallow using CROSSTOOL to select the cc_toolchain label #6382. The problem was that it took us too long to polish the policy, so the upcoming incompatible changes announcement didn't receive due attention.
  3. I pinged linkopts flags are not passed when cpp toolchains are enabled #5291, let's see what are the next steps.

Thanks for chiming in!

@katre
Copy link
Member

katre commented Nov 30, 2018

Also, this issue is specifically for "cc rules work when --enabled_toolchain_types=@bazel_tools//tools/cpp:toolchain_type is passed". Other cc rules issues should be filed separately.

@nlopezgi
Copy link
Contributor

nlopezgi commented Dec 7, 2018 •
edited by hlopko
Loading

Thanks for the clarifications @hlopko , sorry for tagging along other stuff here @katre

@hlopko hlopko mentioned this issue Dec 17, 2018
Closed
@jin jin added untriaged and removed untriaged labels Jan 14, 2019
@jin
Copy link
Member

jin commented Jan 14, 2019

@gregestren could you add a priority to this issue, please?

@katre katre added the P1 I'll work on this now. (Assignee required) label Jan 15, 2019
@gregestren
Copy link
Contributor Author

See #7260

@katre katre mentioned this issue Feb 12, 2019
Open
This was referenced Feb 12, 2019
Closed
Closed
Closed
@excitoon excitoon mentioned this issue Feb 15, 2019
Closed
@dslomov
Copy link
Contributor

dslomov commented Feb 15, 2019

Please do not assign issues to more than one team

@dslomov dslomov removed the team-Configurability platforms, toolchains, cquery, select(), config transitions label Feb 15, 2019
bazel-io pushed a commit that referenced this issue Mar 1, 2019
@hlopko @copybara-github
Add CcToolchainInfo.all_files field
f59038d 
Previously, we accessed that information from TransitiveInfoCollection, but when we migrate to platforms/toolchains we only have access to the toolchain provider. This cl adds a field on the `CcToolchainInfo` so we can migrate C++ rules to platforms/toolchains.

#6516

RELNOTES: None.
PiperOrigin-RevId: 236351968
bazel-io pushed a commit that referenced this issue Mar 13, 2019
@hlopko @copybara-github
Add toolchains attribute to compiler_flag
5af2cbf 
This is needed to build Bazel when --incompatible_enable_cc_toolchain_resolution is set.

#6516
#7260

RELNOTES: None.
PiperOrigin-RevId: 238209238
@gregestren gregestren mentioned this issue Jul 10, 2020
Open
@gregestren
Copy link
Contributor Author

Update: a major reason this hasn't been toggled on yet is it'll break Android or iOS projects with C++ native deps. Configurability devs are doing a concerted focus now on adding platform/toolchain support for Android builds to unblock. We may be getting support for doing the same for iOS builds too, although I'm unclear on how much yet.

@c-mita c-mita added team-Configurability platforms, toolchains, cquery, select(), config transitions and removed team-Rules-CPP Issues for C++ rules labels Nov 23, 2020
@c-mita
Copy link
Member

c-mita commented Nov 23, 2020

Changing team to reflect who's been doing the work.

@katre
Copy link
Member

katre commented Nov 23, 2020

Downgrading priority to reflect the current level of activity.

@katre katre added P2 We'll consider working on this in future. (Assignee optional) type: feature request and removed P1 I'll work on this now. (Assignee required) labels Nov 23, 2020
@zhlhahaha zhlhahaha mentioned this issue Jan 21, 2021
Closed
luca-digrazia pushed a commit to luca-digrazia/DatasetCommitsDiffSearch that referenced this issue Sep 4, 2022
Expose cc_toolchain.static_runtime_lib and cc_toolchain.dynamic_runt…
d021b59 
…ime_lib to Starlark

    This is required since existing cc_toolchain.all_files doesn't usually contain C++ runtime library.

    Progress towards bazelbuild/bazel#6516

    RELNOTES: `cc_toolchain.static_runtime_lib` and `cc_toolchain.dynamic_runtime_lib` are now exposed to Starlark.
    PiperOrigin-RevId: 242863875
luca-digrazia pushed a commit to luca-digrazia/DatasetCommitsDiffSearch that referenced this issue Sep 4, 2022
Do not try to unzip fdo profile in the host configuration
1b07f74 
    bazelbuild/bazel#6516

    RELNOTES: None.
    PiperOrigin-RevId: 241523004
luca-digrazia pushed a commit to luca-digrazia/DatasetCommitsDiffSearch that referenced this issue Sep 4, 2022
Analyze target libc in cc_toolchain
eaf6f66 
    With this cl we will pass target libc to the host configuration and analyze it
    from cc_toolchain attribute. This is needed to migrate C++ rules to platforms,
    under which cc_toolchain is analyzed in the host configuration.

    bazelbuild/bazel#6516

    RELNOTES: None
    PiperOrigin-RevId: 241043559
luca-digrazia pushed a commit to luca-digrazia/DatasetCommitsDiffSearch that referenced this issue Sep 4, 2022
Pass AdditionalBuildVariableComputer to CcToolchainProvider
6027c20 
    This way, we can compute build variables for a different configuration than the
    one present at cc_toolchain analysis. Do bad things in the process (such as storing BuildOptions in FeatureConfigurationForStarlark or creating AppleConfiguration inline in the AdditionalBuildVariableComputer).

    bazelbuild/bazel#6516

    RELNOTES: None
    PiperOrigin-RevId: 240997356
luca-digrazia pushed a commit to luca-digrazia/DatasetCommitsDiffSearch that referenced this issue Sep 4, 2022
Refactor AppleCcToolchain to not pass ruleContext around too much
32d6c49 
    This is a preparation for moving variables away from toolchains to rules
    (because toolchains don't get analyzed in the same configuration as rules).

    bazelbuild/bazel#6516

    RELNOTES: None
    PiperOrigin-RevId: 240759048
luca-digrazia pushed a commit to luca-digrazia/DatasetCommitsDiffSearch that referenced this issue Sep 4, 2022
Pass FDO options to the host configuration, but only enable FDO for …
3bddb34 
…target configuration

    This is there to allow cc_toolchain to detect that FDO is active, and to
    preprocess profiles, while being analyzed in the host configuration. FDO related
    options are passed to the host configuration, they are preprocessed in the host
    configuration, but then used in the target configuration. Extra care is made to
    not enable FDO in the host configuration (as this is the existing behavior).

    bazelbuild/bazel#6516

    RELNOTES: None
    PiperOrigin-RevId: 240721577
luca-digrazia pushed a commit to luca-digrazia/DatasetCommitsDiffSearch that referenced this issue Sep 4, 2022
Introduce FdoHelper
c580bf3 
    Fdo related logic is complicated enough to have it in a separate class.

    bazelbuild/bazel#6516

    RELNOTES: None
    PiperOrigin-RevId: 240376113
luca-digrazia pushed a commit to luca-digrazia/DatasetCommitsDiffSearch that referenced this issue Sep 4, 2022
Move some error reporting from cc_toolchain to rules
bb96c93 
    This is unfortunate, confusing for the user, and less accurate than what the
    original behavior was, but because cc_toolchain cannot access (currently, with
    platforms enabled) the target configuration, there is nothing we can do other
    than showing the error for the target.

    bazelbuild/bazel#6516

    RELNOTES: None
    PiperOrigin-RevId: 240345500
luca-digrazia pushed a commit to luca-digrazia/DatasetCommitsDiffSearch that referenced this issue Sep 4, 2022
Remove unused arguments
b5ed979 
    This falls nicely into our quest to not use configuration from cc_toolchain :)

    bazelbuild/bazel#6516

    RELNOTES: None
    PiperOrigin-RevId: 240338037
luca-digrazia pushed a commit to luca-digrazia/DatasetCommitsDiffSearch that referenced this issue Sep 4, 2022
Don't precompute coverage environment in cc_toolchain
e8911d8 
    Instead, compute it for each target separately using its rule context.

    bazelbuild/bazel#6516

    RELNOTES: None
    PiperOrigin-RevId: 240330179
luca-digrazia pushed a commit to luca-digrazia/DatasetCommitsDiffSearch that referenced this issue Sep 4, 2022
Don't thread coverage information through CcToolchainProvider
7dff199 
    Instead read it from the CppConfiguration passed from the target.

    bazelbuild/bazel#6516

    RELNOTES: None
    PiperOrigin-RevId: 240317365
luca-digrazia pushed a commit to luca-digrazia/DatasetCommitsDiffSearch that referenced this issue Sep 4, 2022
Add --incompatible_require_ctx_in_configure_features
d0d4f72 
    In order to migrate C++ rules to platforms, we need the access to the C++ configuration fragment from the caller rule in Starlark APIs. All existing APIs have already access to it, but cc_common.configure_features doesn't. This incompatible change adds a ctx argument to configure_features.

    bazelbuild/bazel#7793
    bazelbuild/bazel#6516

    RELNOTES: Added `--incompatible_require_ctx_in_configure_features`, see bazelbuild/bazel#7793 for details.
    PiperOrigin-RevId: 240099411
copybara-service bot pushed a commit that referenced this issue Sep 30, 2022
@gregestren @copybara-github
Don't propagate FDO to exec actions.
56dba00 
Benefits:

 - More efficient caching for exec actions (which shouldn't integrate FDO data)
 - simpler, more straightforward code
 - resolves outdated logic from before toolchain transitions (https://github.com/bazelbuild/proposals/blob/main/designs/2020-02-07-toolchain-transition-migration.md)

#6516

PiperOrigin-RevId: 478011721
Change-Id: I8df049eeb78003fa9e979a91313e45cb9de95d01
aiuto pushed a commit to aiuto/bazel that referenced this issue Oct 12, 2022
@gregestren @aiuto
Don't propagate FDO to exec actions.
14e765c 
Benefits:

 - More efficient caching for exec actions (which shouldn't integrate FDO data)
 - simpler, more straightforward code
 - resolves outdated logic from before toolchain transitions (https://github.com/bazelbuild/proposals/blob/main/designs/2020-02-07-toolchain-transition-migration.md)

bazelbuild#6516

PiperOrigin-RevId: 478011721
Change-Id: I8df049eeb78003fa9e979a91313e45cb9de95d01
@github-actions
Copy link

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 2+ years. It will be closed in the next 14 days unless any other activity occurs or one of the following labels is added: "not stale", "awaiting-bazeler". Please reach out to the triage team (@bazelbuild/triage) if you think this issue is still relevant or you are interested in getting the issue resolved.

@github-actions github-actions bot added the stale Issues or PRs that are stale (no activity for 30 days) label Apr 12, 2023
@github-actions
Copy link

This issue has been automatically closed due to inactivity. If you're still interested in pursuing this, please reach out to the triage team (@bazelbuild/triage). Thanks!

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Apr 27, 2023
tymurmustafaiev pushed a commit to tymurmustafaiev/rules_swift that referenced this issue Jul 19, 2023
@swiple-rules-gardener
Add ctx argument to cc_common.configure_features
7cf1ce4 
In order to migrate C++ rules to platforms, we need the access to the C++
configuration fragment in Starlark APIs. All existing APIs have already access
to it, but cc_common.configure_features doesn't. This change adds a
ctx argument to configure_features.

This is the migration needed for
bazelbuild/bazel#7793, and is part of the effort for
bazelbuild/bazel#6516.

If the rule doesn't depend on cpp fragment yet, you will have to add `fragments
=['cpp']` argument to the rule() call.

Note that this behavior is only available in Bazel 0.25 (to be released this month).

RELNOTES: None.
PiperOrigin-RevId: 247206987
tymurmustafaiev pushed a commit to tymurmustafaiev/rules_swift that referenced this issue Jul 19, 2023
@swiple-rules-gardener
Add ctx argument to cc_common.configure_features
2ddb4cc 
In order to migrate C++ rules to platforms, we need the access to the C++
configuration fragment in Starlark APIs. All existing APIs have already access
to it, but cc_common.configure_features doesn't. This change adds a
ctx argument to configure_features.

This is the migration needed for
bazelbuild/bazel#7793, and is part of the effort for
bazelbuild/bazel#6516.

If the rule doesn't depend on cpp fragment yet, you will have to add `fragments
=['cpp']` argument to the rule() call.

Note that this behavior is only available in Bazel 0.25 (to be released this month).

RELNOTES: None.
PiperOrigin-RevId: 248534309
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@katre katre

Labels
P2 We'll consider working on this in future. (Assignee optional) stale Issues or PRs that are stale (no activity for 30 days) team-Configurability platforms, toolchains, cquery, select(), config transitions type: feature request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@katre @jin @hlopko @c-mita @nlopezgi @dslomov @gregestren