Deploy: take one tag when HEAD has several, and quote the versions - #2430
Deploy: take one tag when HEAD has several, and quote the versions#2430tempoloss wants to merge 4 commits into
Conversation
head -n 1 over -v:refname picks the wrong tag in exactly the case this PR is about: git ranks v1.2.3-rc1 above v1.2.3, so a commit carrying a release and its release candidate would deploy the candidate. One --add per suffix is deliberate. Naming only -rc leaves -beta and -alpha still sorting above the release, which is worth stating because the single-suffix form looks sufficient and is not.
The MSVC leg fails to link against the CLAPACK that vcpkg provides, and because deploy needs the whole build matrix, that one leg took the four platforms that do build down with it. Nothing was published for DuckDB 1.5.4 on any platform. Requested by JAicewizard in #2430 while the MSVC LAPACK link is investigated.
|
@samansmink addressing you rather than the descriptor reviewers, since this touches the deploy path. Two things changed since I opened it, and both make it smaller than it looks. It is no longer latent. The filter is Nothing here changes a single-tag commit. Taking the tags that sit on the commit each of the 319 descriptors pins, there are 57 distinct tag sets, and this PR returns what the current code returns for 56 of them; the 57th is the two-tag one. Run on ubuntu-24.04 under the same shell flags, against a real If you would rather keep the change to one line, I will drop everything except the quoting and the single-tag selection. |
|
Hi @tempoloss, I'll be taking a look today together with @lghavami. |
|
Hi @sebastiaan-dev ! How's it going? |
|
Hey @temoploss, thanks for the contribution (+ patience)! I added a few small comments, but otherwise it should be good to go. |
| # awk and not grep: grep exits 1 when nothing matches, and under this | ||
| # step's `-e -o pipefail` that status would abort the deploy for every | ||
| # commit whose tags are not plain releases. It survives today only | ||
| # because the line begins with `export`, which swallows the status. |
There was a problem hiding this comment.
Hi, you're configuring the ordering of -rc, -beta, and -alpha, however the awk filter on line 154 strips out anything with a suffix before sorting. Therefore either the three git config calls should be deleted, or the awk filter changed
There was a problem hiding this comment.
Though I see that the fallback does go through the sorted list of tags, so it actually shouldn't be an issue, but could you confirm the intention? Thanks!
| run: | | ||
| pwd | ||
| python3 -m pip install awscli | ||
| # A release and its variants can sit on one commit, and version sort |
There was a problem hiding this comment.
Appreciate the big comment, but for code cleanliness its nice to have a more concise explanation of what is being done
3 participants
git tag --points-at HEADreturns every tag on the commit, one per line. Unquoted, that expands to multiple words and shifts every argument after it, sodeploy.shreceives the wrong arch and the wrong bucket rather than failing loudly.It fires today
When this PR was opened I could not find a commit carrying two tags and called it latent. That was a sampling problem, not the state of the registry.
duckdb_mcp's current descriptorref(7db1650a,teaguesterling/duckdb_mcp) carries bothv2.1.2andv2.2.0.EXT_VERSIONis computed in the extension's own checkout - theDeployjob checks outinputs.repositoryatinputs.refinto the workspace root, then runsgit tag --points-at HEADthere - so the next rebuild of that extension runs the unquoted expansion. The deploy step's shell as written, on that tag pair:Eight arguments instead of seven, and
BUCKET_NAMElands in the slot the arch belongs to. The tag this PR picks,v2.2.0, is the version the descriptor pins.Scanning every repo in the registry - 319 extensions, tags resolved through the API so annotated tags report the commit they point at - finds ten commits in this state:
Nine of the ten are not currently pinned, so they are one descriptor bump away rather than firing. The first one is pinned now.
Four changes
Quote the expansions. This is the fix for the shift.
"$EXT_VERSION","$DUCKDB_VERSION"and"$BUCKET_NAME"keep one value in one argument. Quoting alone is not enough: it turns a shifted argument list into a version string with a newline in the middle of it, which becomes the S3 key.--sort=-v:refname | head -n 1. Takes a single tag, and version-sorts rather than sorts lexically, sov1.5.10beatsv1.5.9instead of losing to it.Prefer a plain release tag. Version sort ranks anything suffixed above the release it belongs to, so
head -n 1would takev1.2.3-rc1overv1.2.3. Naming the pre-release suffixes fixes the cases somebody thought to name, and cannot be complete:anofox_forecastcarriesv0.2.3besidev0.2.3-cpp, and-cppis not a pre-release anybody would have listed. Matching^v?[0-9]+([.][0-9]+)*$first takes the release whatever the variant is called, and falls through to the sorted list when the commit carries no plain tag at all.The filter is
awkand notgrepon purpose. This step declaresshell: bash, which the runner invokes with-e -o pipefail, andgrepexits 1 when nothing matches. Today 220 of the 319 pinned refs carry no tag at all and one more carriesquackfix-0.0.3, so for 221 extensions the filter matches nothing on every deploy. Withgrepthat is survivable only because the line begins withexport, which swallows the status - as a bare assignment the same pipeline aborts the step:awkexits 0 whether or not it matched, so the filter cannot fail a deploy no matter how the line is later rewritten.versionsort.suffix. Still there, because it is what decides betweenv1.0.0-rc1andv1.0.0-rc2when there is no plain tag to prefer.Every pair above, plus the two cases that must not change, through the three lines as they now stand:
Both call sites are changed,
EXT_VERSIONand theDUCKDB_VERSIONbranch that readsduckdb/duckdb.duckdb/duckdbhas 64 tags on 64 distinct commits, so no commit there carries two and that half stays precautionary.Checked the other way as well: taking the tags that sit on the commit each of the 319 descriptors pins, there are 57 distinct tag sets, and the rule in this PR returns exactly what
--sort=-v:refname | head -n 1returns for all 57. The only row where anything differs frommainis the two-tag one. 220 of those refs carry no tag at all and fall through to the commit hash, which is unchanged.Honestly
${VAR:=...}also became${VAR:-...}; both work where the result is being assigned anyway, but substitution is what is meant here.A commit that carries no tag at all still deploys a short commit hash as its version. That is pre-existing and this PR does not change it, though it is visible in the registry right now: the published
duckdb_mcpbinary carriesfaf20bdin itsextension_versionmetadata field. I have not traced which deploy produced that, so I am not claiming this bug caused it.Workflow-only changes generate no checks in this repo, so none of this ran here. It did run on an identical runner: https://github.com/tempoloss/quackiso/actions/runs/32509299741 fetches these three lines from this branch by URL rather than retyping them, then executes them on
ubuntu-24.04undershell: bash, which reportspipefail onand-ein force, with GNU Awk 5.2.1. It asserts the expected tag for all fifteen shapes above, clonesteaguesterling/duckdb_mcpat7db1650a1b4c04914ae724dd43244e690094ffc0and prints whatdeploy.shreceives from the real commit:What that run cannot cover is the deploy itself, which needs this repository's secrets.