Skip to content
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

ci(git-artifacts): backport a couple of patches from git-sdk-64 #33

Merged
merged 7 commits into from
Feb 6, 2025

Conversation

dscho
Copy link
Member

@dscho dscho commented Feb 5, 2025

The ci-artifacts workflow is rarely exercised in git-sdk-arm64 (because of that eternally postponed general availability of the Windows/ARM64 hosted runners). As such, it is understandable that there has been a bit of bitrot in the meantime.

Note: I skipped 444678c because we will never build mingw-w64-i686-git using Windows/ARM64. Ever.

dscho added 5 commits February 5, 2025 13:41
There is no need to upload installer logs when the installer was not run
;-)

Signed-off-by: Johannes Schindelin <[email protected]>
This helps debugging a lot.

Signed-off-by: Johannes Schindelin <[email protected]>
When running a PowerShell script, the exit code will be determined
by the last command that was run.

Unfortunately, in the PR build, we run a couple of commands _after_
running the installer, and since we do not check for the exit code
of the installer process, we miss failures.

This is particularly bad because we missed the fact that we failed to
run the 32-bit installer tests ever since we deprecated installing the
32-bit variant on a 64-bit system (such as the GitHub Actions build
agent where the PR build runs...) in c800a3f60 (installer: loudly warn
when installing a 32-bit Git for Windows, 2023-02-21).

Let's check explictly if the installer failed.

Note: We cannot simply use `$?` but need to look at the `ExitCode`
attribute of the process returned by `Start-Process`, _and_ we need to
pass `-PassThru` for this to work, due to a bug in `Start-Process`. See
https://stackoverflow.com/a/16018287/1860823 for a more complete
explanation.

Signed-off-by: Johannes Schindelin <[email protected]>
In addition to the exit code, let's make extra certain that the
installation succeeded.

Signed-off-by: Johannes Schindelin <[email protected]>
We really want no failures to be hidden, whether fatal or non-fatal.
Regular installations should always succeed without any failures.

Signed-off-by: Johannes Schindelin <[email protected]>
@dscho dscho requested review from mjcheetham and rimrul February 5, 2025 12:47
@dscho dscho self-assigned this Feb 5, 2025
@dscho dscho marked this pull request as draft February 5, 2025 12:52
dscho added 2 commits February 5, 2025 15:04
In the new world, we cannot discern CPU architectures via BITNESS at
all. Let's just align with how things are done in the `git-artifacts`
GitHub workflow in git-for-windows/git-for-windows-automation.

Signed-off-by: Johannes Schindelin <[email protected]>
The 'Generate bundle artifacts' step implicitly calls
`add-release-note.js`, which required node.js.

However, on our self-hosted Windows/ARM64 runners (the general
availability of GitHub's hosted runners cannot come soon enough!), we do
not install node.js. So let's use a dedicated GitHub Action to ensure
that it's there when it is needed most.

Signed-off-by: Johannes Schindelin <[email protected]>
@dscho
Copy link
Member Author

dscho commented Feb 5, 2025

Had to add 7954d43 and d5f2500 to actually fix the build (see https://github.com/git-for-windows/git-sdk-arm64/actions/runs/13159140512, which only fails because it could not overwrite Git for Windows' uninstaller for one reason or another).

@dscho dscho marked this pull request as ready for review February 5, 2025 15:23
@dscho dscho requested a review from dennisameling February 5, 2025 18:41
Copy link
Collaborator

@dennisameling dennisameling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! 🙏🏼

@dscho dscho merged commit 6a4f71e into main Feb 6, 2025
1 check passed
@dscho dscho deleted the git-artifacts-backports branch February 6, 2025 08:27
@dennisameling
Copy link
Collaborator

dennisameling commented Feb 6, 2025

which only fails because it could not overwrite Git for Windows' uninstaller for one reason or another).

I just tested the artifact locally, and the installer script finished successfully. Let's assume the error in CI was either temporary or related to the self-hosted runner. IMO, let's not spend more effort on this now and have our fingers crossed that the GitHub-hosted runners will become available in the next couple weeks...

@dscho
Copy link
Member Author

dscho commented Feb 6, 2025

which only fails because it could not overwrite Git for Windows' uninstaller for one reason or another).

I just tested the artifact locally, and the installer script finished successfully. Let's assume the error in CI was either temporary or related to the self-hosted runner.

Thank you for testing, and I agree!

IMO, let's not spend more effort on this now and have our fingers crossed that the GitHub-hosted runners will become available in the next couple weeks...

In the interest of our physical health, let's not hold our breath for it, and let's expect history to repeat itself in that the date will slip again.

@rimrul
Copy link
Member

rimrul commented Feb 6, 2025

let's expect history to repeat itself in that the date will slip again.

It has been pushed back again, to Q2 2025

@dscho
Copy link
Member Author

dscho commented Feb 6, 2025

let's expect history to repeat itself in that the date will slip again.

It has been pushed back again, to Q2 2025

giphy-2098352906

.
.
.
.

just-kidding-498-x-280-gif-35ow9q00f9ov2iff-1287746246

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants