Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork938
Run Cygwin CI workflow commands in login shells#1709
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Merged
Merged
Uh oh!
There was an error while loading.Please reload this page.
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
This passes --login to the bash shell used to run commands in theCygwin environment on CI. This eliminates the need to work around apartly broken environment, and the extra code what was used to dothat is accordingly removed. There are two benefits of this change:- The PATH is correct: Cygwin's /usr/local/bin and /usr/bin are present at the beginning of PATH. Otherwise, it is easy to get /usr/bin at the front, but rather involved to get /usr/local/bin to precede it. Because Python on Cygwin puts scripts/executables such as the upgraded "pip" and the "pytest" command in /usr/local/bin, it is valuable to have that directory in the PATH and best to have it before /usr/bin. (I have set CYGWIN_NOWINPATH to omit other directories, since finding any of the commands to be run in the Cygwin environment outside that environment is unintended.)- Every step automatically has correct temporary directories: When Cygwin commands were not being run in login shells, they didn't automatically get correct values for TMP and TEMP for their environment. To work around this, those environment variables were set globally, for every step. But that caused them to refer to nonexistent locations for steps such as actions/checkout. Most likely this would not cause any errors, but it did cause copious warnings about a nonexistent temporary directory, which risked obscuring other potentially important output. Now that Cygwin commands run in login shells, both the few non-Cygwin steps, and the steps run in the Cygwin enviroment, all get correct temporary directories (with TMP and TEMP set in the prewritten startup script the login shell uses).A theoretical disadvantage of this is that login shells takeslightly longer to start up, but that delay is insigificant inthis application. A more significant disadvantage is that settingthe -x shell option the way it was done before would produce a lotof noise at the beginning of the output for every command-runningstep. To work around that, -x is omitted from the value of "shell"and "set -x" is added at the end of the startup script for loginshells, so it runs before each step's "payload" command, butwithout applying to the commands run in the startup script itself.
Byron approved these changesOct 14, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That's so awesome! Great discovery!
I can sense that native Windows support is just around the corner :).
renovatebot referenced this pull request in allenporter/flux-localOct 20, 2023
[](https://renovatebot.com)This PR contains the following updates:| Package | Change | Age | Adoption | Passing | Confidence ||---|---|---|---|---|---|| [GitPython](https://togithub.com/gitpython-developers/GitPython) |`==3.1.37` -> `==3.1.40` |[](https://docs.renovatebot.com/merge-confidence/)|[](https://docs.renovatebot.com/merge-confidence/)|[](https://docs.renovatebot.com/merge-confidence/)|[](https://docs.renovatebot.com/merge-confidence/)|---### Release Notes<details><summary>gitpython-developers/GitPython (GitPython)</summary>###[`v3.1.40`](https://togithub.com/gitpython-developers/GitPython/compare/3.1.38...3.1.40)[CompareSource](https://togithub.com/gitpython-developers/GitPython/compare/3.1.38...3.1.40)###[`v3.1.38`](https://togithub.com/gitpython-developers/GitPython/releases/tag/3.1.38)[CompareSource](https://togithub.com/gitpython-developers/GitPython/compare/3.1.37...3.1.38)#### What's Changed- Add missing assert keywords by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1678](https://togithub.com/gitpython-developers/GitPython/pull/1678)- Make clear every test's status in every CI run by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1679](https://togithub.com/gitpython-developers/GitPython/pull/1679)- Fix new link to license in readme by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1680](https://togithub.com/gitpython-developers/GitPython/pull/1680)- Drop unneeded flake8 suppressions by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1681](https://togithub.com/gitpython-developers/GitPython/pull/1681)- Update instructions and test helpers for git-daemon by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1684](https://togithub.com/gitpython-developers/GitPython/pull/1684)- Fix Git.execute shell use and reporting bugs by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1687](https://togithub.com/gitpython-developers/GitPython/pull/1687)- No longer allow CI to select a prerelease for 3.12 by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1689](https://togithub.com/gitpython-developers/GitPython/pull/1689)- Clarify Git.execute and Popen arguments by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1688](https://togithub.com/gitpython-developers/GitPython/pull/1688)- Ask git where its daemon is and use that by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1697](https://togithub.com/gitpython-developers/GitPython/pull/1697)- Fix bugs affecting exception wrapping in rmtree callback by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1700](https://togithub.com/gitpython-developers/GitPython/pull/1700)- Fix dynamically-set **all** variable by[@​DeflateAwning](https://togithub.com/DeflateAwning) in[https://github.com/gitpython-developers/GitPython/pull/1659](https://togithub.com/gitpython-developers/GitPython/pull/1659)- Fix small[#​1662](https://togithub.com/gitpython-developers/GitPython/issues/1662)regression due to[#​1659](https://togithub.com/gitpython-developers/GitPython/issues/1659)by [@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1701](https://togithub.com/gitpython-developers/GitPython/pull/1701)- Drop obsolete info on yanking from security policy by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1703](https://togithub.com/gitpython-developers/GitPython/pull/1703)- Have Dependabot offer submodule updates by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1702](https://togithub.com/gitpython-developers/GitPython/pull/1702)- Bump git/ext/gitdb from `49c3178` to `8ec2390` by[@​dependabot](https://togithub.com/dependabot) in[https://github.com/gitpython-developers/GitPython/pull/1704](https://togithub.com/gitpython-developers/GitPython/pull/1704)- Bump git/ext/gitdb from `8ec2390` to `6a22706` by[@​dependabot](https://togithub.com/dependabot) in[https://github.com/gitpython-developers/GitPython/pull/1705](https://togithub.com/gitpython-developers/GitPython/pull/1705)- Update readme for milestone-less releasing by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1707](https://togithub.com/gitpython-developers/GitPython/pull/1707)- Run Cygwin CI workflow commands in login shells by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1709](https://togithub.com/gitpython-developers/GitPython/pull/1709)#### New Contributors- [@​DeflateAwning](https://togithub.com/DeflateAwning) made theirfirst contribution in[https://github.com/gitpython-developers/GitPython/pull/1659](https://togithub.com/gitpython-developers/GitPython/pull/1659)**Full Changelog**:gitpython-developers/GitPython@3.1.37...3.1.38</details>---### Configuration📅 **Schedule**: Branch creation - At any time (no schedule defined),Automerge - At any time (no schedule defined).🚦 **Automerge**: Enabled.♻ **Rebasing**: Whenever PR becomes conflicted, or you tick therebase/retry checkbox.🔕 **Ignore**: Close this PR and you won't be reminded about this updateagain.---- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, checkthis box---This PR has been generated by [MendRenovate](https://www.mend.io/free-developer-tools/renovate/). Viewrepository job log[here](https://developer.mend.io/github/allenporter/flux-local).<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4xOS4yIiwidXBkYXRlZEluVmVyIjoiMzcuMTkuMiIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
%5d(https%3a%2f%2frenovatebot.com){kind=link}
EliahKagan added a commit to EliahKagan/GitPython that referenced this pull requestDec 23, 2023
This makes a pip -> pip3 symlink in /usr/bin in a new step prior tothe first step that runs the pip command. Using "pip3", "pip3.9",or a command like "python -m pip" would work, but this allows theCygwin workflow to continue using the same installation commands asthe main testing workflow.Adding this fixes two problems:1. When the pip version installed by the python39-pip package is current, so that upgrading pip doesn't install any new commmand, no "pip" command is created in /usr/local. This has happened not to be the case for a long time, which is why the Cygwin workflow was able to pass. (That the recent failures started at the merge ofgitpython-developers#1783 turns out to be a coincidence: rerunning jobs on prior commits has the failure, as does experimentally reverting it.)2. Even when the pip version installed by python39-pip is behind the latest available version, pip is still used before being upgraded to check if setuptools is installed, to decide whether to upgrade it. This is to keep similar steps in the two testing workflows similar, since the Cygwin workflow only uses Python 3.9, which always has setuptools. Because pip was never in $PATH in that step, the Cygwin workflow wrongly refrained from trying to upgrade setuptools.When the "Update PyPA packages" step does find a newer version ofpip to upgrade to, it installs it in /usr/local/bin, which we havein $PATH before /usr/bin, so the upgraded version, when present,will still be preferred in subsequent commands, as before.Running "pip" on Cygwin when it may not be in $PATH -- and, for onestep, never is -- was a bug introduced ine8956e5 (gitpython-developers#1709). Beforethat, "pip" still was not always available, but it was not used.This change fixes the bug by making sure "pip" is always available.
EliahKagan added a commit to EliahKagan/GitPython that referenced this pull requestDec 23, 2023
This makes a pip -> pip3 symlink in /usr/bin in a new step prior tothe first step that runs the pip command. Using "pip3", "pip3.9",or a command like "python -m pip" would work, but this allows theCygwin workflow to continue using the same installation commands asthe main testing workflow.Adding this fixes two problems:1. When the pip version installed by the python39-pip package is current, so that upgrading pip doesn't install any new commmand, no "pip" command is created in /usr/local. This has happened not to be the case for a long time, which is why the Cygwin workflow was able to pass. (That the recent failures started at the merge ofgitpython-developers#1783 turns out to be a coincidence: rerunning jobs on prior commits has the failure, as does experimentally reverting it.)2. Even when the pip version installed by python39-pip is behind the latest available version, pip is still used before being upgraded to check if setuptools is installed, to decide whether to upgrade it. This is to keep similar steps in the two testing workflows similar, since the Cygwin workflow only uses Python 3.9, which always has setuptools. Because pip was never in $PATH in that step, the Cygwin workflow wrongly refrained from trying to upgrade setuptools.When the "Update PyPA packages" step does find a newer version ofpip to upgrade to, it installs it in /usr/local/bin, which we havein $PATH before /usr/bin, so the upgraded version, when present,will still be preferred in subsequent commands, as before.Running "pip" on Cygwin when it may not be in $PATH -- and, for onestep, never is -- was a bug introduced ine8956e5 (gitpython-developers#1709). Beforethat, "pip" still was not always available, but it was not used.This change fixes the bug by making sure "pip" is always available.
EliahKagan added a commit to EliahKagan/GitPython that referenced this pull requestDec 23, 2023
This makes a pip -> pip3 symlink in /usr/bin in a new step prior tothe first step that runs the pip command. Using "pip3", "pip3.9",or a command like "python -m pip" would work, but this allows theCygwin workflow to continue using the same installation commands asthe main testing workflow.Adding this fixes two problems:1. When the pip version installed by the python39-pip Cygwin package is current, so that upgrading pip doesn't install any new commmand, no "pip" command is created in /usr/local. This has happened not to be the case for a long time, which is why the Cygwin workflow was able to pass. (That the recent failures started at the merge ofgitpython-developers#1783 turns out to be a coincidence: rerunning jobs on prior commits has the failure, as does experimentally reverting it.)2. Even when the pip version installed by python39-pip is behind the latest available version, pip is still used before being upgraded to check if setuptools is installed, to decide whether to upgrade it. This is to keep similar steps in the two testing workflows similar, since the Cygwin workflow only uses Python 3.9, which always has setuptools. Because pip was never in $PATH in that step, the Cygwin workflow wrongly refrained from trying to upgrade setuptools.When the "Update PyPA packages" step does find a newer version ofpip to upgrade to, it installs it in /usr/local/bin, which we havein $PATH before /usr/bin, so the upgraded version, when present,will still be preferred in subsequent commands, as before.Running "pip" on Cygwin when it may not be in $PATH -- and, for onestep, never is -- was a bug introduced ine8956e5 (gitpython-developers#1709). Beforethat, "pip" still was not always available, but it was not used.This change fixes the bug by making sure "pip" is always available.
EliahKagan added a commit to EliahKagan/GitPython that referenced this pull requestDec 23, 2023
This makes a pip -> pip3 symlink in /usr/bin in a new step prior tothe first step that runs the pip command. Using "pip3", "pip3.9",or a command like "python -m pip" would work, but this allows theCygwin workflow to continue using the same installation commands asthe main testing workflow.Adding this fixes two problems:1. When the pip version installed by the python39-pip Cygwin package is current, so that upgrading pip doesn't install any new commmand, no "pip" command is created in /usr/local. This has happened not to be the case for a long time, which is why the Cygwin workflow was able to pass. (That the recent failures started at the merge ofgitpython-developers#1783 turns out to be a coincidence: rerunning jobs on prior commits has the failure, as does experimentally reverting it.)2. Even when the pip version installed by python39-pip is behind the latest available version, pip is still used before being upgraded to check if setuptools is installed, to decide whether to upgrade it. This is to keep similar steps in the two testing workflows similar, since the Cygwin workflow only uses Python 3.9, which always has setuptools. Because pip was never in $PATH in that step, the Cygwin workflow wrongly refrained from trying to upgrade setuptools.When the "Update PyPA packages" step does find a newer version ofpip to upgrade to, it installs it in /usr/local/bin, which we havein $PATH before /usr/bin, so the upgraded version, when present,will still be preferred in subsequent commands, as before.Running "pip" on Cygwin when it may not be in $PATH -- and, for onestep, never is -- was a bug introduced ine8956e5 (gitpython-developers#1709). Beforethat, "pip" still was not always available, but it was not used.This change fixes the bug by making sure "pip" is always available.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Labels
None yet
2 participants
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.
Uh oh!
There was an error while loading.Please reload this page.
This passes
--loginto thebashshell used to run commands in the Cygwin environment on CI. This eliminates the need to work around a partly broken environment, and the extra code what was used to do that is accordingly removed. There are two benefits of this change:The
PATHis correct: Cygwin's/usr/local/binand/usr/binare present at the beginning ofPATH. Otherwise, it is easy to get/usr/binat the front, but rather involved to get/usr/local/binto precede it. Because Python on Cygwin puts scripts/executables such as the upgradedpipand thepytestcommand in/usr/local/bin, it is valuable to have that directory in thePATHand best to have it before/usr/bin. (I have setCYGWIN_NOWINPATHto omit other directories, since finding any of the commands to be run in the Cygwin environment outside that environment is unintended.)Every step automatically has correct temporary directories: When Cygwin commands were not being run in login shells, they didn't automatically get correct values for
TMPandTEMPfor their environment. To work around this, those environment variables were set globally, for every step. But that caused them to refer to nonexistent locations for steps such asactions/checkout. Most likely this would not cause any errors, but it did cause copious warnings about a nonexistent temporary directory, which risked obscuring other potentially important output. Now that Cygwin commands run in login shells, both the few non-Cygwin steps, and the steps run in the Cygwin environment, all get correct temporary directories (withTMPandTEMPset in the prewritten startup script the login shell uses).A theoretical disadvantage of this is that login shells take slightly longer to start up, but that delay is insignificant in this application. A more significant disadvantage is that setting the
-xshell option the way it was done before would produce a lot of noise at the beginning of the output for every command-running step. To work around that,-xis omitted from the value ofshellandset -xis added at the end of the startup script for login shells, so it runs before each step's "payload" command, but without applying to the commands run in the startup script itself.