Skip to content

Conversation

@tobil4sk
Copy link
Member

OpenSSL is preinstalled on the windows runner, so we can use that. Unlike the previous solution, this version sets up luarocks for visual studio binaries instead of mingw, which should be a bit simpler to set up. Still could have some improvements for local execution.

We now get past dependency setup, but some unit tests fail:

unit.spec.sys.net.TestSocket
  test: ERROR E
    bin/unit.lua:16723: bad argument #1 to 'find' (string expected, got nil)
unit.issues.Issue9757
  test: FAILURE F
    line: 11, expected true

Fixes #6927
Closes #10919 (duplicate of above)
Closes #10917 (alternative PR)

@tobil4sk
Copy link
Member Author

The 9757 failure is more general than the original issue, even this fails:

function main() {
	trace(Sys.systemName());
}

It tries to run uname which doesn't exist on windows:

os = lua.Io.popen('uname -s', 'r').read('*l').toLowerCase();

And this is a minimal sample to reproduce the Socket error:

function main() {
	var s = new sys.net.Socket();
	var host = new sys.net.Host("127.0.0.1");
	s.bind(host, 0);
	s.listen(1);
	trace(s.host());
}

It seems after the listen call, the host information isn't available anymore so s.getsockname().address is nil and that causes a crash.

The LuaSocket connect and bind methods are too high level. They also
change the type of the object which previously required reconstructing a
new TcpMaster object on certain method calls (e.g. listen()).

We can avoid this by creating a generic TcpMaster object and using the
lower level methods on it. This way it is still possible to bind and
listen separately on windows which avoids a test failure.

See luasocket documentation:
https://lunarmodules.github.io/luasocket/tcp.html#listen
https://lunarmodules.github.io/luasocket/socket.html#bind
This ended up running uname on windows, which doesn't work well on
systems where it isn't available.

We can instead check the system path separator and assume Windows if the
separator is "\".
Since we prepare the arguments for parsing by cmd.exe, we need to skip
CreateProcess handling otherwise the escape characters get messed up.
The 'verbatim' option takes care of this.

See luv docs:
https://github.com/luvit/luv/blob/master/docs/docs.md#uvspawnpath-options-on_exit
The copy command does not work properly with /, we need to translate it
to \.

Also, now quoting is performed manually as SysTools.quoteWinArg doesn't
quote if the argument contains a ",", which needs to be quoted as it has
a special meaning for the windows "copy" command.
This reverts commit 7727f98.

This causes more harm than good, as it breaks program paths containing
spaces or other unusual characters.
Other haxe targets do not pass through the shell when args are given,
and this makes it easier to properly handle weird characters in the
program path or arguments, as we can let luv handle that.

For reference, here is hxcpp's implementation, which only uses the shell
if args are null:
https://github.com/HaxeFoundation/hxcpp/blob/0d23f857c88a14ecabb946d4ae879a45a48b14ca/src/hx/libs/std/Process.cpp#L208
read and write were fixed in: 5b3641b

but append was ommited.
Os.rename and Os.remove don't work with unicode paths on most systems,
and we are using luv for most operations here anyway. Vanilla versions
can still be used with `-D lua-vanilla`
Unlike the shell command, this will work properly with unicode paths on
windows.

If lua_vanilla is specified, fall back to the previous version.
@tobil4sk
Copy link
Member Author

The remaining unicode filesystem failures are down to the age old issue of windows ansi code pages (neko also suffers from this, and so does hxcpp for printing to stdout). Unfortunately, the builtin lua Io.open method uses c fopen that operates with the active code page on windows, which on most systems doesn't support the full unicode range.

These are the options we could take:

External workarounds at the distributor/user level (not very practical):

None of these seem like great options unfortunately, but unicode is and has always been a pain on windows so this is not really our fault.

I think for now I will add a define flag to set lua.Os.setlocale(".UTF8", Ctype); that users can opt into and we can use in the tests, which will get the tests passing finally. We will just have to document this for now.

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.

[CI] Fix lua tests on windows Get Lua tests working on windows

1 participant