Add console os constraints and config_settings.

[zjturner]

No description provided.

[facebook-github-bot]

@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. (Because this pull request was imported automatically, there will not be any future comments.)

[IanChilds]

Why do these need to go into prelude? Where are they going to be used?

[zjturner]

I am not sure where else it could really go and be usable. It would be used anywhere that a config//os: constraint is currently used. There's too many to list, but just as one example, this is a frequent use case for us:

cxx_library(
    name = "foo",
    srcs = [
        <common sources>
    ] + select({
        "config//os:windows": _windows_sources,
        "config//os:linux": _linux_sources,
        "config//os:macos": _macos_sources,
        "config//os:ps4": _playstation_sources,   # we need this
    })
)

Same argument applies to deps, and most other selectable args.

When deciding whether it made sense to put these here, I looked at the surrounding code in the file. There are other constraints like illumos, netbsd, and fuchsia that if you grep the prelude for them, 0 hits come up. And these are prefaced by a comment that says "Long tail but contemporarily relevant operating systems". I assume Meta internally has lots of internal build code that refers to these OSes, and the same would apply here. So it seems appropriate to me for these to be here, but LMK your thoughts.

[IanChilds]

Hmm, @dtolnay added those (in D45585036), but I don't see that they are actually used anywhere so should probably be removed too I think, but maybe I'm misunderstanding something here

[zjturner]

How would you suggest I accomplish selecting on other operating systems if not by adding a constraint to the operating system setting? Isn't that what it's for? I could do something like:

select({
    "internal//os_visibility:private": {
        "internal//os:ps4": [],
        "internal//os:ps5": [],
    },
    "internal//os_visibility:oss": {
        "config//os:windows": [],
        "config//os:macos": [],
    }
})

but that seems quite extreme given that that the constraints I'm adding actually are operating systems, that conceptually and logically do belong in a list of operating systems. If I have to go that route, I'll probably instead just fork the prelude internally. But I think it's a reasonable ask to get these into prelude since this is where they make logical sense, and if Meta were to ever support these operating systems, I have a strong suspicion this is exactly where the constraints would go.

If this isn't going to work, then some clear guidelines about what the bar is for adding a constraint to this setting would be appreciated, along with some guidance about what to do when one does have a need to support targeting operating systems that don't have an associated constraint.

[dtolnay]

I think there are some misunderstandings in the previous comment. You can just add:

constraint_value(
    name = "ps4",
    constraint_setting = "prelude//os/constraints:os",
)

anywhere in your own codebase and that becomes a valid value of OS. If the prelude isn't going to be using this in any of the prelude's toolchain definitions, there is no technical difference between putting this in prelude or elsewhere. There is no need for "os_visibility" either way.

select({
    "prelude//os:windows": ...,
    "//path/to:ps4": ...,
})

[zjturner]

I think there are some misunderstandings in the previous comment. You can just add:

constraint_value(
    name = "ps4",
    constraint_setting = "prelude//os/constraints:os",
)

anywhere in your own codebase and that becomes a valid value of OS. If the prelude isn't going to be using this in any of the prelude's toolchain definitions, there is no technical difference between putting this in prelude or elsewhere. There is no need for "os_visibility" either way.

select({
    "prelude//os:windows": ...,
    "//path/to:ps4": ...,
})

I didn't realize that would work. If that does in fact work then I agree there is no need for this. I didn't think you could select on constraint_values that were namespaced in different folders. I'll give this is a try, and if this does work then I'll close this issue.