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

Adjusted hook tests to work on windows #236

Merged
merged 14 commits into from
Aug 18, 2020

Conversation

dr-BEat
Copy link
Contributor

@dr-BEat dr-BEat commented Aug 16, 2020

This PR fixes #235.
The extra newline at the beginning of the hook caused the following error on windows:
error: cannot spawn .git/hooks/commit-msg: No such file or directory

@dr-BEat
Copy link
Contributor Author

dr-BEat commented Aug 16, 2020

Apparently my local setup is different from the CI system.
It still returns the same error: Windows Subsystem for Linux has no installed distributions.\r\r\nDistributions can be installed by visiting the Microsoft Store:\r\r\nhttps://aka.ms/wslstore\r\r\n

@extrawurst
Copy link
Owner

@dr-BEat thanks for looking into it. Is the wsl default installed on every Windows nowadays? Or do we have to simulate it by using a vanilla windows without wsl?

@dr-BEat
Copy link
Contributor Author

dr-BEat commented Aug 16, 2020

@extrawurst no problem. Its not installed by default, however I think they added a bash.exe by default that gives the error above.
Usually any git installation on Windows brings a real bash, so it works out normally.
The run_hook function in hooks.rs tries to call bash directly and it gets the fake Windows one.

@extrawurst
Copy link
Owner

Any idea how to fix this? Maybe just install some git using chocolaty in the ci?

@dr-BEat
Copy link
Contributor Author

dr-BEat commented Aug 16, 2020

Yeah installing git might fix it.

@dr-BEat
Copy link
Contributor Author

dr-BEat commented Aug 16, 2020

@dr-BEat
Copy link
Contributor Author

dr-BEat commented Aug 16, 2020

According to this blog by Jon Skeet https://codeblog.jonskeet.uk/2019/10/12/using-git-bash-from-appveyor/ the order of directories in the Path environment can break stuff.
I tried to add the git path as the first entry, but it does not seem to work.

@extrawurst
Copy link
Owner

@dr-BEat any more ideas?

@dr-BEat
Copy link
Contributor Author

dr-BEat commented Aug 18, 2020

@extrawurst nothing concrete but I will try some tricks, probably the path is not correct still

@dr-BEat
Copy link
Contributor Author

dr-BEat commented Aug 18, 2020

@extrawurst the path environment is correct, the problem is that Command::new("bash") in hooks.rs line 105 somehow does not follow normal windows rules in finding which bash to execute.
If I hardcode the full path to the git bash with Command::new(r"C:\Program Files\Git\bin\bash.exe") it works perfectly.

@extrawurst
Copy link
Owner

@dr-BEat interesting. maybe because we run the cargo test in the git shell, did you try setting the shell: cmd in the ci to make him use the windows stuff?

@dr-BEat
Copy link
Contributor Author

dr-BEat commented Aug 18, 2020

@extrawurst I do not think its related to the shell. I can reproduce the issue locally when just running cargo test from Powershell

@extrawurst
Copy link
Owner

@dr-BEat nice a repro is what we needed!!🥳 I wish I could add any windows knowledge whatsoever :(

@dr-BEat
Copy link
Contributor Author

dr-BEat commented Aug 18, 2020

@extrawurst found it! Calling env on the command fixed it. This is probably related to the discussion here rust-lang/rust#37519. The behaviour of Command::spawn() changes because the environment is passed differently to Windows if env is called.

@extrawurst
Copy link
Owner

OMFG!!! you are the man!!

@@ -105,6 +105,7 @@ fn run_hook(
let output = Command::new("bash")
.args(bash_args)
.current_dir(path)
.env("DUMMYENV", "FixPathHandlingOnWindows") // This call forces Command to handle the Path environment correctly on windows, the specific env set here does not matter
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wtf... this is crazy! 🙈

@extrawurst extrawurst merged commit 14cfb39 into extrawurst:master Aug 18, 2020
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.

Windows commit hooks without wsl
2 participants