Skip to content

Draft Windows support (fix for #594 and related)#630

Draft
pjeby wants to merge 2 commits intodirenv:masterfrom
pjeby:windows-fixes
Draft

Draft Windows support (fix for #594 and related)#630
pjeby wants to merge 2 commits intodirenv:masterfrom
pjeby:windows-fixes

Conversation

@pjeby
Copy link
Copy Markdown
Contributor

@pjeby pjeby commented May 6, 2020

The PATH corruption issue is fixed by adding an extra command line argument, UNIX_PATH, to direnv export, direnv dump, and stdlib's __main__:

If it is non-empty and different from the value of $PATH, it is used in place of $PATH for purposes of generating the exported environment. Shells supply this argument from their hook function to bypass the conversion between Windows and Unix-style paths that takes place when changing between shells and Go programs on Windows, thereby preserving symlinks, case, etc. that would otherwise be lost in translation.

Internally, a DIRENV_PLATFORM_PATH key is added to the logical environment processed by direnv export, direnv dump, and direnv exec, and its changes are tracked in DIRENV_DIFF, but it is never actually exposed as an environment variable to parent or child processes, nor is it ever read from the OS environment.

(It's tracked in DIRENV_DIFF so that direnv exec can know the previous OS path, though at the moment that's not very useful because syscall.Exec doesn't work on Windows and I don't know what a proper replacement would be. So for the moment, direnv exec still doesn't work on Windows.)

pjeby added 2 commits May 6, 2020 14:31
* Fix for direnv#594 (write error to fd 3 on Windows)
* Fix wrong SelfPath for 'direnv hook' on Windows
* Fix `$PATH` corruption across process boundaries

The PATH corruption issue is fixed by adding an extra command line
argument, `UNIX_PATH`, to `direnv export`, `direnv dump`, and
stdlib's `__main__`:

If it is non-empty and *different* from the value of `$PATH`, it is
used in place of `$PATH` for purposes of generating the exported
environment.  Shells supply this argument from their hook function
to bypass the conversion between Windows and Unix-style paths that
takes place when changing between shells and Go programs on Windows,
thereby preserving symlinks, case, etc. that would otherwise be lost
in translation.

Internally, a DIRENV_PLATFORM_PATH key is added to the logical
environment processed by `direnv export`, `direnv dump`, and
`direnv exec`, and its changes are tracked in DIRENV_DIFF, but
it is never actually exposed as an environment variable to parent
or child processes, nor is it ever read from the OS environment.

(It's tracked in DIRENV_DIFF so that `direnv exec` can know the
previous OS path, though at the moment that's not very useful
because syscall.Exec doesn't work on Windows and I don't know
what a proper replacement would be.  So for the moment, `direnv
exec` still doesn't work on Windows.)
@pjeby
Copy link
Copy Markdown
Contributor Author

pjeby commented May 6, 2020

FYI, I'm in the middle of trying to get the full test suite to actually run on Windows, some parts of which are harder than others. 😉

(One interesting note is that Windows cannot distinguish between empty and unset environment variables, so those tests would need to be skipped.)

Apart from the empty vs. unset tests, I actually have the bash tests successfully running on Cygwin in my current work directory, but there are a couple of hairy bits having to do with DIRENV_DUMP_FILE_PATH needing to be a Windows path but it being set from bash (and thus needing conversion). There are some hurdles still to go with getting this working on msys/Git bash, as well.

So, this is definitely not ready to merge just yet, but I thought I should inquire as to whether you think this is a viable approach or if you have any objections to the direction this is taking. Thanks!

Copy link
Copy Markdown
Member

@zimbatm zimbatm left a comment

Choose a reason for hiding this comment

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

Awesome. I am really happy to have somebody that cares about Windows support on the project.

Once you get the tests working let me know and I will add the GitHub Actions as well so that we can make sure things keep working in the future.

@pjeby
Copy link
Copy Markdown
Contributor Author

pjeby commented May 8, 2020

So, with some extra changes not shown here, I've gotten the tests to run for bash, fish, and zsh on Cygwin. (Don't have elvish or tcsh installed at the moment.)

So next, I tried it out on msys (git bash), only to find out (after much trial and tribulation), that git bash does this bullcrap.

Yeah that's right. When running any non-msys executable, it *silently translates command arguments based on guesswork as to whether something looks like a path! Which means it takes the extra "$PATH" argument and goes, "oh, it looks like you're trying to pass unix paths to a Windows program. Here, let me fix that for you..." BOOM. Headshot. There goes the entire functioning of this PR.

And it translates environment variables in this way as well.

So the good news is that this means that all msys/git bash needs to work is the stdlib dump piping fix I did here. (Using redirection instead of passing an fd.) Yay? (i.e. the actual fix for #594, and the addition of #632, are really the only thing stopping direnv working under msys/git bash.)

The bad news is that the rest of the work I've done for cygwin is precisely counterproductive for msys, because the matching path value throws this PR off and ends up with it thinking it's working with a pure windows shell like Elvish, and then having stuff break in the stdlib.

It is possible to force msys to not translate an argument by adding it to MSYS2_ARG_CONV_EXCL, but then that would become part of the direnv state while simultaneously preventing the variable from being controllable by direnv. So that's probably best left alone, I guess.

Hm, now that I think of it, probably lots of things can go badly with direnv on git bash/msys due to the conversion, because it also converts environment variables, and anything that looks like an absolute path is going to get mangled on the way back into the enclosing shell. And the only way around it is to pipe stuff in and out to bypass the mangling.

So, it's really more like, "msys/git bash is an easy fix if the mangling doesn't break anything you care about". Otherwise, it requires the full-on "direnv never trusts the OS environment as a diffing source or target on windows and instead expects to be piped variables from the calling shell" approach I laid out above.

Anyway, I need to give this some more thought, to see if there's any way to get a Windows version that doesn't suck for either kind of shell environment (msys or cygwin), and at the same time doesn't require a complete reworking of direnv internals. ☹️

@pjeby pjeby marked this pull request as draft May 8, 2020 06:15
@zimbatm
Copy link
Copy Markdown
Member

zimbatm commented May 8, 2020

I really admire your persistence :)

So next, I tried it out on msys (git bash), only to find out (after much trial and tribulation), that git bash does this bullcrap.

This is the kind of difference that led me to believe that the golang path package should take an encoding argument. Either at runtime or at compilation time.

Do you think it would help if direnv wasn't perceived as a Windows program? I get the sense that on Windows you have to treat cygwin, msys/git, ... each of those as essentially different environments. And have a version of direnv that is compiled specifically for that target.

Another related idea I had was to embed a statically-compiled version of Bash into direnv itself. This would allow to also patch bash to change its behavior.

@zimbatm
Copy link
Copy Markdown
Member

zimbatm commented May 8, 2020

Let's get the FD fix in as a first PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants