Select environment in package goal

[chrisjrn]

Adds support for extracting environment names from package goal targets.

#17129.

[chrisjrn]

@stuhood Plenty more to come, it just appears as though this is the only goal that has a trivial migration of the form recommended in test. The following PR will include the discussed enum to replace environment_migrated = True.

[Eric-Arellano]

Hm, there are a whole bunch of other places that need this if you rg 'Get\(BuiltPackage,

src/python/pants/backend/shell/shell_command.py
src/python/pants/backend/python/util_rules/local_dists.py
src/python/pants/backend/python/goals/run_pex_binary.py
src/python/pants/backend/go/goals/run_binary.py
src/python/pants/backend/python/packaging/pyoxidizer/rules.py
src/python/pants/backend/docker/goals/run_image.py
src/python/pants/backend/docker/util_rules/docker_build_context.py
src/python/pants/jvm/run_deploy_jar.py
src/python/pants/core/target_types.py
src/python/pants/core/goals/package.py
src/python/pants/core/goals/publish.py
src/python/pants/core/goals/test.py

I'm not sure why the test.py one isn't erroring:

pants/src/python/pants/core/goals/test.py

Lines 748 to 762 in 20ac158

	@rule(desc="Build runtime package dependencies for tests", level=LogLevel.DEBUG)
	async def build_runtime_package_dependencies(
	request: BuildPackageDependenciesRequest,
	) -> BuiltPackageDependencies:
	unparsed_addresses = request.field.to_unparsed_address_inputs()
	if not unparsed_addresses:
	return BuiltPackageDependencies()
	tgts = await Get(Targets, UnparsedAddressInputs, unparsed_addresses)
	field_sets_per_tgt = await Get(
	FieldSetsPerTarget, FieldSetsPerTargetRequest(PackageFieldSet, tgts)
	)
	packages = await MultiGet(
	Get(BuiltPackage, PackageFieldSet, field_set) for field_set in field_sets_per_tgt.field_sets
	)
	return BuiltPackageDependencies(packages)

@stuhood I guess it's inheriting the environment from the calling test rules? Which is wrong, we might need to enter a new subgraph.

[chrisjrn]

@Eric-Arellano AIUI, the point of the changes is to make it so that the rules can make use of environments, but it's not until an environment field is added for a given implementation that environments will actually be considered.

I believe in all of the implementation-specific cases above (i.e. backend and jvm), the rules are called from a particular goal that may/may not be environment-aware, and the environment should be inherited from the the initial goal. There is almost certainly work in making sure that goals are not executed for invalid environments (e.g. trying to run any package that isn't specced for a local environment, given run inherently runs an InteractiveProcess)

I suspect you're right about the case about test -- it would inherit the environment, which is in behaviour that would be specced through another field, right?

[Eric-Arellano]

Yeah, it's an error to inherit the environment for every instance returned by ripgrep. For example, with test and the field runtime_package_dependencies, you might be running your test with the Docker image test_env, but we need to build foo.pex inside the Docker env centos6 before we can copy the file into the test sandbox. We need to always respect what the environment of that binary target is set to.

This PR isn't wrong, it's only incomplete. And I now realize the deprecation Stu came up with is a good start, but not sufficient to tackle the problem.

It would be best if this PR updated all call sites of Get(BuiltPackage, to set the environment. Otherwise, open a dedicated ticket to track it.

@stuhood thoughts on how we could ban ever using Get(BuiltPackage, BuildPackageRequest) without also including the EnvironmentName in the Get?

[stuhood]

@stuhood thoughts on how we could ban ever using Get(BuiltPackage, BuildPackageRequest) without also including the EnvironmentName in the Get?

When it happens at the top-level in a @goal_rule, the Goal.environment_migrated change will force you to think about it. But when it happens internally in a @rule, either:

1. the caller would need to remember to change the environment
2. the callee would need to mask/ignore the environment, and lift it from its own target field

I took the first approach in #17106 because there are fewer callers than there are callees: I think that that is still the right approach.

We could probably add an optional facility to @union to require that the "in scope type" is explicitly provided in the Get, rather than just in scope? I think that that could actually be a fairly trivial change, since you could fail eagerly before rule graph solving.

[stuhood]

We could probably add an optional facility to @union to require that the "in scope type" is explicitly provided in the Get, rather than just in scope? I think that that could actually be a fairly trivial change, since you could fail eagerly before rule graph solving.

Would involve adjusting the definition of @union, and then adding an assertion here:

pants/src/python/pants/engine/internals/scheduler.py

Lines 652 to 664 in 20ac158

	unions = [t for t in the_get.input_types if is_union(t)]
	if len(unions) == 1:
	# Register the union by recording a copy of the Get for each union member.
	union = unions[0]
	in_scope_types = union_in_scope_types(union)
	assert in_scope_types is not None
	for union_member in union_membership.get(union):
	native_engine.tasks_add_get_union(
	tasks,
	the_get.output_type,
	tuple(union_member if t == union else t for t in the_get.input_types),
	in_scope_types,
	)

[stuhood]

Independent of the rest of the ease of use changes, this change is correct I think.

There are no PackageFieldSets that have an environment field yet though, so it's a bit of a noop.

[Eric-Arellano]

We could probably add an optional facility to @union to require that the "in scope type" is explicitly provided in the Get, rather than just in scope? I think that that could actually be a fairly trivial change, since you could fail eagerly before rule graph solving.

Yeah, I think that would be useful. I like the modeling of the EnvironmentName being injected directly into the Get - it makes it clear you're changing the whole subgraph. My only concern is how easy it is to miss this

[chrisjrn]

@stuhood I'm moving on with the local-only changes, I can take on the new feature for union once I'm done there. Is there anything that needs to go in a ticket other than what you've described above?

[stuhood]

Is there anything that needs to go in a ticket other than what you've described above?

No... honestly, it might be like a 5 line change, heh. The hardest part will be choosing a name.

[chrisjrn]

Choosing the name, and dealing with all of the errors that fall out of the rule graph?