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

Expose Pants version to BUILD files #18528

Closed
tgolsson opened this issue Mar 19, 2023 · 8 comments
Closed

Expose Pants version to BUILD files #18528

tgolsson opened this issue Mar 19, 2023 · 8 comments

Comments

@tgolsson
Copy link
Contributor

tgolsson commented Mar 19, 2023

It'd be useful to expose the current pants version to build scripts. There's two main reasons for this:

  • it is generally useful to put tool versions into built artifacts, just like git versions; environment information, and so on.
  • it would be useful for third-party plugin developers (and consumers) to handle plugin changes, to gracefully test across multiple pants versions even when those versions might not work everywhere

As a topical example; I'm adding support to my OCI builder backend to support empty base images. I want this to use a real, explicit, target - but I don't want to have a million of them, as it'll always be the same image. Synthetic targets solve this by allowing me to generate this target. However; synthetic targets are a 2.15 feature and my minimum supported is still 2.14 (like Pants itself).

Thus, to provide examples of usage (which also becomes e2e tests!) I ended up wanting to write something like this:

empty_base = "//:empty"  # see [oci].empty_image_target

# using pants 2.15+ there's a built-in for empty bases, but before that we need to declare an empty image to use:
if not pants_at_least("2.15.0.dev0"):
    oci_image_empty(
        name="empty",
    )
    empty_base = ":empty"

oci_image_build(
    name="with_empty_base",
    base=[empty_base],
    packages=[":example"],
    tag="latest",
)

Doing this right now requires some hacking to read and compare the version, since imports are forbidden:

def pants_at_least(version: str) -> bool:
    pants = __import__("pants")

    return pants.version.PANTS_SEMVER >= pants.version.Version(version)

Describe the solution you'd like

If the (stringified?) value of PANTS_SEMVER was exposed to the BUILD files, the first kind of use-cases would be possible - and the second would be possible for someone who cares enough to do it. I don't think this necessarily should be encouraged and thus there's no "easy" comparison, but it's a nice escape hatch for those who find the need or run matrix testing over pants itself and want to early adopt coming features.

Additional context
https://pantsbuild.slack.com/archives/C046T6T9U/p1679234208316439

I'm willing to also do the work required here if we can agree that it should be done (and how ;-))

@stuhood
Copy link
Member

stuhood commented Mar 20, 2023

If the (stringified?) value of PANTS_SEMVER was exposed to the BUILD files, the first kind of use-cases would be possible - and the second would be possible for someone who cares enough to do it. I don't think this necessarily should be encouraged and thus there's no "easy" comparison, but it's a nice escape hatch for those who find the need or run matrix testing over pants itself and want to early adopt coming features.

Sounds good. Rather than stringified though, you might consider exposing a function like pants_version_greater_than_or_equal_to("2.15.x") (excessively long name for demonstration purposes), so that folks also don't have to import Version and/or do semver parsing.

I'm willing to also do the work required here if we can agree that it should be done (and how ;-))

That would be very welcome: thanks!

@tgolsson
Copy link
Contributor Author

I'm thinking forcing users to do it the long/manual way is a hint that maybe what they're doing is a bit hacky. As @thejcannon points out, most users shouldn't ever need to parse versions; it's for CI/testing/plugin development. I'm not sure if an excessively long function gives pause for thought or just invites bug reports about long function names.

@stuhood
Copy link
Member

stuhood commented Mar 20, 2023

I'm thinking forcing users to do it the long/manual way is a hint that maybe what they're doing is a bit hacky. As @thejcannon points out, most users shouldn't ever need to parse versions; it's for CI/testing/plugin development.

I think that it's a fairly reasonable thing to want to do.

I'm not sure if an excessively long function gives pause for thought or just invites bug reports about long function names.

To be clear: I gave it a long name as a strawman: you could definitely pick a better name.

@kaos
Copy link
Member

kaos commented Mar 20, 2023

I think it makes sense to provide the pants version in a ergonomic way. It's useful also for regular use. Consider you rely on a feature but there was a known bug in a prior version. Rather than hitting that bug, you could have a check for it and raise a helpful error instead:

if pants.version <= "2.14.1":
  raise ...

I also think the type of version exposed could be such that it coerces str values during comparison as in the example.

@tgolsson
Copy link
Contributor Author

tgolsson commented Mar 20, 2023

@kaos I'm not sure if that makes sense, in that case you'd just set pants_version = "2.14.2" in your pants.toml, no? I'm not sure raising in a BUILD file makes sense unless you expect those working in the repo to be using PANTS_VERSION regularly. Or maybe you can have 2.14.* as your pants version, so different users might be running different patches?

@kaos
Copy link
Member

kaos commented Mar 20, 2023

[..] for regular use.

OK, maybe this was over-generalizing.. In a single repo, the version will likely be fixed in pants.toml as you say. What I was considering was in cases where you may have a shared/common library of BUILD macros in an organization for instance where it may be used in >1 pants based project, and across those the pants versions may vary/drift. But it also depends on which features are actually used if a particular version of pants would be OK or not.

Ignoring that argument for a second, what I guess is more to the core of what I'm getting at is that I don't see why we'd want to make this "un-wieldy" as a signal to say "stay clear, this is likely not what you want". For us it is no more maintenance to provide the version in an easy to use way, and in most cases it's likely not going to be used any way (nor any more) than a longer/more manual way (whatever that is in practice)--but for whenever there pops up another valid use case, might as well be nice to use, right?

@kaos
Copy link
Member

kaos commented Mar 20, 2023

If we really want to steer users clear of it, could put it behind a feature flag/backend/option whatever in order to make it available/usable which could be documented with caveats and alternatives etc.

@tgolsson
Copy link
Contributor Author

tgolsson commented Mar 21, 2023

OK, maybe this was over-generalizing.. In a single repo, the version will likely be fixed in pants.toml as you say. What I was considering was in cases where you may have a shared/common library of BUILD macros in an organization for instance where it may be used in >1 pants based project, and across those the pants versions may vary/drift. But it also depends on which features are actually used if a particular version of pants would be OK or not.

Thanks, this isn't a use-case I'd thought of! Makes sense to me then; maybe that's a good vote in favour of making version comparison also ergonomic.

Ignoring that argument for a second, what I guess is more to the core of what I'm getting at is that I don't see why we'd want to make this "un-wieldy" as a signal to say "stay clear, this is likely not what you want". For us it is no more maintenance to provide the version in an easy to use way, and in most cases it's likely not going to be used any way (nor any more) than a longer/more manual way (whatever that is in practice)--but for whenever there pops up another valid use case, might as well be nice to use, right?

Gotcha. I'm quite vary of things that might lead to more support load for upstream projects where I'm only an occasional contributor. And if this in an inadvisable pattern then the question is whether it'll let users do things they probably shouldn't be doing, and then they come here to ask. The gating seems sensible as a middle ground, but I'm fine with making it just on-by-default if that makes sense to those that know more about Pants. :-)

kaos pushed a commit that referenced this issue Mar 28, 2023
Proposed fix for #18528.

Very "dumb" implementation exposing it as a constant to all BUILD files.
Unfortunately `packaging.version.Version` cannot be compared to strings,
so I'm shimming it with a local Version class that allows this. I could
do this in a more localized manner as this'll *affect* all uses of
PANTS_SEMVER - but it seems like it'll reduce boilerplate in a lot of
places. Happy to change based on opinion!

We could also use an explicit comparator, or expose `Version` as well:

```py3 
if pants_version_after("2.13"):
```
or
```py3
if PANTS_VERSION >= Version("2.13")
```
@kaos kaos closed this as completed Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants