use a template for computing output_path for packageable targets
#21175
Conversation
tdyas
force-pushed
the
interpolate_artifact_name
branch
2 times, most recently
from
8696810 to
e9ca2ec
Compare
tdyas
force-pushed
the
interpolate_artifact_name
branch
from
e9ca2ec to
80dd7b3
Compare
| @@ -106,7 +106,8 @@ async def pack_node_package_into_tgz_for_publication( | |||
| ), | |||
| ) | |||
| if field_set.output_path.value: | |||
| digest = await Get(Digest, AddPrefix(result.output_digest, field_set.output_path.value)) | |||
| output_path = field_set.output_path.value_or_default(file_ending=None) | |||
There was a problem hiding this comment.
Bug fix: This code should have been calling .value_or_default.
| @@ -40,16 +41,23 @@ class PyOxidizerOutputPathField(OutputPathField): | |||
| ) | |||
|
|
|||
|
|
|||
| class PyOxidizerBinaryNameField(OutputPathField): | |||
| class PyOxidizerBinaryNameField(StringField, AsyncFieldMixin): | |||
There was a problem hiding this comment.
This field should not have been subclassing OutputPathField since the field PyOxidizerOutputPathField is already a subclass of OutputPathField.
| normalized_spec_path=normalized_spec_path, | ||
| normalized_address=normalized_address, | ||
| file_suffix=file_suffix, |
There was a problem hiding this comment.
Any one: Please propose better names for these.
There was a problem hiding this comment.
Documenting this is more important than bikeshedding the names... Maybe add docs somewhere in a followup?
There was a problem hiding this comment.
Sure. Any suggestions as to location? We have several references to output_path in the docs, but most of the form "You can optionally set the field output_path to change [the generated output name]" and nothing pants package specific from what I can see.
tdyas
changed the title
output_pathoutput_path for packageable targets
@benjyw: Any feedback on the choice of template engine? We seem to have three now:
|
Hmm, good question. The config file templating is really ini-specific (and we inherited it into our toml format). I think the string.Template thing is pretty robust, but is it supported in other languages? What if we replaced this code with Rust in the future? |
…antsbuild#21175) As described in pantsbuild#18659, when a user sets `output_path` on a packageable target, the user gives up the default behavior which uses a normalized version of the target's directory as the directory into which the place the built artifact. While the user could replicate the behavior manually, it would be a better user experience if the user can still have partial access to some of the default behavior. This PR teaches the `output_path` field how to derive its value from a template instead of hard-coding its behavior in Python. The `output_path` field learns the following template replacements: - `spec_path_normalized`: The path to the target's directory ("spec path") with forward slashes replaced by dots. - `target_name_normalized`: The target's name - `file_suffix`: For target's which produce single file artifacts, this is the file type suffix to use. This is empty if not applicable. The default value for the `output_path` field is now `"${spec_path_normalized}/${target_name_normalized}${file_suffix}"` which replicates the existing behavior. `string.Template` is used as the template engine. This PR also fixes some incorrect usages of `output_path` in several backends.
This escapes `$` in default values to avoid them being treated as
template literal substitutions.
Default values end up in a template literal `` `something` ``. If the
string contains `${...}` literally, they'll be treated as a
substitution. This PR escapes all `$`s to avoid this.
This bug/oversight doesn't cause problems with any _current_ values, but
will be required in 2.23.0.dev4 due to the new default value for
`output_path` fields from
pantsbuild/pants#21175.
Example of the effect (from
#234 (comment)):
```diff
- default_repr={`'${spec_path_normalized}/${target_name_normalized}${file_suffix}'`}
+ default_repr={`'\${spec_path_normalized}/\${target_name_normalized}\${file_suffix}'`}
```
As described in #18659, when a user sets
output_pathon a packageable target, the user gives up the default behavior which uses a normalized version of the target's directory as the directory into which the place the built artifact. While the user could replicate the behavior manually, it would be a better user experience if the user can still have partial access to some of the default behavior.This PR teaches the
output_pathfield how to derive its value from a template instead of hard-coding its behavior in Python. Theoutput_pathfield learns the following template replacements:spec_path_normalized: The path to the target's directory ("spec path") with forward slashes replaced by dots.target_name_normalized: The target's namefile_suffix: For target's which produce single file artifacts, this is the file type suffix to use. This is empty if not applicable.The default value for the
output_pathfield is now"${spec_path_normalized}/${target_name_normalized}${file_suffix}"which replicates the existing behavior.string.Templateis used as the template engine.This PR also fixes some incorrect usages of
output_pathin several backends.