-
-
Notifications
You must be signed in to change notification settings - Fork 638
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
Use vcs_version
for Pants' version
#20438
base: main
Are you sure you want to change the base?
Conversation
Draft PR until I fix the Rust side of things and actually update the docs/automation |
I love the idea! So when do wheels get built? I guess we always build wheels in main and release branches anyway? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM mod the rust bit of course!
Yup! |
Nice! I like the idea. The version looks quite "rich", potentially changing for every single commit. I wonder if this will cause cache hit rates for tests to be essentially zero: anything that depends on the version will have its digest change every build, and, I imagine most tests do (transitively) via things like doc_url? (I'm on mobile so can't confirm) If this is the case, can we reduce the impact somehow? |
Welp, I was ready to tell you the multiple reasons it won't be a problem, but that just led to a different, worse problem. All the tests just die 😂 So in fixing the deaths, I'll make sure we just use some constant version for tests (like 3.0) |
Ok @huonw I moved the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool stuff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preferably the new fields on vcs_version would be in a separate commit from the changes in the Pants repo, so it's obvious that there are two parts here - a user facing feature enhancement, and an internal Pants repo application of that.
|
Hmm the failure alludes me. The But the |
Have we deprecated the use of |
Yes, IIRC we deprecated it a while back with the suggestion of just checking out the repo locally and using PANTS_FROM_SOURCES |
Excellent. That'll make life a little easier over in scie-pants land. |
# NB: Don't dirty the version in CI | ||
local_scheme="no-local-version" if env("CI", "0") == "1" else None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT about doing this unconditionally? Why might it be handy?
# NB: Don't dirty the version in CI | |
local_scheme="no-local-version" if env("CI", "0") == "1" else None, | |
local_scheme="no-local-version", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
User reports, mostly. Whens someone is using PANTS_SOURCE=...
.
VERSION: str = ( | ||
# Do not remove/change this env var without coordinating with `pantsbuild/scie-pants` as it is | ||
# being used when bootstrapping Pants with a released version. | ||
os.environ.get(_PANTS_VERSION_OVERRIDE) | ||
or | ||
# NB: This is only relevant for the Pants repo itself | ||
( | ||
_determine_version_from_pants_source() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to computing the version 'accurately' when running from sources? What's the downside if we just substituted in a fixed dummy version here, and thus avoided needing to pull in setuptools_scm
?
In particular, I wonder if Pants supports running from a git-less tarball currently... if so, requiring setuptools_scm
and its git interactions might break it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In those cases (which we don't document supporting currently) you're outside the normal flow. in that case, you'd just set _PANTS_VERSION_OVERRIDE
on invocation (among other hacks, I imagine)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh and I guess to answer your questions. I'd rather err on the side of caution. Where we could reasonably ask users to run --version
and it gives us something useful.
Then, if users want to exist outside of this model, they can do so by either setting the env var, or modifying the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also toying with speeding up option registration by doing it at build time instead of runtime, and in that case it would have to be tied to a specific version, and it would be chaotic to not invalidate that correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just so I'm clear, is that something that'll preclude and block this PR? Or is it something that'll be handled in yours? Or something else?
Just trying to understand the impact of your statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just providing a reason that every version should have a well-defined version, even when running from sources, rather than using a fixed dummy.
Ah! But So, that at least would need to be fixed along with this change, then I think.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting closer!
Good catch re the scie-pants
checks. Even if we do land the scie-pants change, landing this might be a breaking change, as anyone using PANTS_SOURCE
will start getting errors that are very opaque:
Error: No such file or directory (os error 2)
Given PANTS_SOURCE
is a fairly niche/experts-only feature, that's probably okay? So, instead of doing a deprecation cycle with
MINIMUM_SCIE_PANTS_VERSION = Version("0.10.0") |
PANTS_SOURCE
. Thoughts?
@@ -170,7 +170,7 @@ def ensure_category_label() -> Sequence[Step]: | |||
|
|||
def checkout( | |||
*, | |||
fetch_depth: int = 10, | |||
fetch_depth: int = 250, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this change?
GIT_TAG=$(git describe --tags) | ||
export GIT_TAG |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about giving this a more distinct name? The current name is generic enough that it could come from a CI provider or similar? E.g. PANTS_CARGO_GIT_TAG
or something (although the PANTS_
prefix is potentially something to avoid)?
Also, does this need to sync up with the version used in Python? If so, this might be different when running from source?
Yea, I think if we release a new version of |
Another question: if the version is only known once tagged, we can easily forget to handle deprecations (e.g. remove or postpone) before tagging, and it seems they can happen in a way that causes the release build to break. Is that something we should be prepared to handle? In particular, #20609 sets up 2.21.0.dev0, but CI fails hard when bootstrap pants, with errors like the following:
Referring to pants/src/python/pants/option/global_options.py Lines 1585 to 1601 in 8481389
(NB. I'm a bit confused by this, because the corresponding deprecation warning doesn't appear on normal builds. 🤷♂️ ) In this case, it was caught by PR CI, but if I had just tagged In either case, I imagine we can usually just fix the problem and do the next release with low impact... assuming we get the deprecations right so that it's always dev release where behaviour changes, and not a stable release. But, we don't always get it right (see `crossversion="partial" in #20616). So, should we think of ways to mitigate this risk? Just brainstorming ideas:
|
This makes sense to me.
I think one way to smoke this out is that you can override the version locally when running tests... but then running the full suite is likely not practical so wouldn't catch everything. |
Maybe we need a new goal that lists upcoming deprecations? |
This change makes it so we can version a release simply from tagging a commit (therefore not requiring a PR to release)
It accomplishes this by:
VERSION
filescie-pants
to reflect this changeversion.py
to usesetuptools_scm
to generate the version on-the-fly ifRUNNING_PANTS_FROM_SOURCES
version_scheme
andlocal_scheme
fields tovcs_version
and wires them upsetuptools_scm
doesn't support.devN
builds that aren't.dev0
without a different schemeVERSION
file (atsrc/python/pants/_version/VERSION
) to be generated viavcs_version
pants_dev_requirements.txt
file which is solely used for Pants dev venv. And addsetuptools_scm
Demo: