GH-49686: [C++][FlightRPC][ODBC][Release] Create signing script for Windows FlightSQL ODBC build#49788
GH-49686: [C++][FlightRPC][ODBC][Release] Create signing script for Windows FlightSQL ODBC build#49788amoeba wants to merge 15 commits intoapache:mainfrom
Conversation
amoeba
requested review from
assignUser,
jonkeane,
kou and
raulcd
as code owners
| if [ -z "${ESIGNER_STOREPASS:-}" ]; then | ||
| echo "ERROR: ESIGNER_STOREPASS is not set" >&2 | ||
| exit 1 | ||
| fi | ||
| if [ -z "${ESIGNER_KEYPASS:-}" ]; then | ||
| echo "ERROR: ESIGNER_KEYPASS is not set" >&2 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
How about setting them in dev/release/.env?
| } | ||
|
|
||
| # All work with release artifacts happens in a temp dir | ||
| tmp_dir="$(mktemp -d)" |
There was a problem hiding this comment.
How about using constant temporary directory (such as dev/release/tmp/) instead of mktemp -d?
If we use mktemp -d, we need to remove the temporary directory manually.
| echo "[4/9] Removing unsigned DLL from GitHub Release..." | ||
| gh release delete-asset "${tag}" \ | ||
| --repo "${GITHUB_REPOSITORY}" \ | ||
| --yes \ | ||
| "${dll_unsigned}" |
There was a problem hiding this comment.
Why do we need to remove unsigned DLL?
There was a problem hiding this comment.
Just to reduce confusion and artifact usage, we don't need to. I don't know if I have a principled reason either way. If we did jsign signing entirely in CI, we'd probably build the unsigned DLL, sign it, and only upload the signed version. Happy to change this though.
| run_url=$(gh workflow run cpp_extra.yml \ | ||
| --repo "${GITHUB_REPOSITORY}" \ | ||
| --ref "${tag}" \ | ||
| --field odbc_release_step=true 2>&1 | grep -oE 'https://[^ ]+') | ||
| run_id=${run_url##*/} # Extract the run ID from the URL (the part after the last slash) |
There was a problem hiding this comment.
How about using --json and --jq like we did in
arrow/dev/release/utils-watch-gh-workflow.sh
Lines 37 to 43 in 6dd07b1
There was a problem hiding this comment.
workflow run doesn't have a --json output arg and the command just prints,
[4/9] Triggering odbc_release_step in cpp_extra.yml workflow...
✓ Created workflow_dispatch event for cpp_extra.yml at AMOEBA-arrow-99.9.9-rc0
https://github.com/amoeba/arrow/actions/runs/24583481107
To see the created workflow run, try: gh run view 24583481107
To see runs for this workflow, try: gh run list --workflow="cpp_extra.yml"
Doing it this way isn't ideal but it seems slightly better than the alternative which would be to use gh run list with the right filters and assume the first result is the correct run. I guess that's a pretty safe assumption.
Do you have a preference?
| echo "Triggered run: ${run_url}" | ||
|
|
||
| echo "[6/9] Waiting for workflow to complete..." | ||
| gh run watch "${run_id}" --repo "${GITHUB_REPOSITORY}" --exit-status |
There was a problem hiding this comment.
If this may take long time, we may want to add --interval XXX.
There was a problem hiding this comment.
Could you remove 07-publish-gh-release.sh?
There was a problem hiding this comment.
Ah, my mistake there. I still had the final commit in my git stash.
There was a problem hiding this comment.
Could you remove 08-binary-verify.sh?
There was a problem hiding this comment.
Pull request overview
Adds release automation scripts under dev/release/ to support Windows FlightSQL ODBC artifact signing and GitHub Release/RC verification steps as part of the Arrow release process.
Changes:
- Add a new
07-flightsqlodbc-upload.shscript intended to download, sign, and re-upload the FlightSQL ODBC DLL/MSI to the RC GitHub Release. - Add
08-publish-gh-release.shto publish (undraft) an RC GitHub Release. - Add
09-binary-verify.shto rerun theverify_rc.ymlworkflow for an RC tag.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| dev/release/07-flightsqlodbc-upload.sh | New local signing + upload workflow for FlightSQL ODBC Windows DLL/MSI artifacts. |
| dev/release/08-publish-gh-release.sh | New script to mark the RC GitHub Release as non-draft. |
| dev/release/09-binary-verify.sh | New script to rerun the GitHub Actions RC verification workflow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Utility function to use jsign to check if a file is signed or not | ||
| is_signed() { | ||
| local file="$1" | ||
| local output | ||
| local exit_code | ||
| output=$(jsign extract --format PEM "${file}" 2>&1) | ||
| exit_code=$? | ||
| # jsign writes a PEM file even though it also prints to stdout. Clean up after | ||
| # it. Use -f since so it still runs on unsigned files without error. | ||
| rm -f "${file}.sig.pem" | ||
|
|
||
| return ${exit_code} | ||
| } |
There was a problem hiding this comment.
is_signed() uses output=$(jsign extract ...) while set -e is enabled. If the file is unsigned, jsign extract will return non-zero and the whole script will exit before exit_code=$? / cleanup runs, so the “unsigned vs signed” checks can’t work. Consider temporarily disabling -e inside the function (or using an if jsign extract ...; then ... pattern) and always running the cleanup.
There was a problem hiding this comment.
This is a good suggestion. I changed the call to use || to avoid this issue: 33866c7.
| echo "[2/9] Signing ${dll_signed}..." | ||
| echo "NOTE: Running jsign. You may be prompted for your OTP PIN..." | ||
| jsign --storetype ESIGNER \ | ||
| --alias d97c5110-c66a-4c0c-ac0c-1cd6af812ee6 \ | ||
| --storepass "${ESIGNER_STOREPASS}" \ | ||
| --keypass "${ESIGNER_KEYPASS}" \ | ||
| --tsaurl="http://ts.ssl.com" \ | ||
| --tsmode RFC3161 \ | ||
| --alg SHA256 \ | ||
| "${tmp_dir}/${dll_unsigned}" | ||
| if ! is_signed "${tmp_dir}/${dll_signed}"; then | ||
| echo "ERROR: ${dll_signed} is not signed" >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| echo "[3/9] Uploading signed DLL to GitHub Release..." | ||
| gh release upload "${tag}" \ | ||
| --repo "${GITHUB_REPOSITORY}" \ | ||
| --clobber \ | ||
| "${tmp_dir}/${dll_signed}" | ||
|
|
There was a problem hiding this comment.
The DLL signing phase signs ${dll_unsigned} in place, but then verifies/uploads ${dll_signed} (arrow_flight_sql_odbc.dll) without ever creating/renaming that file. As written, the verification will fail and the upload will reference a non-existent path. After signing, rename/copy the signed ${dll_unsigned} to ${dll_signed} (or update the subsequent checks/uploads to point at the actual signed file).
There was a problem hiding this comment.
I think this is a correct call-out. Fixed in 47ec038.
| run_url=$(gh workflow run cpp_extra.yml \ | ||
| --repo "${GITHUB_REPOSITORY}" \ | ||
| --ref "${tag}" \ | ||
| --field odbc_release_step=true 2>&1 | grep -oE 'https://[^ ]+') | ||
| run_id=${run_url##*/} # Extract the run ID from the URL (the part after the last slash) | ||
| if [ -z "${run_id}" ]; then | ||
| echo "ERROR: failed to get run ID from workflow trigger" >&2 | ||
| exit 1 | ||
| fi | ||
| echo "Triggered run: ${run_url}" |
There was a problem hiding this comment.
run_url=$(gh workflow run ... | grep ...) runs under set -e -o pipefail; if gh output format changes or grep finds no match, the assignment will cause an immediate exit and the later run_id validation won’t run. Prefer using gh workflow run ... --json (if available) or add a non-fatal fallback (|| true) and/or follow up by querying gh run list for the latest run on the tag.
| run_url=$(gh workflow run cpp_extra.yml \ | |
| --repo "${GITHUB_REPOSITORY}" \ | |
| --ref "${tag}" \ | |
| --field odbc_release_step=true 2>&1 | grep -oE 'https://[^ ]+') | |
| run_id=${run_url##*/} # Extract the run ID from the URL (the part after the last slash) | |
| if [ -z "${run_id}" ]; then | |
| echo "ERROR: failed to get run ID from workflow trigger" >&2 | |
| exit 1 | |
| fi | |
| echo "Triggered run: ${run_url}" | |
| trigger_output=$(gh workflow run cpp_extra.yml \ | |
| --repo "${GITHUB_REPOSITORY}" \ | |
| --ref "${tag}" \ | |
| --field odbc_release_step=true 2>&1) | |
| run_url=$(printf '%s\n' "${trigger_output}" | grep -oE 'https://[^ ]+' || true) | |
| run_id="" | |
| if [ -n "${run_url}" ]; then | |
| run_id=${run_url##*/} # Extract the run ID from the URL (the part after the last slash) | |
| fi | |
| if [ -z "${run_id}" ]; then | |
| run_id=$(gh run list \ | |
| --repo "${GITHUB_REPOSITORY}" \ | |
| --workflow cpp_extra.yml \ | |
| --branch "${tag}" \ | |
| --limit 1 \ | |
| --json databaseId \ | |
| --jq '.[0].databaseId' || true) | |
| fi | |
| if [ -z "${run_id}" ] || [ "${run_id}" = "null" ]; then | |
| echo "ERROR: failed to get run ID from workflow trigger" >&2 | |
| exit 1 | |
| fi | |
| if [ -n "${run_url}" ]; then | |
| echo "Triggered run: ${run_url}" | |
| else | |
| echo "Triggered run id: ${run_id}" | |
| fi |
There was a problem hiding this comment.
This seems overly cautious. The call-out about the grep erroring and failing the script is fair though so I added a || true so it continues and empty run_id gets caught below. 7f0e6e0
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
github-actions
bot
added
awaiting change review
|
The pre-commit failure in https://github.com/apache/arrow/actions/runs/24610839359/job/71964834631?pr=49788#step:5:192 looks like it's caused by an unrelated issue. The fs package recently made a breaking change and it now requires libuv-dev. I'm going to test a fix in ff033b6 but will revert it and put it into another PR. See #49594 for more context. |
This reverts commit ff033b6.
|
Thanks for the review @kou. I think I've addressed everything so this is ready for another review. |
…kage (#49791) ### Rationale for this change Pre-commit in CI just started to fail trying to install the R `fs` package. See https://github.com/apache/arrow/actions/runs/24610839359. I think we didn't see this until now because we were using cached pre-commit and a recent PR of mine just invalidated the cache incidentally. See also #49594. The `fs` package recently made a change that requires we install libuv development headers to install it. ### What changes are included in this PR? - Updates dev.yml to Install libuv1-dev ### Are these changes tested? Yes. See my other PR: #49788 (comment). ### Are there any user-facing changes? No. Authored-by: Bryce Mecum <petridish@gmail.com> Signed-off-by: Bryce Mecum <petridish@gmail.com>
Rationale for this change
We need a script for the release manager to run during the release to locally sign the Windows artifacts for the FlightSQL ODBC driver.
Ref: #49404
What changes are included in this PR?
07-flightsql-odbc-upload.shAre these changes tested?
Not 100% but I've tested each step separately. I tested on my fork using fake tags and releases.
Are there any user-facing changes?
No.