Add powershell script equivalents of test scripts and add windows to the test matrix for the test pipeline

[Kethku]

I think there are some issues with windows that might be catchable via automated testing. This PR takes a stab at adding windows to the test pipeline so that we can avoid these issues in the future

[Kethku]

@Olical I don't have permissions to run the test pipeline, or maybe its not setup to run on PRs? When running locally on my windows machine using the instructions in the readme I get some interesting failures. I will work on addressing them now

[Kethku]

Ok I'm running into test failures largely because the .nfnl.fnl file is not trusted. Is the trust system not an issue on linux builds?

[Kethku]

edit: Nevermind, these are all explained by incorrect os assumptions in the expected values

[Kethku]

Ok I fixed the fs_spec issues and moved on to config_spec tests. I think there are some real issues here. At this point me stumbling around has hit diminishing returns. Is there a way to get this into the pipeline so that we can work on the remaining issues together? I think they are real failures now

[Olical]

As far as trust goes, I think in my scripts that run the tests I have a line that adds trust for the file in a headless way. Maybe you worked that out but thought I'd mention it.

[Olical]

Nice! Thank you for catching all of these places. Does it work locally for you now then?

If so, I'll probably make the changes I suggested about porting over Aniseed and Conjure's FS / path related code that abstracts paths away into something OS agnostic. Then we can have one test that supports all OS paths.

