-
-
Notifications
You must be signed in to change notification settings - Fork 281
Specify bash
shell in CI Workflows
#605
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
Conversation
@Lee-W I've noticed the
in several workflows and opened the following issues to try to resolve it: |
@Lee-W Until the issue is resolved, can we add |
Codecov ReportBase: 98.40% // Head: 97.90% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #605 +/- ##
==========================================
- Coverage 98.40% 97.90% -0.51%
==========================================
Files 39 35 -4
Lines 1634 1243 -391
==========================================
- Hits 1608 1217 -391
Misses 26 26
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
@Lee-W The |
6963fa4
to
90e5cc8
Compare
180f4f2 seems to be weird 🤔 Could you please rebase from the main branch and see whether we could remove this commit? |
Yes, my apologies. I was experimenting between |
Hmmm...rebasing didn't seem to do the trick, so I reverted that commit just to be safe. |
Specify `shell` as `bash` in `Run tests and linters` step. Fixes: Issue commitizen-tools#604
Switch `#!/bin/sh` to `#!/usr/bin/env sh` so scripts work on Windows as well as Linux and MacOS.
- Change shebang from `#!/bin/sh` to `#!/usr/bin/env sh` - Set `$PREFIX` depending on OS
This should be used when parsing binary files, not text streams. The regular expression fails on Windows because the stream contains `\n`, but `os.linesep` is `\r\n`. For reference, see https://stackoverflow.com/questions/38074811/what-is-os-linesep-for
Use `os.sep` for file paths. On Windows, this is `\`. On Linux and MacOS, this is `/`.
Windows uses `\` for path separation, causing python to see the `\U` in `C:\Users` as a Unicode string. Replace `\\` with `/` in file path strings. Also, the colon in drive paths on Windows causes the `partition` logic in `bump.py` and `commands/bump.py` to fail. Perform the logic on the path without the drive portion.
`argcomplete` does not support Git Bash on Windows out of the box. For details, see https://kislyuk.github.io/argcomplete/#git-bash-support.
On Windows, `Popen` with `shell=True` reads the `COMSPEC` environment variable to determine the shell to run, which defaults to Command Prompt (see https://docs.python.org/3/library/subprocess.html#subprocess.run). For backwards-compatibility for older OS utility scripts, `COMSPEC` should not be changed on Windows. Command Prompt requires double-quotes instead of single-quotes. See - https://stackoverflow.com/questions/25534995/escape-double-quotes-in-git-config-from-cmd) - https://docs.python.org/3/library/subprocess.html#converting-an-argument-sequence-to-a-string-on-windows Note that although the commitizen shell `scripts` shebangs have been changed to a portable variety (i.e. `#!/usr/bin/env sh`), which runs Git Bash on Windows when Git is installed (i.e. the `sh.exe` executable that ships with Git for Windows), `subprocess.Popen()` opens a child process from the active running shell (i.e. via `CreateProcess()`). Hence, it will still read `COMSPEC` and run a Command Prompt shell.
The Windows path separator is the backslash (i.e. `\\`). This causes problems when python reads files from the `Users` folder since `\\U` is the prefix for a Unicode string in python. Since `os.path` can properly parse forward slashes on all OS's, we substitute `\\` for `/` wherever `os.path` methods return backslashes.
For some reason, `pytest-mock` does not affect `os.isatty` when `sys.stdin` is mocked to use `StringIO` (i.e. a file-like object). Checking `sys.stdin.isatty()` works and is equivalent to checking `os.isatty(0)` (`stdin` is file descriptor `0`).
For `pydocstyle`, use `--match-dir` switch to prevent traversal into `venv/` folder. For `commitizen`, use `cz` instead of `commitizen` as `cz` is present on all OS's, but `commitizen` is not present in `venv/Scripts` on Windows.
Add error message if `test` script encounters unsupported OS.
`py -m` was added to command in `test_argcomplete_activation` during experimentation and was forgotten to be removed.
d8aa4f1
to
4536f93
Compare
🤦♂️ Doh! I didn't actually rebase properly; my mistake. This should NOW be fixed. |
Commit `d936262e` switched the `test` script to use `cz` instead of `commitizen`, but forgot the prefix to the command is `poetry run python -m`, which looks for a python module. `cz` is not a module: it is an entry point script (specified in the `pyproject.toml` table `[tool.poetry.scripts]`). Switching `$PREFIX` to `venv/bin/` creates two problems: - The subdirectory `venv` creates differs depending on OS. On Linux/MacOS, the subdirectory containing copies/symlinks to Python binaries is `bin`, but on Windows it is `Scripts`. Similiarly, the `site-packages` directory on Linux/MacOS is `lib/pythonX.Y/site-packages`, but on Windows it is `Lib\site-packages`. See: https://docs.python.org/3/library/venv.html#module-venv - Since `commitizen` isn't the entry-point script, but rather `cz`, the command `venv/bin/commitizen` will fail on Linux/MacOS and Windows. Poetry will check to see if it is currently running inside a virtual environment, and if it is, it will use it directly without creating a new one. If it isn't, it will use one that it has already created or create a brand new one for you. See: https://python-poetry.org/docs/managing-environments/ A better solution than switching the prefix would be to activate the virtual environment. However, users can create virtual environments with any name they choose (`venv/`, `.venv/`, or otherwise). In addition, they can specify whether or not `poetry` should create a virtual environment in the project folder `virtualenvs.in-project`), or at all (`virtualenvs.create`). Since switching to `venv/bin` only affects developers running scripts locally (the GitHub Actions CI scripts run `poetry install` and let it manage the virtual environment), it's best to remove changing the `$PREFIX` and instead add instructions to `README`/`CONTRIBUTING`/etc. to tell users to configure `poetry` and activate their virtual environment appropriately if running these scripts locally.
Command Prompt on Windows requires double-quotes. Otherwise, the command `${PREFIX}pydocstyle --convention=google --add-ignore=D1,D415 --match-dir="^(?![.]|venv).*"` will fail with 'venv).*' is not recognized as an internal or external command this will not affect the command in other Linux-based/bash shells.
Last CI run ended with a random `poetry` exception on Windows, which happens from time-to-time.
Pipe appears broken in certain instances within the version of MinGW64 that ships as Git Bash in Git for Windows. See: - https://sourceforge.net/p/mingw/bugs/2303/ - msys2/MINGW-packages#13662
@Lee-W All tests are now passing. Please kindly re-review. Code coverage has dropped 0.5%; are you alright with that? |
@woile Pinging you as well, in case you want to be another reviewer. |
|
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.
I took a brief look and i'm good with these changes. @woile Could you please take a look when you have time? Thanks
@woile I plan to merge this one late this week. Please let me know if you want to take a deeper look :) |
LGTM, thanks for the PR |
shell
asbash
inRun tests and linters
step.Failing Windows Tests (generated with
pytest-html
plugin):pytest_report.zip
Fixes: Issue #602, #604