pantsbuild / pants

The Pants Build System
https://www.pantsbuild.org
Apache License 2.0
3.35k stars 640 forks source link

Ability to specify default `platforms` / `complete_platforms` for all `pex_binary` targets #15764

Open danxmoran opened 2 years ago

danxmoran commented 2 years ago

Is your feature request related to a problem? Please describe.

We primarily use Macs for local development, Linux for CI, and deploy into (containerized) Linux environments. We want our pex_binary targets to be usable in all these contexts:

We're currently working around this locally using a wrapper script that runs Pants inside a Linux container (CI works as-is since it's running Linux). We get semi-frequent reports that things are "broken" from new hires who don't know about the wrapper script and try to directly ./pants package a Dockerfile on Mac, producing a Linux image containing a Mac-only PEX.

Describe the solution you'd like

pex_binary targets already have platforms and complete_platforms fields to help create multi-platform PEXes. I would like some way to set defaults for those fields globally, maybe as part of the pex-binary-defaults subsystem?

It's important to me that this is a config setting because I'd ideally like to use different values for local runs vs. CI:

Describe alternatives you've considered

I considered implementing this with a macro, but I'm not sure how I'd be able to set different values for local vs. CI with that setup. The macro also wouldn't be auto-generated by ./pants tailor so we'd need to add custom linting to prevent people from using the default pex_binary target.

Eric-Arellano commented 2 years ago

Would Pants's 2.14's new set_defaults() mechanism do the trick for you?

# BUILD
set_defaults({pex_binary: {"platforms": []}}

Individual binaries can still override the value.

--

It sounds like maybe not, because you want to dynamically change the value in local vs CI?

danxmoran commented 2 years ago

It sounds like maybe not, because you want to dynamically change the value in local vs CI?

Yeah, exactly. We could probably do some hacks with overwriting BUILD files at runtime in CI, but it'd be nice not to have to do that.

Eric-Arellano commented 2 years ago

@stuhood @thejcannon what are your thoughts on this? I know we've discussed in the past getting rid of subsystem defaults in favor of set_defaults -- but this ticket is a great point in the value of having both, so that you can override via CLI and env vars.

I'm personally +1 for extending pex-binary-defaults with this.

thejcannon commented 2 years ago

Yeah, I've come around to hierarchical field defaults, however I think we should start thinking about a more cohesive strategy than our current ad-hoc one.

I'm thinking each field could have it's default be customizable via options. but that gets hairy and opens several cans of worms.

Eric-Arellano commented 2 years ago

I'm thinking each field could have it's default be customizable via options. but that gets hairy and opens several cans of worms.

One concern I had is help not being useful, but Stu added an API where that can be fixed :)

So now my main concern is a proliferation of options and subsystems.

--

What do you think about only having options for some particularly important fields? resolve is one of them. Here, @danxmoran makes the case for platforms. In contrast, zip_safe from pex_binary is probably not really worth an option...and set_defaults() gives a workaround.

thejcannon commented 2 years ago

One attractive thing about every field having this feature is we don't have to pick-and-choose which ones make sense (and to everyone)

Eric-Arellano commented 2 years ago

is we don't have to pick-and-choose

I'd rather that we have some extra decision making if it results in less noise for users. If we could figure out a way where it isn't one option per field per target, I would be more inclined to consider everything having it. Otherwise, that feels extremely noisy.

thejcannon commented 2 years ago

Another thorn here is one field can be used by any number of targets, making the naming strategy a bit hairy.

At the very least we should codify a convention for naming of these things.

stuhood commented 2 years ago

Reusing the "shape" of the set_defaults API, but within an options scope could make sense.

[target.defaults]
python_tests = { platforms = [] }
thejcannon commented 2 years ago

Yeah that's somewhat what I had in mind, hide it behind a dedicated subsystem

Eric-Arellano commented 2 years ago

It indeed would need to be something like that nested dict option. There are some awkward parts to that though:

1) Merging wouldn't really be a thing, given dict options. If CI wants different defaults than local, you will need to reproduce values in common. 2) You have to enumerate every target type when setting the same field, e.g. resolve.

huonw commented 9 months ago

The "modern" phrasing of set_defaults seems to be __defaults__, e.g.

__defaults__({
  (pex_binary, pex_binaries): dict(complete_platforms=["path/to:platform"])
})

(With or without extend.)

I think this could potentially be customised on CI using the env() build file symbol, e.g. a "a" if env("CI", "0") == "1" else "b" or even:

__defaults__({
  (pex_binary, pex_binaries): dict(complete_platforms=[
    env("DEFAULT_COMPLETE_PLATFORM_ADDRESS", "path/to/local:platform")
  ])
})

https://github.com/pantsbuild/pants/issues/20506 is also related, but I think it doesn't consider the "override in CI" case.

kaos commented 9 months ago

There's also an option of using dedicated BUILD files for CI (and/or local etc), as presented in this comment leveraging the GLOBAL.build_pattern option: https://github.com/pantsbuild/pants/issues/20506#issuecomment-1960851011