facebookincubator / fastmod

A fast partial replacement for the codemod tool
Apache License 2.0
1.66k stars 41 forks source link

Windows support? #1

Closed bbatha closed 6 years ago

bbatha commented 6 years ago

The documentation states:

fastmod is supported on macOS and Linux.

Is there a reason for this support policy? I tested out fastmod on windows and it seems to work fine. I also skimmed the code base for any unixisms and the only one I could find was assuming $EDITOR was set/defaulting to vim.

swolchok commented 6 years ago

I'm glad it works fine! The reason is just that nobody had asked for Windows support yet (congratulations on being the first) and I personally don't use Windows for work. I'd be happy to consider pull requests fixing Windows-specific issues and to consider Windows "supported" if there is enough demand.

Regarding the issue with $EDITOR, I'm not sure what the equivalent Windows convention is. Can you fill me in on that?

The only other question I have is whether the colored output via the term crate works OK on Windows. Does it?

ErichDonGubler commented 6 years ago

@swolchok: Windows HAS no $EDITOR convention -- the vast majority of workflow in Windows assumes the presence of its baked-in graphical toolchain and file associations with your favorite editor a la Gnome's "Default Applications". This would probably be pretty easy to handle by either forcing users to have $EDITOR defined or defaulting to notepad.exe if the variable is not set.

term doesn't support colored output as portably as term_color, which ripgrep and cargo actually use so that even environments like MSYS2 work. term also notably has a memory leak on Windows right now. The only other TUI functionality I see fastmod depend on is clearing the screen, which AFAIK there's not an awesome library to support it yet.

swolchok commented 6 years ago

The vast majority of workflow in Windows assumes the presence of its baked-in graphical toolchain and file associations with your favorite editor a la Gnome's "Default Applications"

Would defaulting to start be a reasonable choice?

term also notably has a memory leak on Windows right now.

IMO the correct solution to this is to fix term. I'd be interested in doing that eventually, perhaps on my personal Windows machine, but my personal life is quite busy right now. I might be able to take a look at it in a few weeks.

clearing the screen, which AFAIK there's not an awesome library to support it yet

I couldn't find a Rust terminal library I was completely thrilled with for purposes of porting codemod's terminal module. codemod just wants you to support ncurses and get out of its way, and the fancier terminals really get in the way of that. If there's a terminal crate that I missed that supports all the operations we need itself, I wouldn't mind switching; I just don't want to end up with a patchwork of crates in order to add Windows support.

ErichDonGubler commented 6 years ago

Would defaulting to start be a reasonable choice?

Absolutely. :) Wish I'd thought of that first!

I just don't want to end up with a patchwork of crates in order to add Windows support.

I don't know of a cross-platform one that supports clearing the screen yet... sigh. :\ Which is not to say that one isn't out there for sure -- but there's not a popular one AFAIK.

bbatha commented 6 years ago

The only other question I have is whether the colored output via the term crate works OK on Windows. Does it?

I can't speak to the general quality but I get red and green text in conhost terminals (cmd/powershell) on Windows 7.

swolchok commented 6 years ago

Just to give an update, I think we should probably try $EDITOR then vim then notepad then start, but I want to see if that will work well for all of 1) vanilla cmd.exe 2) Powershell 3) Windows Subsystem for Linux. The other issue with start is that it's presumably not going to block until the edit is done, and that probably won't result in a good user experience. I'm not sure if that's going to be an issue with notepad as well.

ErichDonGubler commented 6 years ago

notepad blocks, but start does not unless one uses start "" /B /W <command...>. However, many editor executables in Windows are stubs that kick off async processes...which unfortunately defeats even a blocking start. 😕

On another note, notepad should ALWAYS be present on Windows machines, so I don't think designing a fallback for it is valid.

swolchok commented 6 years ago

I plan to update the README to note that, while Windows is not a primary use case, the tool largely works and non-invasive PRs for Windows-specific bugs will be considered. I would prefer to keep winapi out of fastmod itself, though. After the README update lands, I'll close this issue and we can open specific issues as needed. For example, I plan to open a separate issue to note that clearing the screen currently does not work on Windows conhost terminals.

swolchok commented 6 years ago

I also forgot to mention that 2502ae105450412be0fa8b28410df5368072d18e landed yesterday. It makes fastmod fall back to notepad.exe on Windows if EDITOR and vim are not working.

swolchok commented 6 years ago

768e8cf02a0a69b3385f2de6c013bf4611022389 has landed.