Adapted cmdlineTests.sh for MinGW

[a3d4]

Here is what I did to make cmdlineTest.sh succeed, almost completely, on my Windows system.
Disclaimer: I'm not a bash expert :).

Checklist

• Code compiles correctly
• All tests are passing
• New tests have been created which fail without the change (if possible)
• README / documentation was extended, if necessary
• Changelog entry (if change is visible to the user)
• Used meaningful commit messages

[chriseth]

Can you explain a bit more? Should this prompt a change in solc or is it a "shortcoming" of mingw's shell?

[a3d4]

It is a MinGW feature. In some cases MinGW tries to convert Posix paths to Win32 paths. So applications receive patched command lines.

For instance, when called from my bash, ping.exe "/some/thing" outputs

Ping request could not find host C:/Program Files/Git/some/thing. Please check the name and try again.

If we add ;, MinGW thinks that the argument is a Windows path list and does not convert it.
ping.exe "/some/thing" outputs

Value must be supplied for option /some/thing;.

[ekpyron]

Will the rest of the script work with MSYS_NO_PATHCONV=1 set? That would be a nicer solution, but it might actually need the path conversion elsewhere...

[a3d4]

The first comment to https://stackoverflow.com/a/34386471 states:

the MSYS_NO_PATHCONV variable is not recognized by the upstream version of MSYS: not 1.0, not MSYS2, not 32 nor 64 bit variants

That's why I added ; (although I do not quite like it either).

Anyway, MSYS_NO_PATHCONV=1 works for me.

It breaks "$REPO_ROOT"/scripts/update_bugs_by_version.py with

python3.exe: can't open file '/d/..../scripts/update_bugs_by_version.py': [Errno 2] No such file or directory

but this can be easily fixed:

• by setting Windows-style REPO_ROOT or
• by setting/unsetting MSYS_NO_PATHCONV locally, before/after remapping tests.

[ekpyron]

Hm... if MSYS_NO_PATHCONV is not really supported, I'm not sure - also it's not that nice if we have to set and unset it... I haven't read up on it in detail, but do you think MSYS2_ARG_CONV_EXCL="*=*" would work and we could just globally set it in the beginning and keep it set?

[a3d4]

MSYS2_ARG_CONV_EXCL might be the best option indeed. Any of these lines at the beginning of the script make the remapping tests pass:

export MSYS2_ARG_CONV_EXCL="=/some/remapping"
export MSYS2_ARG_CONV_EXCL="=/"
export MSYS2_ARG_CONV_EXCL="="

BTW, the documentation does not seem to mention "*" option.

However, according to the discussion it should work. And here is an excerpt from the corresponding msys2 patch to cygwin's path.cc:

+  for (size_t excl = 0; excl < exclusions_count; ++excl)
+    {
+      /* Since we've got regex linked we should maybe switch to that, but
+         running regexes for every argument could be too slow. */
+      if ( strcmp (exclusions, "*") == 0 || (strlen (exclusions) && strstr (arg, exclusions) == arg) )
+        return (char*)arg;
+      exclusions += strlen (exclusions) + 1;
+    }

So * does not work as a wildcard character within a string, rather as a global turn-off-all-path-translations switch.

[ekpyron]

That's unfortunate...

[ekpyron]

Ah, ok, I just now realize that it's strstr in the code and you're saying that export MSYS2_ARG_CONV_EXCL="=" alone will be enough.
At least I would be fine with adding that one to the top of the file with a comment that it will prevent path conversions for remappings on msys2. We can put it in the MINGW*) case only.

[ekpyron]

@chriseth What do you think? I'd say that'd be a solution that's non-invasive enough. We could also add a comment that mingw/msys is not actively supported or tested.

[a3d4]

@ekpyron
strstr (arg, exclusions) == arg checks if arg starts with exclusions, not that it contains it. But that is not a problem. We have two remapping tests

test_solc_behaviour "${0}" "=/some/remapping/target" "" "" 1 "Invalid remapping: \"=/some/remapping/target\"." "" ""
test_solc_behaviour "${0}" "ctx:=/some/remapping/target" "" "" 1 "Invalid remapping: \"ctx:=/some/remapping/target\"." "" ""

However the second one was always working, msys2 does not translate the path for it.

I pushed the proposed change. I used export MSYS2_ARG_CONV_EXCL="=", but personally might prefer export MSYS2_ARG_CONV_EXCL="=/some/remapping/target".

In that case we are explicit about the workaround, and minimize the risk that our fix creates some undesirable side effects in future.

[chriseth]

I'm not sure we should introduce this complication and would prefer the more sloppy comparison.

[a3d4]

OK, reverted for now. We can implement flexible comparison (maybe optional?).
It could be taken together with the issue that our tests are insensitive to extra trailing newlines.

(BTW the LF-CRLF problem would still exist for expectation updates. If new outputs are produced with LFs, updated expectations will be inconsistent with CRLF expectations. Not nice, even if Git corrects it during line ending normalization.)

[chriseth]

To be honest, mingw looks like a very broken system to me and I'm not sure we should introduce these complications especially given the fact that we have no CI run on mingw.

[ekpyron]

To be honest, mingw looks like a very broken system to me and I'm not sure we should introduce these complications especially given the fact that we have no CI run on mingw.

mingw-w64 is actually quite nice (I've been using it for windows development myself in the past) and I think some support might actually be nice - but I'm not too happy about adding workarounds like semicolons in remapping paths for it either... maybe there's a better way, though.

[chriseth]

Could you please squash this?

Fine otherwise.