GH-49537: [C++][FlightRPC] Windows CI to Support ODBC DLL & MSI Signing#49603
Conversation
|
I did an empty commit (a522393) and got this error: The same implementation worked yesterday (see https://github.com/apache/arrow/actions/runs/23622018872/job/68803175375). Seems that GitHub might have updated runner, I will look into this. |
This issue should be resolved now by commit 95bc75b |
alinaliBQ
left a comment
There was a problem hiding this comment.
Addressed comments. Please have another look, thanks!
amoeba
left a comment
There was a problem hiding this comment.
Thanks @alinaliBQ.
@raulcd do you want to have a look? Once we merge, I'll test the whole flow with the script (PR to come) that does the signing so we'll have more chances to fix any issues.
raulcd
left a comment
There was a problem hiding this comment.
Thanks for the work @alinaliBQ ! I am unsure this is ready yet, sorry it took me a couple days for review, it was Easter and had some family around. My main concern is around, the tag hasn't been updated to match the one on the packaging linux jobs, currently any tag will trigger the job and we only want to do it for RC's. The worlflow dispatch is still around and I thought this was not necessary with the tag.
@amoeba I am about to do the feature freeze, I think this is going to miss 24.0.0, is that ok?
| schedule: | ||
| - cron: | | ||
| 0 0 * * * | ||
| workflow_dispatch: |
There was a problem hiding this comment.
I am not sure I understand this. Didn't we agreed to use the tag for the RC? Why is the workflow dispatch needed and referenced below?
There was a problem hiding this comment.
The flow is a bit complicated, we need to trigger some jobs when the RC is created but the odbc-msvc-upload-msi step needs to be triggered manually so we use workflow_dispatch. The sequence is,
- Release manager tags release
- cpp_extra.yml jobs
odbc-msvc-upload-dllandodbc-dll-releaseare run automatically, unsigned DLL is added to release - Release manager locally runs script (TBD) that downloads, signs (w/ jsign), and uploads the signed DLL to the release
- Release manager triggers
odbc-msvc-upload-msiviaworkflow_dispatchwhich builds an unsigned MSI with a signed DLL inside and adds it to the release - Release manager locally runs script that downloads signs (w/ jsign), and uploads the signed MSI to the releasee
There was a problem hiding this comment.
I see, I did not understood the process. As the process isn't entirely automated and it requires several manual steps, can we document it on the release page?
https://github.com/apache/arrow/blob/main/docs/source/developers/release.rst
There was a problem hiding this comment.
@amoeba Could you kindly help with the documentation on release page?
There was a problem hiding this comment.
Absolutely. I'll file an issue and fill it in as we finish the final PRs.
There was a problem hiding this comment.
hi @raulcd, @amoeba has helped to document the process on release page:
arrow/docs/source/developers/release.rst
Lines 267 to 276 in 62cdda9
|
Thanks for taking a look @raulcd. It's okay if this doesn't make it for 24 but if it's at all possible that'd be great. Re: the jobs running on any tag, I'm not seeing what you see, could you point out which job needs updating?
I could see making the last one ( |
No worries @raulcd, hope you had a good Easter. Since having a |
5106e27 to
ad511e0
Compare
| odbc-msvc-upload-msi: | ||
| name: ODBC Windows Build & Upload Unsigned MSI | ||
| runs-on: windows-2022 | ||
| if: inputs.odbc_release_step |
There was a problem hiding this comment.
We may be able to remove all inputs for workflow_dispatch and just check whether this workflow is triggered by workflow_dispatch or not.
There was a problem hiding this comment.
We can raise an issue for this.
@justing-bq Please raise one GitHub issue for:
- removing
inputsforworkflow_dispatchinpackage_odbc.yml. - Then
if: github.event_name == 'workflow_dispatch'can be used. - The
dev/release/07-flightsqlodbc-upload.shand release documentation needs to be updated as well.
cc @amoeba
alinaliBQ
left a comment
There was a problem hiding this comment.
Thanks folks for the reviews. @justing-bq please have a look when you get a chance
| odbc-msvc-upload-msi: | ||
| name: ODBC Windows Build & Upload Unsigned MSI | ||
| runs-on: windows-2022 | ||
| if: inputs.odbc_release_step |
There was a problem hiding this comment.
We can raise an issue for this.
@justing-bq Please raise one GitHub issue for:
- removing
inputsforworkflow_dispatchinpackage_odbc.yml. - Then
if: github.event_name == 'workflow_dispatch'can be used. - The
dev/release/07-flightsqlodbc-upload.shand release documentation needs to be updated as well.
cc @amoeba
ad511e0 to
8b552fe
Compare
8b552fe to
b4d7f51
Compare
|
Hi @justing-bq, I reviewed the recent PR feedback and I think every comment has either been addressed or had an issue created for it. Is that right? Can you fix the merge conflicts and then comment that the PR is ready for a final review? |
b4d7f51 to
6ba6283
Compare
Hi @amoeba. Yes that is correct. I fixed the merge conflict and this PR is ready for a final review. |
* Add draft code for CI A and CI B Attempt workflow dispatch Only ODBC Windows original workflow should run. Later need to add `github.event_name != 'workflow_dispatch' ||` to all existing workflows after uncomment Use `GITHUB_REF_NAME` directly via push Add `workflow_dispatch` definitions Add `ODBC Windows Upload DLL` Use common ODBC Windows environment variables Use ODBC as composite action Create cpp_odbc.yml Initial draft temp disable test step Temp disable non-ODBC Windows workflows * Clean Up Code * Remove comments * Fix Installer path for MSI Trigger CI Fix issue with `secrets.GITHUB_TOKEN` Change `odbc_msi_upload` to boolean input Change `odbc-msvc-upload-dll` to be triggered via rc tag and can be invoked manually Work on Bryce's code review comments ^ Conflicts: ^ .github/workflows/cpp_extra.yml
6ba6283 to
f491475
Compare
|
Merged. Thanks for the work on this @alinaliBQ and @justing-bq and thanks for the reviews @raulcd and @kou. |
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 9265a71. There weren't enough matching historic benchmark results to make a call on whether there were regressions. The full Conbench report has more details. |
Rationale for this change
GH-49537
What changes are included in this PR?
odbc-msvc-upload-dllUpload unsigned DLLodbc-msvc-upload-msiDownload signed DLL and upload unsigned MSIodbc-releaseCI that is replaced by the new CIs.Example of
07-flightsqlodbc-upload.shscript (not tested):We need to either 1) implement a way to get
RUN_IDand then callgh run watch,or 2) enter each command manually and wait for the CI to finish.
Documentation is in:
arrow/docs/source/developers/release.rst
Lines 267 to 276 in 62cdda9
Are these changes tested?
Are there any user-facing changes?
N/A