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: add ci-artifacts pipeline #22

Merged

Conversation

dennisameling
Copy link
Collaborator

This is step 3 of configuring ci-artifacts pipelines to upload artifacts to GitHub releases.

A similar pipeline already exists in git-sdk-64, so this commit is to port most of that code to git-sdk-arm64.

We don't (yet) copy over the test-minimal-sdk job, because it spins up 16 parallel jobs, which is probably a bit too much for our self-hosted runner budget. We can add those once GitHub-hosted arm64 runners become available for OSS projects by the end of 2024.

dennisameling added a commit to dennisameling-org/git-sdk-arm64 that referenced this pull request Sep 14, 2024
While working on adding a `ci-artifacts` pipeline to `git-sdk-arm64`, which contains a test for `clang.exe`, the command failed because of some missing DLLs.

This commit ensures the missing DLLs will be added to the `minimal-sdk` artifact.

Ref: git-for-windows#22
Signed-off-by: Dennis Ameling <[email protected]>
dennisameling added a commit to dennisameling-org/git-sdk-arm64 that referenced this pull request Sep 14, 2024
While working on adding a `ci-artifacts` pipeline to `git-sdk-arm64`, which contains a test for `clang.exe`, the command failed because of some missing DLLs.

This commit ensures the missing DLLs will be added to the `minimal-sdk` artifact.

Ref: git-for-windows#22
Signed-off-by: Dennis Ameling <[email protected]>
Copy link
Member

@dscho dscho left a comment

Choose a reason for hiding this comment

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

So you don't want to have those 20 test-minimal-sdk matrix jobs? 😁

Even so, it may make sense to at least clone git/git and build it, without compressing and uploading git-artifacts.tar.gz, just to make sure that it can be built.

Besides, if you leave out that big matrix job, what remains is a sequence of 3 jobs that have to be run in order, all on Windows/ARM64. That's a bit wasteful what with spinning up and tearing down VMs, it would probably make sense to merge them into a single job (and then the upload-artifact/download-artifact steps become unnecessary).

The rest of the diff relative to git-for-windows#87 version of the workflow looks reasonable to me. Thank you so much!

@dennisameling dennisameling force-pushed the add-ci-artifacts branch 2 times, most recently from 7af537e to 1873a1b Compare September 15, 2024 16:29
A similar pipeline already exists in git-sdk-64, so this commit is to port most of that code to git-sdk-arm64.

We don't (yet) copy over the test-minimal-sdk job, because it spins up 16 parallel jobs, which is a bit too much for our self-hosted runner budget. We can add those once GitHub-hosted arm64 runners become available for OSS projects by the end of 2024.

Ref: git-for-windows/git-for-windows-automation#91
Ref: https://github.blog/news-insights/product-news/arm64-on-github-actions-powering-faster-more-efficient-build-systems/#get-started-using-arm-hosted-runners-today
Signed-off-by: Dennis Ameling <[email protected]>
@dennisameling
Copy link
Collaborator Author

So you don't want to have those 20 test-minimal-sdk matrix jobs? 😁

I'd love to have those, but your Azure subscription wouldn't really like that. I keep running into a concurrent cores quota when running multiple things in parallel. I should have clarified that a bit more in the PR description. Happy to add those right away once the Hosted ARM64 Runners are in place, which also start within just a few seconds in my testing 😄

Besides, if you leave out that big matrix job, what remains is a sequence of 3 jobs that have to be run in order, all on Windows/ARM64. That's a bit wasteful what with spinning up and tearing down VMs, it would probably make sense to merge them into a single job (and then the upload-artifact/download-artifact steps become unnecessary).

The third one was ubuntu-latest actually, but agreed, let's merge them!

I just kicked off a pipeline in my fork, and everything's working as expected 🎉 Here's the release with the artifacts.

@dscho shall we kill my TO-DROP commit and get this merged? 😊

@dennisameling dennisameling marked this pull request as ready for review September 15, 2024 16:54
@dscho
Copy link
Member

dscho commented Sep 15, 2024

So you don't want to have those 20 test-minimal-sdk matrix jobs? 😁

I'd love to have those, but your Azure subscription wouldn't really like that.

😀 Thank you for being considerate!

I keep running into a concurrent cores quota when running multiple things in parallel. I should have clarified that a bit more in the PR description. Happy to add those right away once the Hosted ARM64 Runners are in place, which also start within just a few seconds in my testing 😄

Yes, I bet! All the more sad that hosted Windows/ARM64 runners are still not free for OSS. I could imagine that this is part of the reason why the ecosystem is so slow to support Windows/ARM64. It sure holds me back.

I just kicked off a pipeline in my fork, and everything's working as expected 🎉 Here's the release with the artifacts.

@dscho shall we kill my TO-DROP commit and get this merged? 😊

Awesome, let's do that!

@dennisameling dennisameling merged commit 87c2433 into git-for-windows:main Sep 15, 2024
1 check passed
@dennisameling dennisameling deleted the add-ci-artifacts branch September 15, 2024 17:10
@dscho
Copy link
Member

dscho commented Sep 15, 2024

@dennisameling oy vey, I forgot that git-sdk-arm64 is a fork of git-sdk-64!

@dscho
Copy link
Member

dscho commented Sep 15, 2024

See https://github.com/git-for-windows/git-sdk-arm64:

image

So maybe we want to imitate this:

    if: github.repository_owner == 'git-for-windows'

@dennisameling what do you think?

@dennisameling
Copy link
Collaborator Author

So maybe we want to imitate this:

    if: github.repository_owner == 'git-for-windows'

@dennisameling what do you think?

Sounds good, was going to suggest something similar. Do you want to push a commit to main, or want me to do it?

@dscho
Copy link
Member

dscho commented Sep 15, 2024

@dennisameling please go ahead, I am busy with some Git for Windows patches in preparation for the upcoming release.

@dennisameling
Copy link
Collaborator Author

Looks like that worked! The self-hosted runner is still being created; I'll keep an eye on it.

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.

2 participants