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

Update an archive and add some missing executable bits #1589

Merged
merged 2 commits into from
Sep 11, 2024

Conversation

EliahKagan
Copy link
Member

@EliahKagan EliahKagan commented Sep 10, 2024

One of the fixture scripts was modified in b12c7c9 (#1587) to fix typos, which changed its hash, but a regenerated archive was not committed at that time.

Other fixture scripts were recently introduced, and they have current archives committed, but they were not given executable bits, even though they are executable scripts that start with #! and the other fixture scripts have +x.

This small "maintenance" PR addresses those two things (one per commit).

I have verified that tests pass locally on Ubuntu 24.04 LTS, macOS 14.6, and Windows 10, both before and after this change, and that the make_dangling_symlink_to_windows_reserved archive, which previously had an uncommitted change on Ubuntu and macOS after running the tests, is no longer changed when the test suite is run. (It did not show as dirty on Windows because gix-testtools suppresses archive generation on Windows, but committing the regenerated script should help Windows tests to run slightly faster, too.)

In b12c7c9 (GitoxideLabs#1587), the `make_dangling_symlink_to_windows_reserved`
script was changed to fix typos. Though this did not alter its
effect, its hash changed, so the committed archive generated from
it could no longer be used. This commits a regenerated archive.
Some of the more recently introduced fixture scripts were not
marked executable, even though the others are.

Because `gix-testtools` has a fallback that runs scripts with
`bash` explictly when executing them directly fails, this did not
break any scripts, but the executable bit is also useful to
document which files are meant to be runnable scripts.

This marks them executable. Now all the shell scripts that begin
with a `#!` have the executable bit set. (The handful of `.sh`
files that are not meant to be run as scripts, but instead are
sourced by other scripts, do not start with `#!` and continue not
to have `+x` applied to them.)
@Byron
Copy link
Member

Byron commented Sep 11, 2024

Thanks a lot, much appreciated.

I wondered if there should be a CI job that tests invariants, maybe based on just check-invariants, which simply runs a script to verify a few things. We might already have that in the form of the script that validates that gix-packetline-blocking still matches gix-packetline.

@Byron Byron merged commit 7c2af44 into GitoxideLabs:main Sep 11, 2024
15 checks passed
@EliahKagan EliahKagan deleted the maintenance branch September 11, 2024 07:08
@EliahKagan
Copy link
Member Author

I don't know a good near-instantaneous way to test whether archives need to be regenerated without having just run the tests. This is one of the reasons I implemented #1590 the way I did.

For scripts, though, we can certainly check all non-ignored files to make sure that files that seem to be scripts (or, I think better, that seem to be plain text) start with #! if and only if they are marked executable. Conceptually, I want to say this is sort of like checking the relationship between gix-packetline and gix-packetline-blocking, except that all of the design considerations point in the other direction:

  • This should be only a check (instead of only a fix) because examination is required to determine if a file should gain an executable bit or lose its #!.
  • Everything, automated including building and using just, works even if it finds a problem, so there is no reason for it to be a shell script. So it should be a subcommand in tests/tools/main.rs.

@Byron
Copy link
Member

Byron commented Sep 11, 2024

I like the idea of implementing such functionality in Rust instead.

However, I'd hope to bring more of such 'internal' functionality into the internal-tools crate, which probably isn't cheap to build but when done, it can do a lot of work quickly. Maybe that could be a separate job to not prolong the runtime of anything else.

Maybe another argument could be made to say that tests/tools/main.rs should only contain tools that are useful to everyone, which means that the mess-in-the-middle sub-command is probably too specific and should rather go to internal-tools as well. However, having 'what-seems-like-shell-scripts' carry executable bits, seems like something useful to everyone.

Despite all of this, my disposition is to turn gix-testtools into a pure library, and move all new functionality to run within this repository into internal-tools, a crate the won't be published. After all, maybe the build times aren't that bad, I didn't check.

@EliahKagan
Copy link
Member Author

Checking that fixtures have correct executable bits seems like something relevant to most users of gix-testtools, assuming most users of gix-testtools use it to run fixture scripts. So maybe it should go in gix-testtools. But that doesn't require that it go in main.rs: it could be implemented in a newly introduced public function in lib.rs and the utility that calls it could be elsewhere: either as a command-line tool in internal-tools, or just as a test case for gitoxide itself, alongside the top-level integration tests.

@Byron
Copy link
Member

Byron commented Sep 11, 2024

That's fair - let's put it into gix-testtools main.rs then - for now I wouldn't feel comfortable adding an actual 'test' or 'check' to the library though. If there is a demand, that could change, of course.

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