The sound distributed version control system

#234 Hooks on Windows

Opened by Skia on December 21, 2020 ReviewingBeta
Skia on December 21, 2020

Currently, when pijul runs hooks set up in .pijul/config, it runs bash to execute the hook commands.

It is only possible to rely on bash on Windows if it is installed along side pijul and that instance of bash is used.

While it might be possible to use one of cmd or powershell on Windows (or have a configured shell in the global ~/.config/pijul/config.toml), I have one other possible direction to suggest.

It would also be possible to call the commands directly without a shell. One down side is that hooks that run shell script files rather than executables would be more verbose to configure (though it would be the same as running a different shell that the one pijul uses). Also with the way hooks are currently configured it would not be possible to set up arguments for them.

So the configuration for hooks would need to be changed if pijul is to run commands directly. Example of current hook configuration:

[hooks]
record = [ "cargo fmt" ]

Example rewritten to be suitable for pijul to run hook commands directly:

[hooks]
record = [ { command = "cargo", args = [ "fmt" ] } ]

Which is more verbose to write. But it makes it easy to make the code for running hooks portable. It would also allow for some future additions such as being able to configure environment variables for a hook command.

It would also be possible to not change the config if pijul could parse the command string, but that is quite a bit more work unless there is a good crate for doing that kind of thing.

Skia added a change on December 22, 2020
ETVYFS4YF4KPUGJ2AFIVMHKL2ZNEPQYHGOW45HNR5ZJPAQIPG6GQC
Skia added a change on December 22, 2020
OTA33L6RLEFKRJBFMCN5FOX5LYFHGM6FK2PIL3YKVWFJYLIFAMJAC
Skia on December 22, 2020

The first change implements the change to have pijul run hooks directly rather than through bash. It also includes the relevant changes to ./.pijul/config to support the change as I outlined in my initial post.

The second change adds an optional label field to hooks. If present, the ‘label’ will be used in the error message about the hook failing. If not present, the command of the hook is used instead.

So given a config for hooks of:

record = [ { label = "format", command = "cargo", args = [ "fmt" ] } ]

If the hook fails the error message is: Hook "format" exited with code ExitStatus(ExitStatus(1))

Skia added a change on December 22, 2020
JU6IAXZJRGANRUVLLDRWS3XGSTP47UIHRKOPQHPXSHTAZD3WHIYQC
Skia on December 22, 2020

Realized that the command field should not default to an empty string when not present. The error message is still not great, but it is better than the OS error from trying to pass an empty string to std::process::Command::new().

Skia added a change on December 23, 2020
ZOLH5DSPWMU2PBBUGUPDET6GAU7IOLZED2RBBDJPPCYXZPDYKJGQC
Skia on December 23, 2020

I made squashed version (ZOLH5DSPWMU2PBBUGUPDET6GAU7IOLZED2RBBDJPPCYXZPDYKJGQC) of the previous changes.

pmeunier on December 27, 2020

Hi! Thanks for your change. This looks good, but would need to be Windows-specific, as I believe that many Unix users don’t always realise what is bash syntax (~, *, among others) and what is actually sent to commands. That said, I also understand that having a different syntax for Unix and Windows might lead to highly confusing answers on StackOverflow.

Do you know what happens if one uses environment variables or wildcards (*) in hooks on Windows?

Skia on December 30, 2020

@pmeunier

Neither of the Windows shells (cmd and powershell) support file globbing. They both have different syntax for environment variables from each other and from Unix shells. ‘cmd’ does environment variables like %PATH%. ‘powershell’ does them like $ENV:PATH. The latter is why I was having trouble with getting Pijul’s logging to work. In powershell, $RUST_LOG="pijul=debug"' sets a script variable called 'RUST_LOG', but it needs to be $ENV:RUST_LOG=“pijul=debug”` to set the environment variable other programs (like Pijul) can access.

This change was a first step as I wanted to talk about it before going further. The next step is allowing shells to be configured in the global configuration file, and the hooks can use those shells. Also it could be set up so users can configure a default shell to use in the global and or per repository configuration file(s).

Have shells be explicit helps make it clear that the proper way to write hooks will depend on the shell being used.

As to people being unclear about what Unix shells are responsible for, in the manual there should be a special note that for hooks that use file globbing should be set to use a shell such as Bash. Such a note would be good in both the ‘Getting Started’ (suggesting Unix users set up their usual shell and make it the default) and in the documentation for hooks.

pmeunier added a change on January 7, 2021
HCH4BFPMUCK2KLMI5RRRBAM6TWXZ5HUVXF7KOXWFOZEX5745VZGAC
pmeunier on January 7, 2021

Hi! Actually, the first example in the documentation for Command shows how to do this!

What do you think of the patch I just pushed?

pmeunier added tag Reviewing on January 7, 2021
pmeunier added tag Beta on January 12, 2021
Skia on January 12, 2021

@pmeunier

I have not tried it, but it should work as a fix.. It will be tomorrow when I get a chance to do so.

pmeunier on January 12, 2021

No rush, there are plenty of things to fix before we move to beta.

Skia on January 13, 2021

@pmeunier

It works.

Here is a thought, it would be possible to allow to have an optional entry in to config to set a shell to use on a per repository basis.

Something like:

[shell]
exec = "/bin/zsh"
flag = "-c"

and on Windows to use PowerShell instead of cmd

[shell]
exec = "powershell.exe" //or `exec = "C:\\Program Files\\PowerShell\\7\\pwsh.exe"` to use PowerShell 7 instead of the PowerShell 5 that comes with Windows 10
flag = "-Command"

If not present, then pijul would default to what you have implemented in the change.

pmeunier on January 13, 2021

Good point: we might want to use a different shell sometimes.

That may complicate the configuration file though, what would you think about a slightly more general solution where we can add an option to bypass the shell entirely for some hooks, like:

[hooks]
record = [ "cargo fmt", { command = "/bin/zsh", args = [ "-c", "cargo fmt" ] } ]
pmeunier added a change on January 13, 2021
VL7ZYKHBPKLNY5SA5QBW56SJ7LBBCKCGV5UAYLVF75KY6PPBOD4AC
main
Skia on January 13, 2021

@pmeunier

Adding the ability to configure an alternative shell to use in a repository is something that can be waited on to see if there is enough demand for it.

Being able to bypass the shell would likely cover most cases where being able to set an alternative would be wanted.