Improve scripts and tool configurations #1693
Merged
Conversation
This file contains 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 also reorders the hooks from pre-commit/pre-commit-hooks so that the overall order of all hooks from all repositories is: lint Python, lint non-Python, non-lint.
Its output is colorized normally, but on CI it is not colorized without this (pre-commit's own output is, but not shellcheck's).
This suppresses ShellCheck SC2016, "Double quote to prevent globbing and word splitting," on the command in the version check script that expands $config_opts to build the "-c ..." arguments. It also moves the code repsonsible for getting the latest tag, which this is part of, into a function for that purpose, so it's clear that building config_opts is specifically for that, and so that the code is not made harder to read by adding the ShellCheck suppression comment. (The suppression applies only to the immediate next command.)
In the release building script, this changes $1 to "$1", because $1 without quotes means to perform word splitting and globbing (pathname expansion) on the parameter (unless otherwise disabled by the value of $IFS or "set -f", respectively) and use the result of doing so, which isn't the intent of the code. This function is only used from within the script, where it is not given values that would be changed by these additional expansions. So this is mainly about communicating intent. (If in the future it is intended that multiple arguments be usable, then they should be passed as separate arguments to release_with, which should be modified by replacing "$1" with "$@". I have not used "$@" at this time because it is not intuitively obvious that the arguments should be to the interpreter, rather than to the build module, so I don't think this should be supported unless or until a need for it determines that.)
I think this is easier to read, but this is admittedly subjective. This commit also makes the separate change of adjusting comment spacing for consistency within the script. (Two spaces before "#" is not widely regarded as better than one in shell scripting, so unlike Python where PEP-8 recommends that, it would be equally good to have changed all the other places in the shell scrips to have just one.)
The script is nonportable to other shells already because of its use of trap (and other features, such as implicit assumptions made about echo, and the function keyword), which its hashbang already clearly conveys. This uses: - $(<X) in place of $(cat X), to have the shell read the file directly rather than using cat. - printf -v in one place to set a variable rather than printing. This eliminates a command substitution that was harder to read.
Because users may have an old version of git without "git switch",
init-tests-after-clone.sh should continue to use "git checkout" to
attempt to switch to master. But without "--", this suffers from
the problem that it's ambiguous if master is a branch (so checkout
behaves like switch) or a path (so checkout behaves like restore).
There are two cases where this ambiguity can be a problem. The most
common is on a fork with no master branch but also, fortunately, no
file or directory named "master". Then the problem is just the
error message (printed just before the script proceeds to redo
the checkout with -b):
error: pathspec 'master' did not match any file(s) known to git
The real cause of the error is the branch being absent, as happens
when a fork copies only the main branch and the upstream remote is
not also set up. Adding the "--" improves the error message:
fatal: invalid reference: master
However, it is possible, though unlikely, for a file or directory
called "master" to exist. In that case, if there is also no master
branch, git discards unstaged changes made to the file or (much
worse!) everywhere in that directory, potentially losing work.
This commit adds "--" to the right of "master" so git never
regards it as a path. This is not needed with -b, which is always
followed by a symbolic name, so I have not added it there.
(Note that the command is still imperfect because, for example, in
rare cases there could be a master *tag*--and no master branch--in
which case, as before, HEAD would be detached there and the script
would attempt to continue.)
This had been done everywhere except in init-tests-after-clone.sh.
This translates init-tests-after-clone.sh from bash to POSIX sh,
changing the hashbang and replacing all bash-specific features, so
that it can be run on any POSIX-compatible ("Bourne style") shell,
including but not limited to bash. This allows it to be used even
on systems that don't have any version of bash installed anywhere.
Only that script is modified. The other shell scripts make greater
use of (and gain greater benefit from) bash features, and are also
only used for building and releasing. In contrast, this script is
meant to be used by most users who clone the repository.
This makes three related changes: - Removes "git fetch --tags" from the instructions in the readme, because the goal of this command can be achieved in the init-tests-after-clone.sh script, and because this fetch command, as written, is probably only achieving that goal in narrow cases. In clones and fetches, tags on branches are fetched by default, and the tags needed by the tests are on branches. So the situations where "git fetch --tags" was helping were (a) when the remote recently gained the tags, and (b) when the original remote was cloned in an unusual way, not fetching all tags. In both cases, the "--tags" option is not what makes that fetch get the needed tags. - Adds "git fetch --all --tags" to init-tests-after-clone.sh. The "--all" option causes it to fetch from all remotes, and this is more significant than "--tags", since the tags needed for testing are on fetched branches. This achieves what "git fetch --tags" was achieving, and it also has the benefit of getting tags from remotes that have been added but not fetched from, as happens with an upstream remote that was manually added with no further action. (It also gets branches from those remotes, but if master is on multiple remotes but at different commits then "git checkout master" may not be able to pick one. So do this *after* rather than before that.) - Skips this extra fetch, and also the submodule cloning/updating step, when running on CI. CI jobs will already have taken care of cloning the repo with submodules recursively, and fetching all available tags. In forks without tags, the necessary tags for the test are not fetched--but the upstream remote is not set up on CI, so they wouldn't be obtained anyway, not even by refetching with "--all".
This extracts duplicated code to functions in check-version.sh. One effect is unfortunately that the specific commands being run are less explicitly clear when reading the script. However, small future changes, if accidentally made to one but not the other in either pair of "git status" commands, would create a much more confusing situation. So I think this change is justified overall.
- Because the substitition string after the hyphen is empty,
"${VIRTUAL_ENV:-}" and "${VIRTUAL_ENV-}" have the same effect.
However, the latter, which this changes it to, expresses the
correct idea that the special case being handled is when the
variable is unset: in this case, we expand an empty field rather
than triggering an error due to set -u. When the variable is set
but empty, it already expands to the substitution value, and
including that in the special case with ":" is thus misleading.
- Continuing in the vein of d18d90a (and 1e0b3f9), this removes
another explicit newline by adding another echo command to print
the leading blank line before the table.
This adds comments to init-tests-after-clone.sh to explain what each of the steps is doing that is needed by some of the tests. It also refactors some recently added logic, in a way that lends itself to greater clarity when combined with these comments.
This is already done in the other shell scripts. Although init-tests-after-clone.sh does not have as many places where a bug could slip through by an inadvertently nonexistent parameter, it does have $answer (and it may have more expansions in the future).
This is a minor improvement to the robustness of the command for "make all", in the face of plausible future target names. - Use [[:alpha:]] instead of [a-z] because, in POSIX BRE and ERE, whether [a-z] includes capital letters depends on the current collation order. (The goal here is greater consistency across systems, and for this it would also be okay to use [[:lower:]].) - Pass -x to the command that filters out the all target itself, so that the entire name must be "all", because a future target may contain the substring "all" as part of a larger word.
This seems simpler to me, but I admit it is subjective.
- Add the psf/black-pre-commit-mirror pre-commit hook but have it just check and report violations rather than fixing them automatically (to avoid inconsistency with all the other hooks, and also so that the linting instructions and CI lint workflow can remain the same and automatically do the black check). - Remove the "black" environment from tox.ini, since it is now part of the linting done in the "lint" environment. (But README.md remains the same, as it documented actually auto-formatting with black, which is still done the same way.) - Add missing "exclude" keys for the shellcheck and black pre-commit hooks to explicitly avoid traversing into the submodules.
This splits the pre-commit hook for black into two hooks, both using the same repo and id but separately aliased: - black-check, which checks but does not modify files. This only runs when the manual stage is specified, and it is used by tox and on CI, with tox.ini and lint.yml modified accordingly. - black-format, which autoformats code. This provides the behavior most users will expect from a pre-commit hook for black. It runs automatically along with other hooks. But tox.ini and lint.yml, in addition to enabling the black-check hook, also explicitly skip the black-format hook. The effect is that in ordinary development the pre-commit hook for black does auto-formatting, but that pre-commit continues to be used effectively for running checks in which code should not be changed.
In the lint workflow, passing extra-args replaced --all-files, rather than keeping it but adding the extra arguments after it. (But --show-diff-on-failure and --color=always were still passed.)
Now that the changes to lint.yml are fixed up, tested, and shown to be capable of revealing formatting errors, the formatting error I was using for testing (which is from 9fa1cee where I had introduced it inadvertently) can be fixed.
As on CI and with tox (but not having to build and create and use a tox environment). This may not be the best way to do it, since currently the project's makefiles are otherwise used only for things directly related to building and publishing. However, this seemed like a readily available way to run the moderately complex command that produces the same behavior as on CI or with tox, but outside a tox environment. It may be possible to improve on this in the future.
Including tox.
This adds BUILDDIR as a variable in the documentation generation makefile that, along SPHINXOPTS, SPHINXBUILD, and PAPER, defaults to the usual best value but can be set when invoking make. The main use for choosing a different build output directory is to test building without overwriting or otherwise interfering with any files from a build one may really want to use. tox.ini is thus modified to pass a path pointing inside its temporary directory as BUILDDIR, so the "html" tox environment now makes no changes outside the .tox directory. This is thus suitable for being run automatically, so the "html" environment is now in the envlist.
As well as still checking for Travis, for backward compatibility and because experience shows that this is safe. The check can be much broader, and would be more accurate, with fewer false negatives. But a false positive can result in local data loss because the script does hard resets on CI without prompting for confirmation. So for now, this just checks $TRAVIS and $GITHUB_ACTIONS. Now that GHA is included, the CI workflows no longer need to set $TRAVIS when running the script, so that is removed.
This extends the init script so it tries harder to get version tags: - As before, locally, "git fetch --all --tags" is always run. (This also fetches other objects, and the developer experience would be undesirably inconsistent if that were only sometimes done.) - On CI, run it if version tags are absent. The criterion followed here and in subsequent steps is that if any existing tag starts with a digit, or with the letter v followed by a digit, we regard version tags to be present. This is to balance the benefit of getting the tags (to make the tests work) against the risk of creating a very confusing situation in clones of forks that publish packages or otherwise use their own separate versioning scheme (especially if those tags later ended up being pushed). - Both locally and on CI, after that, if version tags are absent, try to copy them from the original gitpython-developers/GitPython repo, without copying other tags or adding that repo as a remote. Copy only tags that start with a digit, since v+digit tags aren't currently used in this project (though forks may use them). This is a fallback option and it always displays a warning. If it fails, another message is issued for that. Unexpected failure to access the repo terminates the script with a failing exit status, but the absence of version tags in the fallback remote does not, so CI jobs can continue and reveal which tests fail as a result. On GitHub Actions CI specifically, the Actions syntax for creating a warning annotation on the workflow is used, but the warning is still also written to stderr (as otherwise). GHA workflow annotations don't support multi-line warnings, so the message is adjusted into a single line where needed.
In the step output, the warning that produces a workflow annotation is fully readable (and even made to stand out), so it doesn't need to be printed in the plain way as well, which can be reserved for when GitHub Actions is not detected.
The tox environments that are not duplicated per Python version were set to run on Python 3.9, for consistency with Cygwin, where 3.9 is the latest available (through official Cygwin packages), so output can be compared between Cygwin and other platforms while troubleshooting problems. However, this prevented them from running altogether on systems where Python 3.9 was not installed. So I've added all the other Python versions the project supports as fallback versions for those tox environments to use, in one of the reasonable precedence orders, while keeping 3.9 as the first choice for the same reasons as before.
This changed a while ago but README.md still listed having a main branch as a condition for automatic CI linting and testing in a fork.
This is probably the *only* way anyone should run that script on Windows, but I don't know of specific bad things that happen if it is run in some other way, such as with WSL bash, aside from messing up line endings, which users are likely to notice anyway. This commit also clarifies the instructions by breaking up another paragraph that really represented two separate steps.
EliahKagan
commented
Oct 4, 2023
Byron
approved these changes
Oct 4, 2023
There was a problem hiding this comment.
Thank you so much!
However, I would be pleased to make further changes as requested.
Even though I think you should do what seems best to you right now, I'd hope that the considerable time that I assume goes into the related issues and PRs could also be channeled towards not making improvements to these scripts necessary as one takes a path towards test isolation, which, as you already pointed out, would make such script unnecessary.
It is inelegant, though, [..]
Maybe you would consider it an improvement to 'upgrade' the Makefile like this:
help: ## Display this help
@awk 'BEGIN {FS = ":.*##"; printf "\nNote: Make is only for specific functionality used by CI - run `just` for developer targets:\n make \033[36m<target>\033[0m\n"} /^[a-zA-Z_-]+:.*?##/ { printf " \033[36m%-15s\033[0m %s\n", $$1, $$2 } /^##@/ { printf "\n\033[1m%s\033[0m\n", substr($$0, 5) } ' $(MAKEFILE_LIST)
always:
##@ Release Builds
release-all: release release-lean release-small ## all release builds
release: always ## the default build, big but pretty (builds in ~2min 35s)
cargo build --release
release-lean: always ## lean and fast, with line renderer (builds in ~1min 30s)
cargo build --release --no-default-features --features lean
release-small: always ## minimal dependencies, at cost of performance (builds in ~46s)
cargo build --release --no-default-features --features small
##@ Debug Builds
debug: always ## the default build, big but pretty
cargo build
debug-lean: always ## lean and fast, with line renderer
cargo build --no-default-features --features lean
debug-small: always ## minimal dependencies, at cost of performance
cargo build --no-default-features --features smallThat way, documentation and headers can be inlined and will be displayed by default.
If the goal is to maximize portability then another direction could also be taken depending on how you see fit. I am pretty sure python projects use python as 'hub' script, too, these days.
| head_sha="$(git rev-parse HEAD)" | ||
| latest_tag_sha="$(git rev-parse "${latest_tag}^{commit}")" | ||
|
|
||
| # Display a table of all the current version, tag, and HEAD commit information. | ||
| echo $'\nThe VERSION must be the same in all locations, and so must the HEAD and tag SHA' | ||
| echo |
There was a problem hiding this comment.
This conveys spacing much better thanks to the separate echo line, even though I have a feeling this was done for portability as $'magic' might not be allowed everywhere.
There was a problem hiding this comment.
I did it for spacing, and to to take fuller advantage of echo to have fewer \n sequences. Although it's true that $' ' is not POSIX and shouldn't be used in a #!/bin/sh script, this script is a bash script and $' ' is a available in bash on all systems. (ksh, zsh, and probably some other shells also have $' ', but not all POSIX-compatible shells have it.)
Regarding spacing, the table is currently formatted using printf and this has the advantage that its own output spacing can be adjusted by, for example, changing 14 to 15. But I originally used printf for it because I was preferring printf to echo, because originally I was doing this inline in Makefile, and a Makefile runs commands using /bin/sh (unless SHELL is assigned in the Makefile itself). This was to follow the practice of avoiding echo in portable sh scripts, at least when displaying data read from files.
But that is not necessary in a #!/bin/bash script, where we know how echo works and where echo doesn't expand escape sequences by default. So it would actually be okay, at this point, to go back to displaying the table this way:
echo
echo 'The VERSION must be the same in all locations, and so must the HEAD and tag SHA'
echo "VERSION file = $version_version"
echo "changes.rst = $changes_version"
echo "Latest tag = $latest_tag"
echo "HEAD SHA = $head_sha"
echo "Latest tag SHA = $latest_tag_sha"This is arguably more readable, and arguably conveys spacing the best. I'd be happy to change it to this if you like.
(Another option could be to go in the opposite direction and make this check-version.sh script, and maybe the build-release.sh script as well, a #!/bin/sh script, which is feasible.)
There was a problem hiding this comment.
This is arguably more readable, and arguably conveys spacing the best. I'd be happy to change it to this if you like.
(Another option could be to go in the opposite direction and make thischeck-version.shscript, and maybe thebuild-release.shscript as well, a#!/bin/shscript, which is feasible.)
Since trap is supported in POSIX shells and thus sh, making these scripts more portable seems valuable, even though I think that as the project currently is setup there isn't any need as it's only me using them.
Alternatives involve running these on CI which also supports bash everywhere (at least so it seems).
If you think there is benefits to portability assuming that one day CI can perform trusted publishes, I think it's fair to adjust for portability next time you touch them. Otherwise, it would be just as fair to change printf with echo as shown above. It's perfectly optional as well.
There was a problem hiding this comment.
The desired interaction of trap and set -e may be achievable in sh. Even if not, we don't have to use trap; it can be replaced even without making the control-flow logic more complicated or error prone. For example, the script could accept -q to not append the failure message, and otherwise invoke itself with -q, check the result, print the message if it failed, and exit with the original failing exit code. (There is also an argument to be made that set -e shouldn't be relied on at all.)
There are not many systems where bash doesn't exist and can't be made available. However, due to the problem described in 729372f--which only happens when MinGW make is being used on a native Windows system that has also WSL installed, and does not happen outside make nor on other systems--the scripts' bash hashbangs are written as #!/bin/bash rather than #!/usr/bin/env bash, and thus assume bash is in /bin.
Making them #!/bin/sh scripts would also fix that. It is even safer to assume sh is in bin than to assume env is in /usr/bin, so #!/usr/bin/env sh is rarely used. (Anyway, there is no corresponding WSL-related sh.exe in System32.) So, when combined with the other minor reasons for using #!/bin/sh, that is probably worth doing.
As you've suggested, next time I work on those scripts I'll look into making them POSIX sh scripts. However, this would be unrelated to facilitating trusted publishing. CI does support bash everywhere, at least on runners hosted by GitHub Actions.
This was referenced Oct 7, 2023
EliahKagan
added a commit
to EliahKagan/GitPython
that referenced
this pull request
Oct 18, 2023
Since 7110bf8 (in gitpython-developers#1693), "git submodule update --init --recursive" was not run on CI, on the mistaken grounds that the CI test workflows would already have taken care of cloning all submodules (ever since 4eef3ec when the "submodules: recursive" option was added to the actions/checkout step). This changes the init-tests-after-clone.sh script to again run that command unconditionally, including on CI. The assumption that it wasn't needed on CI was based on the specific content of GitPython's own GitHub Actions workflows. But this disregarded that the test suite is run on CI for *other* projects: specifically, for downstream projects that package GitPython.
EliahKagan
added a commit
to EliahKagan/GitPython
that referenced
this pull request
Oct 18, 2023
Since 7110bf8 (in gitpython-developers#1693), "git submodule update --init --recursive" was not run on CI, on the mistaken grounds that the CI test workflows would already have taken care of cloning all submodules (ever since 4eef3ec when the "submodules: recursive" option was added to the actions/checkout step). This changes the init-tests-after-clone.sh script to again run that command unconditionally, including on CI. The assumption that it wasn't needed on CI was based on the specific content of GitPython's own GitHub Actions workflows. But this disregarded that the test suite is run on CI for *other* projects: specifically, for downstream projects that package GitPython (gitpython-developers#1713). This also brings back the comment from fc96980 that says more about how the tests rely on submodules being present (specifically, that they need a submodule with a submodule). However, that is not specifically related to the bug being fixed.
For the benefit of anyone (including future me) finding this while searching for it, the issue for how GitPython's tests currently depend on the GitPython repository being present is #914. |
EliahKagan
added a commit
to EliahKagan/GitPython
that referenced
this pull request
Mar 12, 2024
This is to make it so simple `tox` usage has the expected property of leaving all source code files in the working tree unchanged. As noted in c66257e, linting how sometimes performs auto-fixes since gitpython-developers#1862, and the pre-commit command in tox.ini, which had also run `black --check`, will do even more file editing with gitpython-developers#1865. The bifurcation for black into separate mutating and non-mutating hooks, introduced in 5d8ddd9 (gitpython-developers#1693), is not carried over into Ruff autoformatting in gitpython-developers#1865. But also it: - Was not necessarily a good approach, and likely should not be preserved in any form. It is an unusual and unintuitive use of pre-commit. (It can be brought back if no better approach is found, though.) - Was done to avoid a situation where it was nontrivial to set up necessary dependencies for linting in the GitPython virtual environment itself, because flake8 and its various plugins would have to be installed. They were not listed in any existing or newly introduced extra (for example, they were not added to test-requirements.txt) in part in the hope that they would all be replaced by Ruff, which happened in gitpython-developers#1862. - Already does not achieve its goal since gitpython-developers#1862, since it was (probably rightly) not extended to Ruff linting to use/omit --fix. Now that Ruff is being used, people can run `pip install ruff` in a virtual environment, then run the `ruff` command however they like. This takes the place of multiple tools and plugins. This commit *avoids* doing any of the following, even though it may be useful to do them later: - This does not give specific instructions in the readme for installing and running ruff (and c66257e before this also omits that). This can be added later and the best way to document it may depend on some other upcoming decisions (see below). - This does not add ruff to the test extra or as any other kind of extra or optional dependency. Although the test extra currently contains some packages not used for running unit tests, such as pre-commit and mypy, adding Ruff will cause installation to take a long time and/or or fail on some platforms like Cygwin where Ruff has to be built from (Rust) source code. This can be solved properly by reorganizing the extras, but that is likely to wait until they are expressed completely inside pyproject.toml rather than one per requirements file (see discussion in comments in gitpython-developers#1716 for general information about possible forthcoming changes in how the project is defined). - This does not update tox.ini to run ruff directly, which could be done by splitting the current lint tox environment into two or three environments for Python linting, Python autoformatting, and the miscellaneous other tasks performed by pre-commit hooks, only the latter of which would use the pre-commit command. Whether and how this should be done may depend on other forthcoming changes. - This does not remove or update the Makefile "lint" target. That target should perhaps not have been added, and was always meant to be improved/replaced, since everything else in the top-level Makefile is for building and publishing releases. See 5d15063 (gitpython-developers#1693). This is likewise not done now since it may depend on as-yet unmerged changes and tooling decisions not yet made. It should be feasible to do together when further updating tox.ini. - This does not update tox.ini, Makefile, or the lint.yml GitHub Actions workflow to omit the manual hook-stage, which will be unused as of gitpython-developers#1865. This would complicate integration of changes, and once it is known that it won't be needed, it can removed. The situation with the tox "lint" environment is thus now similar to that of the tox "html" environment when it was added in e6ec6c8 (gitpython-developers#1667), until it was improved in f094909 (gitpython-developers#1693) to run with proper isolation.
EliahKagan
added a commit
to EliahKagan/GitPython
that referenced
this pull request
Mar 13, 2024
This is to make it so simple `tox` usage has the expected property of leaving all source code files in the working tree unchanged. Linting how sometimes performs auto-fixes since gitpython-developers#1862, and the pre-commit command in tox.ini, which had also run `black --check`, will do even more file editing due to the changes in gitpython-developers#1865. The bifurcation for black into separate mutating and non-mutating hooks, introduced in 5d8ddd9 (gitpython-developers#1693), was not carried over into Ruff autoformatting in gitpython-developers#1865. But also it: - Was not necessarily a good approach, and likely should not be preserved in any form. It was an unusual and unintuitive use of pre-commit. (It can be brought back if no better approach is found, though.) - Was done to avoid a situation where it was nontrivial to set up necessary dependencies for linting in the GitPython virtual environment itself, because flake8 and its various plugins would have to be installed. They were not listed in any existing or newly introduced extra (for example, they were not added to test-requirements.txt) in part in the hope that they would all be replaced by Ruff, which happened in gitpython-developers#1862. - Already did not achieve its goal as of gitpython-developers#1862, since it was (probably rightly) not extended to Ruff linting to use/omit --fix. Now that Ruff is being used, people can run `pip install ruff` in a virtual environment, then run the `ruff` command however they like. This takes the place of multiple tools and plugins. The situation with the tox "lint" environment is thus now similar to that of the tox "html" environment when it was added in e6ec6c8 (gitpython-developers#1667), until it was improved in f094909 (gitpython-developers#1693) to run with proper isolation.
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.
Fixes #1690
Fixes #1691
Fixes #1692
This combines the solutions I recommended in those three issues on a single branch. Although I feel this does compose into a coherent whole, with each of those issues' practical ramifications having some tendrils into each other, and opening separate PRs would have resulted in nontrivial merge conflicts... nonetheless this PR is broader than I would usually like. But I have done it this way because it was by working on them together that I was able to get clear on the distinct ideas that I presented as those issues, as well as whether the solutions would work reasonably when all combined.
This PR is what remains after pieces that seemed reasonable to sever into their on PRs, such as #1684, have been removed, and excessively enterprising ideas, such as writing a batch-file version of
init-tests-after-clone.sh(discussed in #1691) or switchinglint.ymlto use pre-commit.ci, abandoned or deferred. However, I would be pleased to make further changes as requested. I think the changes here are feasible to review together, but I am willing to rework this into multiple more narrowly scoped PRs if necessary.The problems and their solutions in general are presented in the three linked issues that this would close. (Details are given in commit messages.)
Specific considerations that I suspect may be important when reviewing this PR follow.
Shell script portability
I believe the shell scripts that pertain to building do not need to be any more portable than they are now: they run on
bashversion 3 or later, which most systems have or can get. I did make modifications to them, some of which were inspired and emboldened byshellcheck, but not for portability to other shells.I take #1661 (comment) (specifically: d18d90a, 1e0b3f9) as an indication of a general preference of
echooverprintf. Thebashshell provides anechobuiltin that has good design decisions and, more importantly, behaves the same inbashon any system, because it is a builtin. (I have actually slightly increased the use ofechoinbashin this PR.)However, for POSIX shell scripts that assume only what the standard insists on (or that call
echothrough another command, such as withfind -execorxargs, thereby using an externalechoexecutable), the situation is considerably messier. To write a portable script, I have avoided the use ofechoin the init script that, for portability, I have changed frombashtosh.What fallback version-tag fetching looks like
Fallback fetching of version tags is the "back-up" strategy that is part of the fix for #1690. It works both locally and on CI. Here's what it looks like on CI (I temporarily deleted all the version tags from my fork and reran the tip of this PR's branch):
(In case for any reason you want to see the version from an earlier commit and prior to multiple rebases that I had shown before in an earlier draft of this PR description, that's here.)
Fallback version-tag fetching would fail on old
gitAt least as currently written, it uses a feature that was introduced in Git 2.6.0.
I have posted a review comment on the specific code this affects, with full details.
Security implications of where
init-tests-after-clone.shmakesmasterCurrently, on
main,init-tests-after-clone.shcreates or switches to amasterbranch like this:git checkout master || git checkout -b masterOne of the changes in this PR is to ensure that
mastercannot be interpreted as a path--it has to be taken to be a branch, or at least a ref--which is mainly for better output to stderr when the branch doesn't exist, but also to safeguard against rare situations where a file or directory namedmasterexists:git checkout master -- || git checkout -b masterHowever, there is a simpler, more elegant, and more robust way to get a suitable
masterbranch for tests, that I think is more likely to work well most of the time [edit: though the answer to #1615 suggests a reason it might not]:masteris already getting reset back to__testing_point__, so the main disadvantage of-B, that one can lose one's branch if it is not pushed, applies already.I would like to do it that way (and I have a branch for it, ready to go). But it has security implications, specifically for people who review pull requests. As detailed in #1690, wherever
mastergets checked out, editors and IDEs may be fooled into running unreviewed untrusted code from there. Using the-Bway I want to use would be a mitigation for the situation described in #1690 (though that's not the main reason I want to do it). But it would be an exacerbation of it for a reviewer, because it would weaken the already brittle assurancegit diff main featurewould provide. Consider:There, a pull request proposes a useful feature on the
featurebranch. But the preceding three commits have evil code in them that, if an editor/IDE imports modules to do test discovery (or any other operation the editor reserves for "trusted folders"), will be run. The tip offeatureremoves the evil code, sogit diff main featuredoes not reveal it. But becausemasteris checked out at the tip offeatureeven ifmasteralready exists in the reviewer's local or remote repository, the script's reflog-building commands reset to each of the evil commits.This may not be a serious problem. When reviewing, one should already not rely merely on
git diff main feature, and CI in pull requests is set up to run on thepull-requesttrigger (which, unlikepull-request-targetwhich is dangerous if not used very carefully, runs with the fork's permissions, not allowing elevation). Furthermore, one may already not havemasterlocally or on a remote, in which case the existing checkout code already creates it atHEAD, so it's not like this is a new situation. Furthermore, it's generally unnecessary to run that script while reviewing, even if one opens up an only partially reviewed PR branch in an editor that may perform unsafe operations based on its contents.Nonetheless, I wanted to bring this up rather than just adding a commit to change it to
git checkout -B master.blackviapre-commit: I'm not sure what's best.It seems to me that there is actually just one thing that, in hindsight, would have been better to have developed separately. Adding
blacktopre-commit, and using that on CI, presents choices to be made while reviewing that are practically separate from the other changes.The core issue is that all the other tools run via
pre-commitonly perform checks, while the way most users ofblackandpre-commitwould expect and prefer they be used together is forpre-committo actually perform auto-formatting. This is actually no problem on CI; the GitHub Action being used accepts code changes from a hook as a check failure. But there should be a way to just lint locally, that includes checking forblack-conforming formatting and that does not change any files.Therefore, I have set up two hooks for
black: one that runs by default and formats, and another that does not run by default and that only performs checks. Thetoxlinting environment andlint.ymlCI workflow use the check-only hook and skip the formatting one. But there should also be a way to do this locally without building the project and setting uptoxenvironments and without typing in a complexpre-commitcommand. So I have mademake lintdo that.This is all documented as part of the readme improvements. It is inelegant, though, because everything else done by makefiles in this project is directly related to building. There are a few options, which I present in descending order of my preference, but they are all things I think are reasonable:
make lintmay go away. (I think this is not really necessary, because the development instructions are not implied to be stable. But I am not sure.)pre-committo have only the check-onlyblackhook, at least for now, and modify the readme accordingly. Then there is no need formake lintbecausepre-commit run --all-filesjust lints, as it did before, but includingblack. This has the disadvantage that people who likeblackandpre-commitprobably prefer that they facilitate auto-formatting, but it is otherwise elegant. I suspect you might prefer this approach, therefore I have made a commit for it on mysh-nofmtbranch, which can be easily fast-forward merged into here (or opened as a separate PR if the change is desired after merging this).blackfrom.pre-commit-config.ymlon this branch, modifying the readme accordingly, and propose it separately. It could be removed by rebasing, or just by adding a commit to remove it. This is more work, but actually a bigger reason this is not my preferred approach is the same reason I had addedblacktopre-commitin the first place: with everything else being done bypre-commit, and with this project being inblackstyle (aside from this project's very long lines), it is unintuitive and unexpected that it would not, in any form, be usable viapre-commit.