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

Allow for a bulk assignment of the platform compatibility attribute. #12413

Closed
konste opened this issue Nov 4, 2020 · 14 comments
Closed

Allow for a bulk assignment of the platform compatibility attribute. #12413

konste opened this issue Nov 4, 2020 · 14 comments
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

@konste
Copy link

konste commented Nov 4, 2020

Description of the problem / feature request:

The new feature introduced target_compatible_with attribute, which we plan to use on a substantial (hundreds) amount of targets. For most of them this attribute should be set to the same default value and only very few are different from the rest. We are looking for a way to assign the default value without visiting each and every rule and copy-pasting the same default target_compatible_with attribute.

Feature requests: what underlying problem are you trying to solve with this feature?

Here is a specific problem we are trying to solve with the `target_compatible_with" attribute:
We have thousands of “output” targets (binaries, test_binaries, packages) most of which are compatible with the tree standard platforms: Linux, Darwin, Windows_64. I would rather not visit them all and add them standard “target_compatible_with” attribute – too much hassle and code bloat.
Few of those targets are special and compatible to Windows only – they should be skipped on any other platform and we want to mark them individually.
Very few of these targets are special in the other direction – they are compatible with Emscripten target platform, additionally to the tree standard platforms. Again we don’t mind marking them individually.
What I would like to avoid though is the explicit marking of the bulk of the targets in exactly the same way. Maybe we can introduce a configurable default value for “target_compatible_with” for the package, workspace, or simply in .bazelrc which we should be able to change on the target level. I would hate to visit each and every target to add that it is not compatible with Emscripten.

What's the output of bazel info release?

3.7.0

@philwo philwo added team-Configurability platforms, toolchains, cquery, select(), config transitions untriaged type: feature request labels Nov 10, 2020
@sventiffe sventiffe added P2 We'll consider working on this in future. (Assignee optional) and removed untriaged labels Nov 10, 2020
@philsc
Copy link
Contributor

philsc commented Nov 11, 2020

This is probably the simplest proposal I can think of, based on your description:

package(
    default_target_compatible_with = selects.with_or({
        ("@platforms//os:windows", "@platforms//os:macos", "@platforms//os:linux"): []
        "//conditions:default": "//:not_compatible",
    }),
)

Either that or create a command line flag. Though I'm really not sure what that command line flag would look like. A single string would not be expressive enough unless it referred to a target that somehow expressed default compatibility.

@philsc
Copy link
Contributor

philsc commented Nov 11, 2020

/cc @gregestren and @katre in case they have ideas/thoughts.

@konste
Copy link
Author

konste commented Nov 11, 2020

I am thinking about the scenarios this feature may help with. I don't have hard data to support this vision, but it feels like for the typical projects most targets (and most packages) would have the same default target_compatible_with and only very few targets would need to be configured differently. In our particular case the ratio is hundreds of packages (and thousand of targets) to have default compatibility and only a few are the exceptions. Based on that the command line flag (which we would be able to put into .bazelrc) looks ideal. If you are not sure what this command line flag should look like we can do something similar to what build settings do i.e. the flag points to the code (list? target?) which describes the default compatibility.

@AustinSchuh
Copy link
Contributor

I'd need to think through it a bit more, but a command line flag implies that there is a global setting which makes sense across a project both when used as @, and when used as an external repo. I'm not sure that is the right message.

@konste
Copy link
Author

konste commented Nov 12, 2020

I admit that defining default compatibility on the package level (as shown above) is cleaner. Trying to think how can we accommodate for this model. We should be able to load a common .bzl file for all (or most) packages involved and we probably can define the default compatibility criteria in that common .bzl file. What would be the way to apply that default compatibility criteria defined in the common .bzl file to each package, so that it would require minimal changes to the package code beyond the load statement?

@philsc
Copy link
Contributor

philsc commented Nov 12, 2020

I'd need to think through it a bit more, but a command line flag implies that there is a global setting which makes sense across a project both when used as @, and when used as an external repo. I'm not sure that is the right message.

Good point!

What would be the way to apply that default compatibility criteria defined in the common .bzl file to each package, so that it would require minimal changes to the package code beyond the load statement?

I'm not aware of any existing mechanism where load() statements have side-effects. It's probably better not do do that. That being said, it's possible that package() works inside a macro. I don't know if package() is special in some way.

# (defaults.bzl)
def custom_package(**kwargs):
    native.package(
        default_target_compatible_with = selects.with_or({
            ("@platforms//os:windows", "@platforms//os:macos", "@platforms//os:linux"): []
            "//conditions:default": "//:not_compatible",
        }),
        **kwargs
    )

# (BUILD)
load(":defaults.bzl", "custom_package")
custom_package()

@konste
Copy link
Author

konste commented Nov 12, 2020

I agree that load() statement better not have side effects. If native.package() can be applied using the macro it would be totally sufficient. I am going to try to prototype this using visibility attribute to simulate the proposed default_target_compatible_with attribute. One more consideration is that individual packages may already use native.package() method to set other package attributes. Do you know if package() can be applied more than once (and combine the attributes)?

@konste
Copy link
Author

konste commented Nov 12, 2020

I just noticed how you use **kwargs to accumulate the attributes for the package() call. So if package () cannot be invoked more than once (for the same package) then we use macro to generate the dictionary with the default attributes, including proposed default_target_compatible_with and individual packages can amend that dictionary and apply package(dict).

@philsc
Copy link
Contributor

philsc commented Nov 28, 2020

@konste, I was just made aware of BUILD file preludes by @bsilver8192 . I haven't looked into it too much, but it seems possible to load wrapper macros in there that set a default target_compatible_with. I mention this as a possible work around until this issue gets fixed.

@konste
Copy link
Author

konste commented Nov 28, 2020

Yes I am aware of this rather hidden feature, but hesitate to take a dependency on it as its future is dim - Bazel team still has not decided if they are going to document it or remove it.

@gregestren
Copy link
Contributor

To play devil's advocate, there's something to be said for leveraging tools that make otherwise large refactorings manageable. Hundreds of packages doesn't intrinsically have to be an obstacle to a maintainable approach with the package() function.

Buildozer is a great such tool (I don't know how well it handles package() specifically).

You could even imagine a presubmit check that automatically tags new packages with whatever defaults you need. Of course this all depends on the details of your particular dev pipeline.

@konste
Copy link
Author

konste commented Dec 7, 2020

Bulk edits are a viable solution for a real monorepo. In our case a substantial part of the code lives in individually owned Git repos and making a change to each of them requires a merge request and approval from the respective team by their respective rules. This makes any potential bulk update a nightmare.

@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 17, 2023
@github-actions
Copy link

github-actions bot commented May 2, 2023

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 May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

No branches or pull requests

6 participants