If you want to port that code over into nfnl.fs that's cool, if not I'll get to it as well. As long as you have a branch that works for you right now I'm happy and I'll add my changes on top and get it merged ASAP, hopefully this weekend since I finally have some time at home (it's been weeks!).

Thank you again, this is really appreciate, I don't do my due diligence with Windows support and it's not fair. Your input is a huge help to me and everyone else who wants to use this on Windows. I'll be happy to merge this soon, after one of use does a little refactoring of the ideas and abstracting of paths is all.

I'm so glad I don't have to learn to write powershell too! That might take me a while!

[Olical]

Yep, here's the trust line, I guess you found it and worked that part out, nice!

[Kethku]

I think this isn't working for some reason... I'll look into it a bit more

[Kethku]

Weirdly. The test action and local runs of the tests still have messages about not trusting the file. So I think something is missing here on windows for some reason.

[Kethku]

If so, I'll probably make the changes I suggested about porting over Aniseed and Conjure's FS / path related code that abstracts paths away into something OS agnostic. Then we can have one test that supports all OS paths.

If you want to port that code over into nfnl.fs that's cool, if not I'll get to it as well. As long as you have a branch that works for you right now I'm happy and I'll add my changes on top and get it merged ASAP, hopefully this weekend since I finally have some time at home (it's been weeks!).

It does not work yet. I'm suspicious that some of the path changes are causing the issues I encountered with finding the .nfnl.fnl file properly. I've got some time now so I'll take a stab at the changes you outlined in aniseed and conjure.

I did see your automated line for trusting, but for some reason I still get messages about not trusting in the logs still. I just noticed a typo in the runner line which is causing the actions to fail, so I'll fix that and link the exact issue.

[Kethku]

Ok I've got the tests running. So at least now you can see the actual failures: https://github.com/Kethku/nfnl/actions/runs/9618182719/job/26531523272#step:5:49

I swapped to the neovim installer action that we use for Neovide which works well cross platform.

[Kethku]

Ok I've tracked all this down to what I believe is a bug in the secure read code in neovim. If I add print(contents:len()) to https://github.com/neovim/neovim/blob/d82efeccc7e89230ba6673da0fdf62c09fa38c17/runtime/lua/vim/secure.lua#L73 and https://github.com/neovim/neovim/blob/d82efeccc7e89230ba6673da0fdf62c09fa38c17/runtime/lua/vim/secure.lua#L182

and then also print out a table comparing the trusted hash and the expected hash when doing the secure read, and then run the tests I get this extremely suspicious line in the test output:

4
3
{
  hash = "ca3d163bab055381827226140568f3bef7eaac187cebd76878e0b63e9e442356",
  trusted_hash = "e3566b3a06430868d71e9287dfd6c6c520a3da027aabea01951d407ee131dc2f"
}

I think what might be going on is that the code to create the contents which get hashed on the read side: https://github.com/neovim/neovim/blob/d82efeccc7e89230ba6673da0fdf62c09fa38c17/runtime/lua/vim/secure.lua#L64-L72
is different from the code that gets the contents to get hashed on the write side:
https://github.com/neovim/neovim/blob/d82efeccc7e89230ba6673da0fdf62c09fa38c17/runtime/lua/vim/secure.lua#L176-L181

Given that the length is different by only one byte and the write side is doing some new line check by buffer format I guess, it seems like somehow either the buffer format isn't getting set correctly or the file doesn't have the expected newline or some similar issue.

I'm pretty dang confident at this point that the issue is upstream, but I'd like your opinion or thoughts before I report it in the neovim repo. Maybe we could write a failing test on their side to repro the issue? Idk

[Kethku]

Encouragingly, if we can figure out how to get the trusted read working I'm guessing that all of these issues will be resolved. I believe this because the config file that was written by you into the repository (and which has unix newlines rather than the silly windows ones) works perfectly. I can edit files in the repo, save them and auto compile just fine. Its only files that were created on my machine that run into this issue.

So if we can fix this upstream the problem should be solved for good

[Kethku]

The tests for this code are here: https://github.com/neovim/neovim/blob/master/test/functional/lua/secure_spec.lua but on a quick glance, I don't really understand them. I'm getting a bit tired, so I'm gonna pause work on this for now, but I think this is the next step if you're up for it, or I'll get back to it sometime soonish:

Add a test to secure_spec.lua which demonstrates adding a file with non windows newlines to the trust database and then being unable to trust said file afterwards due to the newline mismatch. If we can write that test, I expect the issue will get resolved quickly

[Kethku]

@Olical I think this line should get added to the other test script so that the trust db is isolated from the user's config

[Olical]

Done, thanks although I put mine in .test-config/state just to keep it all in one directory that can be ignored or deleted when required.

I can update this path though, not a problem.

[Olical]

Does this one support Windows when the other didn't or something like that?

[Kethku]

Yes. This supports windows when the previous one does not

[Olical]

Done, thanks although I put mine in .test-config/state just to keep it all in one directory that can be ignored or deleted when required.

I can update this path though, not a problem.

[Olical]

So gsub is actually doing pattern based replacement, not whole string replacement. This will cause issues if someone uses Lua pattern syntax without realising.

I don't think this needs to be wrapped in a function here and I'm totally happy for the rest of the code to call into string.gsub directly, no need for the indirection imo.

[Kethku]

Yeah duh. My bad

[Olical]

Good idea, quite a common function to write repeatedly.

[Olical]

I'm not sure of the right answer yet, but something about how these two functions work makes me think there will be issues 🤔 also I didn't think string.gsub mutated the original string? So only the second str.replace actually does things here right?

My thinking is along the lines of "you can have a \ inside a unix path", and I guess you can have a / in a Windows path. So we want to make sure those stay intact when messing with paths. The Conjure code for this probably falls into the same trap, I'll have to check.

[Kethku]

Ugh. This is just my not being so familiar with fennel. Sounds like its better to use vim.fs.normalize than my changes

[Olical]

I've got your branch locally and I'm taking a look + will push it so we can run the CI.

[Olical]

Oh! :h vim.fs.normalize()! That's what we want for always converting paths to / no matter what. Going the other way then is a little easier because we KNOW we're going from / to \. So I think we pass paths through normalize before we work with them then do a whole "backslashify" when absolutely required.

[Olical]

Oh and one more note that you might find useful!

			*'shellslash'* *'ssl'* *'noshellslash'* *'nossl'*
'shellslash' 'ssl'	boolean	(default off)
			global
			only for MS-Windows
	When set, a forward slash is used when expanding file names.  This is
	useful when a Unix-like shell is used instead of cmd.exe.  Backward
	slashes can still be typed, but they are changed to forward slashes by
	Vim.
	Note that setting or resetting this option has no effect for some
	existing file names, thus this option needs to be set before opening
	any file for best results.  This might change in the future.
	'shellslash' only works when a backslash can be used as a path
	separator.  To test if this is so use: >vim
		if exists('+shellslash')
<	Also see 'completeslash'.

[Olical]

I have some local changes that basically perform the path normalisation as we glob for paths, so ALL paths in memory should be /, then as we go to use those paths to do things Windows specific (like write to a file) we can "localize" that specific path as late as possible to avoid littering the code with conversion calls. Still working on it though!

[Olical]

Pushed my current work to windows-paths branch in this repo. I have everything working on Linux again and will now use your work to get the suite running in CI on Windows to work out the Windows side. I still need to add some more tests and I don't think this will work on Windows right now but my general idea is coming together:

• Anywhere a path enters the system (getting the cwd, globbing paths) we normalize those paths to all be / since that's what Neovim as a whole seems to mostly use and normalize towards, regardless of OS.
• At the few points where we NEED \ we can localize those paths to the OS specific paths, but we do it as late as possible.

I want to avoid mixed slashes in the project, I want it to ALL be one style so everything is fairly easy to reason about and at the boundaries we think about what the OS cares about. I think that'll be much easier to maintain and harder to break going forward. I have avoided rebasing your commits and have tried to merge to make sure shas stay stable between our efforts.

[Olical]

WOAH! The tests actually ran under Windows in CI! They failed, but still, I can work with that!

Thank you for getting that working, it's amazing stuff and super helpful for the future. Not only will this help with preventing regressions in nfnl on Windows in the future (when we fix it...) but I can copy this format to other repos easily now I have a working example! Thank you!

Oh and you're probably onto something with the bug in the secure read, I just haven't got to that bit yet.

[Olical]

So I'm pretty happy with the windows-tests branch right now. It exhibits the behaviour you describe around trust files, on Linux mine outputs the following into .test-config/state/nvim/trust

ca3d163bab055381827226140568f3bef7eaac187cebd76878e0b63e9e442356 /tmp/nvim.olical/D8hm4P/0/.nfnl.fnl
b8d37f0eae599447a076553deabddf848a034cae33938bfc7382e17b7d5959ab /home/olical/repos/Olical/nfnl/.nfnl.fnl

I guess the paths are part of that hash so we can't compare them between each other 🤔 so how do we go about proving this is a bug in Neovim (or not). Does it mean you basically can't use secure read in ANY way on Windows? Unrelated to nfnl entirely? Because if so that should be a fairly quick and small repro I hope?

[Kethku]

take a look at this comment again #42 (comment)

its extremely possible im missing something stupid here but to me this means the actual content is different. the file name isnt used in the hash so the difference has to be something to do with the newlines being different lengths. to me this is a straightforward bug in their lua code that i linked. if we are comparing file contents before and after, we should read the file the same exact way both times. but thats not what they do.

in one way they read the file and hash it directly. the other, they read the buffer and try to recreate the file contents by joining the lines with newlines. i dont understand how it fails but im pretty sure the failure is in the newlines selection. i think it might assume that all files on windows systems use carriage return + new lines which is for sure not always the case.

so in summary, the failure only shows up when you have a mismatch of newline kinds with the expected newline on the os. i thiiiink 😅

[Olical]

OH! OH WOW! Yeah that sounds plausible! It's maybe \r\n on Windows and \n on Unixy systems. It'd be interesting to see if a file with 10 new lines is 10 bytes off. Amazing find!

…

On Sat, 22 Jun 2024, 22:21 Kaylee Simmons, ***@***.***> wrote: take a look at this comment again #42 (comment) <#42 (comment)> its extremely possible im missing something stupid here but to me this means the actual content is different. the file name isnt used in the hash so the difference has to be something to do with the newlines being different lengths. to me this is a straightforward bug in their lua code that i linked. if we are comparing file contents before and after, we should read the file the same exact way both times. but thats not what they do. in one way they read the file and hash it directly. the other, they read the buffer and try to recreate the file contents by joining the lines with newlines. i dont understand how it fails but im pretty sure the failure is in the newlines selection. i think it might assume that all files on windows systems use carriage return + new lines which is for sure not always the case. so in summary, the failure only shows up when you have a mismatch of newline kinds with the expected newline on the os. i thiiiink 😅 — Reply to this email directly, view it on GitHub <#42 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACM6XNVWDVMZRBEKD6Y5LLZIXTDXAVCNFSM6AAAAABJNAACISVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOBUGE4TGMRYG4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[Olical]

So I did some research into the environment when the CI tests run. Looks like autoclrf is ON in the Windows git, which is expected. So all \n will be converted to \r\n on checkout (in theory unless something else is up).

I've also checked some Neovim settings after .nfnl.fnl is opened, looks like the buffer is not modified, fixendofline is enabled and fileformat is dos. So my theory of Neovim going into a modified state every time we open a files doesn't really hold.

fixendofline should only apply on write, but I wonder if some passive thing is modifying the buffer as it loads in :edit to give it a different sha to on disk.

I'm trying to find some config I can set that keeps the line endings and file ending consistent on disk and in memory for Neovim on Windows without breaking anything else. All of my attempts are against this branch which is rebased on main if you'd like to try anything as well: https://github.com/Olical/nfnl/tree/windows-tests

One thought I have is to use editor config + autocrlf=false in git so we keep the files as they are on Unix at all times. Basically try to keep the files byte for byte identical between Windows and Unix I guess other than the ps1 scripts which I think NEED to be CRLF encoded? Which if they are encoded like dos in git it means I can set a flag in git on the CI host to keep those as CLRF on checkout but keep every other file as unixy which might be good.

[Olical]

I added a special env var that disables the use of secure read and trust calls in the Windows tests. That gets us a little further but now the error is that the Lua output isn't being written!

My test code tries to check for the C:/Users/RUNNER~1/AppData/Local/Temp/nvim.0/Qu4ICF/0/lua directory which should be written to after writing to a .fnl file but it isn't there. So either that path is wrong and should be something else on Windows or maybe it's an async thing that only manifests on Windows?

If you can run the tests now on your own Windows machine and see if you see the same thing that'd be amazing. Then maybe we can inspect the file system and see if that path is wrong. Maybe it needs to be backslashes for this particular isdirectory call?

I'll leave it here for today though, another afternoon lost to Windows things 😭 I'm so bad at getting anything working on this OS.