[Test PR] Extract "Upgrade Git for Windows" to a composite action #68
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Although this is -- currently, and in the immediately foreseeable future -- only used in one place, it's cumbersome and also something that we've removed and reintroduced before. Making it a composite action may make it reasonable to keep it around for next time it is needed. It had previously been removed based on the idea that it wasn't clear what form it would need to be in if needed again. Now that it is needed again, the current form as well as the form that seems most likely for future needs is to install the latest stable release of Git for Windows -- rather than a release candidate, hard coded version, etc. Note that this should still not be used habitually, as the Git for Windows installation that ships preinstalled on a runner image should be preferred except when insufficient. See GitoxideLabs#1870, GitoxideLabs#1892, GitoxideLabs#1920, and the recent commit that reintroduced this logic, for background. This may be a bit complicated to get working, because we want to run the composite action before we have used `actions/checkout`. This is so `actions/checkout` can use and therefore verify that the new Git for Windows version works, and so we don't leave any dangling subprocesses or otherwise open files that would keep the new Git for Windows's Inno Setup installer from removing or replacing the old Git for Windows. But to run a composite action from a`./` path, one ordinarily needs to have checked out the repository already. A composite action can of course be run from a specified ref on a specified repository. But unless we're actually spinning it out to be a general reusable action that versions separately from workflows here such as `ci.yml`, we would always want to run the composite action as defined at the same commit we are at. (The effect of modifying it on a feature branch is othewise cumbersome and unintuitive.) This same commit is often pointed to by the same remote ref -- but not always, because the `pull_request` trigger runs workflows on a temporary merge commit.
This may not work, because the `github` context may not be available in `uses`.
Though I don't expect that to work, since I think the `github` context is just not available in `uses`. Also, if it does work, I'm not sure how robust it would be. In common scenarios, a commit is available by its OID througout the fork network. However, when this is a merge commit for an unmerged PR (which is what `github.sha` is in workflow runs triggered by the `pull_request` event), I am uncertain if it is actually guaranteed to be available when accessed on repositories in the fork network *other* than the repository from which the PR originates and the repository the PR is opened on.
For the call to the extracted `upgrade-git-for-windows` composite action. Though even if this works, the conceptual heft (complexity of what we are doing and why, even though the code is simple), together with making it harder for automated tools to know it's a composite action in our that is being indirectly used, may make this approach unworthwhile compared to just keeping the steps inlined in `ci.yml`.
EliahKagan
added a commit
that referenced
this pull request
Jul 14, 2025
This is to test (in #68) that changes to the composite action, as it is used in `ci.yml`, are immediately reflected. This commit shall be reverted after testing.
This is to test (in #68) that changes to the composite action, as it is used in `ci.yml`, are immediately reflected. This commit shall be reverted after testing.
EliahKagan
force-pushed
the
arm-windows-next
branch
from
17d81e9 to
2e4fa30
Compare
Instead of the test notice.
EliahKagan
added a commit
that referenced
this pull request
Jul 19, 2025
The "Install rustup" step was duplicated across two jobs: `test-fast` and `test-fixtures-windows`, both conditioned on `windows-11-arm`. There is furthermore the possibility that it may be added in the future to other jobs (such as if it ends up being useful to run the MSRV check on `windows-11-arm`). The duplication was such that it would be easy to wrongly change one but not the other occurrence. If that were to happen, and tests passed/failed or otherwise behaved differently due to differences in their CI environments, then it would create the wrong impression of being related to whether `GIX_TEST_IGNORE_ARCHIVES` is set. To decrease unnecessary duplication, and more specifically to avoid that specific confusion and the wild goose chase that might ensue from it, this extracts the logic of the "Install rustup" steps, other than the `if` key checking that we are running on `windows-11-arm`, to a newly created composite action `setup-windows-arm-rustup`. The reason for keeping the operating system check in the calling jobs is so that it is always immediately clear in the job output whether the step is meant to have an effect. Although composite actions often make sense to publish separately and use across repositories, that is probably not the case here. This logic does only what we need right now, and does not include functionality such as respecting a preexisitng `CARGO_HOME` environment variable or making sure to download `rustup-init.exe` to a location that would be reasonable for all callers. (This does *not* also extract the "Upgrade Git for Windows" step to a composite action. It's not necessary to do so, because the step only currently appears in one job: the `test-fixtures-windows` job, conditioned on `windows-11-arm`. It would, in principle, be useful to extract it nonetheless, because it is cumbersome and also something we've removed and reintroduced before; so making it a composite action could make it more reasonable to keep around for next time it is needed. The problem is that there is a benefit to upgrading Git for Windows before, rather than after, performing any nontrivial Git operations, to decrease the likelihood of dangling subprocesses or otherwise open files preventing the Inno Setup installer from performing the upgrade. There is also a separate specific benefit to upgrading it before `actions/checkout` runs, so that step uses it, thereby effectively validating that it really works. But extracting the "Upgrade Git for Windows" step to a composite action that runs before the `actions/checkout` step is complicated, because the otherwise ideal way to refer to the action locally with a `./` path couldn't be used, because its code would not yet have been checked out. There are ways around this, but their complexity suggests they may not be worthwhile until a composite action is needed in order to actually decrease logic duplication. See the experiments in #68 for details.)
EliahKagan
added a commit
that referenced
this pull request
Jul 19, 2025
The "Install rustup" step was duplicated across two jobs: `test-fast` and `test-fixtures-windows`, both conditioned on `windows-11-arm`. There is furthermore the possibility that it may be added in the future to other jobs (such as if it ends up being useful to run the MSRV check on `windows-11-arm`). The duplication was such that it would be easy to wrongly change one but not the other occurrence. If that were to happen, and tests passed/failed or otherwise behaved differently due to differences in their CI environments, then it would create the wrong impression of being related to whether `GIX_TEST_IGNORE_ARCHIVES` is set. To decrease unnecessary duplication, and more specifically to avoid that specific confusion and the wild goose chase that might ensue from it, this extracts the logic of the "Install rustup" steps, other than the `if` key checking that we are running on `windows-11-arm`, to a newly created composite action `setup-windows-arm-rustup`. The reason for keeping the operating system check in the calling jobs is so that it is always immediately clear in the job output whether the step is meant to have an effect. Although composite actions often make sense to publish separately and use across repositories, that is probably not the case here. This logic does only what we need right now, and does not include functionality such as respecting a preexisitng `CARGO_HOME` environment variable or making sure to download `rustup-init.exe` to a location that would be reasonable for all callers. (This does *not* also extract the "Upgrade Git for Windows" step to a composite action. It's not necessary to do so, because the step only currently appears in one job: the `test-fixtures-windows` job, conditioned on `windows-11-arm`. It would, in principle, be useful to extract it nonetheless, because it is cumbersome and also something we've removed and reintroduced before; so making it a composite action could make it more reasonable to keep around for next time it is needed. The problem is that there is a benefit to upgrading Git for Windows before, rather than after, performing any nontrivial Git operations, to decrease the likelihood of dangling subprocesses or otherwise open files preventing the Inno Setup installer from performing the upgrade. There is also a separate specific benefit to upgrading it before `actions/checkout` runs, so that step uses it, thereby effectively validating that it really works. But extracting the "Upgrade Git for Windows" step to a composite action that runs before the `actions/checkout` step is complicated, because the otherwise ideal way to refer to the action locally with a `./` path couldn't be used, because its code would not yet have been checked out. There are ways around this, but their complexity suggests they may not be worthwhile until a composite action is needed in order to actually decrease logic duplication. See the experiments in #68 for details.)
EliahKagan
added a commit
that referenced
this pull request
Jul 19, 2025
The "Install rustup" step was duplicated across two jobs: `test-fast` and `test-fixtures-windows`, both conditioned on `windows-11-arm`. There is furthermore the possibility that it may be added in the future to other jobs (such as if it ends up being useful to run the MSRV check on `windows-11-arm`). The duplication was such that it would be easy to wrongly change one but not the other occurrence. If that were to happen, and tests passed/failed or otherwise behaved differently due to differences in their CI environments, then it would create the wrong impression of being related to whether `GIX_TEST_IGNORE_ARCHIVES` is set. To decrease unnecessary duplication, and more specifically to avoid that specific confusion and the wild goose chase that might ensue from it, this extracts the logic of the "Install rustup" steps, other than the `if` key checking that we are running on `windows-11-arm`, to a newly created composite action `setup-windows-arm-rustup`. The reason for keeping the operating system check in the calling jobs is so that it is always immediately clear in the job output whether the step is meant to have an effect. Although composite actions often make sense to publish separately and use across repositories, that is probably not the case here. This logic does only what we need right now, and does not include functionality such as respecting a preexisitng `CARGO_HOME` environment variable or making sure to download `rustup-init.exe` to a location that would be reasonable for all callers. (This does *not* also extract the "Upgrade Git for Windows" step to a composite action. It's not necessary to do so, because the step only currently appears in one job: the `test-fixtures-windows` job, conditioned on `windows-11-arm`. It would, in principle, be useful to extract it nonetheless, because it is cumbersome and also something we've removed and reintroduced before; so making it a composite action could make it more reasonable to keep around for next time it is needed. The problem is that there is a benefit to upgrading Git for Windows before, rather than after, performing any nontrivial Git operations, to decrease the likelihood of dangling subprocesses or otherwise open files preventing the Inno Setup installer from performing the upgrade. There is also a separate specific benefit to upgrading it before `actions/checkout` runs, so that step uses it, thereby effectively validating that it really works. But extracting the "Upgrade Git for Windows" step to a composite action that runs before the `actions/checkout` step is complicated, because the otherwise ideal way to refer to the action locally with a `./` path couldn't be used, because its code would not yet have been checked out. There are ways around this, but their complexity suggests they may not be worthwhile until a composite action is needed in order to actually decrease logic duplication. See the experiments in #68 for details.)
EliahKagan
added a commit
that referenced
this pull request
Jul 19, 2025
The "Install rustup" step was duplicated across two jobs: `test-fast` and `test-fixtures-windows`, both conditioned on `windows-11-arm`. There is furthermore the possibility that it may be added in the future to other jobs (such as if it ends up being useful to run the MSRV check on `windows-11-arm`). The duplication was such that it would be easy to wrongly change one but not the other occurrence. If that were to happen, and tests passed/failed or otherwise behaved differently due to differences in their CI environments, then it would create the wrong impression of being related to whether `GIX_TEST_IGNORE_ARCHIVES` is set. To decrease unnecessary duplication, and more specifically to avoid that specific confusion and the wild goose chase that might ensue from it, this extracts the logic of the "Install rustup" steps, other than the `if` key checking that we are running on `windows-11-arm`, to a newly created composite action `setup-windows-arm-rustup`. The reason for keeping the operating system check in the calling jobs is so that it is always immediately clear in the job output whether the step is meant to have an effect. Although composite actions often make sense to publish separately and use across repositories, that is probably not the case here. This logic does only what we need right now, and does not include functionality such as respecting a preexisitng `CARGO_HOME` environment variable or making sure to download `rustup-init.exe` to a location that would be reasonable for all callers. (This does *not* also extract the "Upgrade Git for Windows" step to a composite action. It's not necessary to do so, because the step only currently appears in one job: the `test-fixtures-windows` job, conditioned on `windows-11-arm`. It would, in principle, be useful to extract it nonetheless, because it is cumbersome and also something we've removed and reintroduced before; so making it a composite action could make it more reasonable to keep around for next time it is needed. The problem is that there is a benefit to upgrading Git for Windows before, rather than after, performing any nontrivial Git operations, to decrease the likelihood of dangling subprocesses or otherwise open files preventing the Inno Setup installer from performing the upgrade. There is also a separate specific benefit to upgrading it before `actions/checkout` runs, so that step uses it, thereby effectively validating that it really works. But extracting the "Upgrade Git for Windows" step to a composite action that runs before the `actions/checkout` step is complicated, because the otherwise ideal way to refer to the action locally with a `./` path couldn't be used, because its code would not yet have been checked out. There are ways around this, but their complexity suggests they may not be worthwhile until a composite action is needed in order to actually decrease logic duplication. See the experiments in #68 for details.)
EliahKagan
added a commit
that referenced
this pull request
Jul 19, 2025
The "Install rustup" step was duplicated across two jobs: `test-fast` and `test-fixtures-windows`, both conditioned on `windows-11-arm`. There is furthermore the possibility that it may be added in the future to other jobs (such as if it ends up being useful to run the MSRV check on `windows-11-arm`). The duplication was such that it would be easy to wrongly change one but not the other occurrence. If that were to happen, and tests passed/failed or otherwise behaved differently due to differences in their CI environments, then it would create the wrong impression of being related to whether `GIX_TEST_IGNORE_ARCHIVES` is set. To decrease unnecessary duplication, and more specifically to avoid that specific confusion and the wild goose chase that might ensue from it, this extracts the logic of the "Install rustup" steps, other than the `if` key checking that we are running on `windows-11-arm`, to a newly created composite action `setup-windows-arm-rustup`. The reason for keeping the operating system check in the calling jobs is so that it is always immediately clear in the job output whether the step is meant to have an effect. Although composite actions often make sense to publish separately and use across repositories, that is probably not the case here. This logic does only what we need right now, and does not include functionality such as respecting a preexisitng `CARGO_HOME` environment variable or making sure to download `rustup-init.exe` to a location that would be reasonable for all callers. (This does *not* also extract the "Upgrade Git for Windows" step to a composite action. It's not necessary to do so, because the step only currently appears in one job: the `test-fixtures-windows` job, conditioned on `windows-11-arm`. It would, in principle, be useful to extract it nonetheless, because it is cumbersome and also something we've removed and reintroduced before; so making it a composite action could make it more reasonable to keep around for next time it is needed. The problem is that there is a benefit to upgrading Git for Windows before, rather than after, performing any nontrivial Git operations, to decrease the likelihood of dangling subprocesses or otherwise open files preventing the Inno Setup installer from performing the upgrade. There is also a separate specific benefit to upgrading it before `actions/checkout` runs, so that step uses it, thereby effectively validating that it really works. But extracting the "Upgrade Git for Windows" step to a composite action that runs before the `actions/checkout` step is complicated, because the otherwise ideal way to refer to the action locally with a `./` path couldn't be used, because its code would not yet have been checked out. There are ways around this, but their complexity suggests they may not be worthwhile until a composite action is needed in order to actually decrease logic duplication. See the experiments in #68 for details.)
EliahKagan
added a commit
that referenced
this pull request
Jul 22, 2025
The "Install rustup" step was duplicated across two jobs: `test-fast` and `test-fixtures-windows`, both conditioned on `windows-11-arm`. There is furthermore the possibility that it may be added in the future to other jobs (such as if it ends up being useful to run the MSRV check on `windows-11-arm`). The duplication was such that it would be easy to wrongly change one but not the other occurrence. If that were to happen, and tests passed/failed or otherwise behaved differently due to differences in their CI environments, then it would create the wrong impression of being related to whether `GIX_TEST_IGNORE_ARCHIVES` is set. To decrease unnecessary duplication, and more specifically to avoid that specific confusion and the wild goose chase that might ensue from it, this extracts the logic of the "Install rustup" steps, other than the `if` key checking that we are running on `windows-11-arm`, to a newly created composite action `setup-windows-arm-rustup`. The reason for keeping the operating system check in the calling jobs is so that it is always immediately clear in the job output whether the step is meant to have an effect. Although composite actions often make sense to publish separately and use across repositories, that is probably not the case here. This logic does only what we need right now, and does not include functionality such as respecting a preexisitng `CARGO_HOME` environment variable or making sure to download `rustup-init.exe` to a location that would be reasonable for all callers. (This does *not* also extract the "Upgrade Git for Windows" step to a composite action. It's not necessary to do so, because the step only currently appears in one job: the `test-fixtures-windows` job, conditioned on `windows-11-arm`. It would, in principle, be useful to extract it nonetheless, because it is cumbersome and also something we've removed and reintroduced before; so making it a composite action could make it more reasonable to keep around for next time it is needed. The problem is that there is a benefit to upgrading Git for Windows before, rather than after, performing any nontrivial Git operations, to decrease the likelihood of dangling subprocesses or otherwise open files preventing the Inno Setup installer from performing the upgrade. There is also a separate specific benefit to upgrading it before `actions/checkout` runs, so that step uses it, thereby effectively validating that it really works. But extracting the "Upgrade Git for Windows" step to a composite action that runs before the `actions/checkout` step is complicated, because the otherwise ideal way to refer to the action locally with a `./` path couldn't be used, because its code would not yet have been checked out. There are ways around this, but their complexity suggests they may not be worthwhile until a composite action is needed in order to actually decrease logic duplication. See the experiments in #68 for details.)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fork-internal PR may be merged to a feature branch, but the feature branch will likely be reset afterwards.