[releases] Harmonize versions and fix PyPI publishing - #1687
[releases] Harmonize versions and fix PyPI publishing#1687BenjaminPelletier wants to merge 5 commits into
Conversation
| -f monitoring/Dockerfile \ | ||
| -t "${DOCKER_TAG}" \ | ||
| --build-arg version="${VERSION_STR}" \ | ||
| --build-arg commit_hash="${COMMIT_HASH}" \ |
There was a problem hiding this comment.
This changed from HEAD to COMMIT_HASH, witch may include the -dirty flag (nit: and it's not an hash).
This is injected in urls by repo_url_of and generate invalid links.
There was a problem hiding this comment.
Thanks; this was unintentional and just trying to put everything related to versions in get_version.py -- I've updated to use the full commit hash (in to-be-pushed commit).
| help=( | ||
| "Explicit output format.\n" | ||
| " pep440: Canonical PEP440 version (e.g., '0.31.0', '0.31.0rc1', '0.31.0+gd56bb4d.dirty'); fails for malformed pre-releases (-RC, -1.2, etc.).\n" | ||
| " imagetag: docker image tag version (e.g., 'v0.31.0', 'v0.31.0-d56bb4d417-dirty').\n" |
There was a problem hiding this comment.
Imagetags don't have -rc suffix, but pep ones does. Shouldn't it be consistent?
There was a problem hiding this comment.
This PR was previously trying to be consistent with what we had before, but I agree the rc suffix is probably good to have for the image tag; will adjust.
| tag_regex = "^interuss/monitoring/v(?P<version>\\d+\\.\\d+\\.\\d+)" | ||
|
|
||
| # Strictly anchor to the repo-level prefix, 3-part numeric PEP440/SemVer, and optionally PEP440 lowercase '-rcN' suffixes. | ||
| tag_regex = "^interuss/monitoring/v(?P<version>\\d+\\.\\d+\\.\\d+(?:-rc\\d+)?)$" |
There was a problem hiding this comment.
setuptools_scm don't have the same behavior as the rest. Should just the script be re-used?
(if I uv build I get different results, on my fork it thinks its release 0.28.0, where was get_version.py use the remote and use different tags)
There was a problem hiding this comment.
I've now completely removed setuptool_scm; its pretend versions were very confusing to me and I think the new approach is clearer (and reuses get_version.py).
| ## Background | ||
|
|
||
| Releases of monitoring are based on git tags in the format `interuss/monitoring/v[0-9]+\.[0-9]+\.[0-9]+`, optionally suffixed with `-[0-9A-Za-z-.]+`. This tag form follows the pattern `[owner]/[component]/[semantic version]`; see [semantic version](https://semver.org) for more information. | ||
| Releases of `monitoring` utilize Git tags structured precisely as `interuss/monitoring/v[X].[Y].[Z]`, optionally accompanied by strict pre-release/candidate identifiers adhering to **PEP 440 Pre-Release Conventions**. |
There was a problem hiding this comment.
Are they? There are '-' in some situation described bellow, witch don't strictly follow PEP440 ? (They are allowed, but that not the normalized form')
There was a problem hiding this comment.
I've clarified.
| * **Release Candidates (`vX.Y.Z-rc[N]`)**: **Strict Pre-Release Identifiers**. | ||
| * Pre-release tags **must** utilize the strictly lowercase, hyphenated `-rc[N]` suffix (e.g., `v0.31.0-rc1`). | ||
| * Build tooling normalizes these delimiters into PEP 440-compliant pre-release strings (`X.Y.Zrc[N]`) for PyPI compatibility. | ||
| * Note: Pre-releases containing uppercase identifiers (e.g., `-RC1`), space delimiters, alphabetic metadata, or non-numeric suffixes (e.g., `-alpha`, `-1.2`) are prevented to avoid accidental malformed PyPI publication. |
There was a problem hiding this comment.
Nit, but that not very correct: -RC1 is a valid PEP440 format, -alpha as well. Only -1.2 is invalid, PyPI will accept (and normalize) the rest.
We probably want to restrict tags to a common format, but that the same for the docker image, and that not a PEP/PyPI requirement?
There was a problem hiding this comment.
-RC1 is not in PEP440 canonical format to be used in public version identifiers, nor is -alpha.
| * **Interim Import Namespace (Phase 1 Phase-in)**: | ||
| * Consumers install the package via `pip install interuss_monitoring`. | ||
| * In the current structural phase (Phase 1), internal code modules and external users importing from the PyPI package interact with the Python Import Namespace via **`import monitoring.<submodule>`**. | ||
| * *Warning for Interim Consumers*: Users must ensure their active Python virtual environment does not contain a conflicting top-level `monitoring/` directory from alternative third-party packages to prevent Python import-shadowing and runtime `ModuleNotFoundError` conflicts. |
There was a problem hiding this comment.
nit: the whole style of the doc is a bit off (things like "Interim Consumers")
There was a problem hiding this comment.
"Interim Consumers" seems fairly clear to me: consumers of the package while it still using monitoring for imports rather than interuss_monitoring. But, I'll see if I can adjust any way.
| # If this is not an exact match on the release tag, or if workspace is dirty, append local version | ||
| metadata_segments = [] | ||
| if not is_exact_tag_boundary: | ||
| metadata_segments.append(f"g{commit_hash.lstrip('g')}") |
There was a problem hiding this comment.
What the 'g' ? It cannot be present in a hash ?
There was a problem hiding this comment.
Without --abbrev=0, describe returns something like interuss/monitoring/v1.2.0-4-g2a3b4c5. But the versioning logic is apparently very difficult even when factored out like this, so I'll work to make it as crystal clear as possible.
…and fix docker tag delimiters
|
This PR now has substantial changes after the previous round of comments. |
2 participants
This PR primarily fixes the PyPI publishing configuration to use the
interuss_monitoringpackage name rather than the unavailablemonitoringpackage name, and to ensure the version used for publishing is valid (follows PEP440). To do this consistently, the PR also coalesces multiple disparate versioning scripts into a single get_versions.py. While doing this, I found that tag.sh is only used in an obsolete release workflow so I did not coalesce tag.sh and instead removed it, correcting the release procedure documentation to match current practices. The complexity of build.sh increases somewhat with these changes, so to avoid duplication, build_dev.sh functionality is merged into build.sh and build_dev.sh removed.