Path fixes for running tests om Cygwin/MinGW #675

Closed
wants to merge 4 commits into
from

Projects

None yet

6 participants

@Karlson2k
Contributor

This PR fix path conversion in testsuite for Cygwin/Msys.
Cygwin allow to map drives to some custom paths, so C:\\Windows\ can be mapped not to /cygdrive/c/Windows. Moreover, Msys2 use simplified scheme /c/Windows by default.

@mention-bot

By analyzing the blame information on this pull request, we identified @mback2k, @yangtse and @dfandrich to be potential reviewers

@mback2k
Member
mback2k commented Feb 22, 2016

Why do you want to hardcode the use of cygpath? I am running the testsuite, including OpenSSH tests using CopSSH, using just Msys2 and MinGW-w64 from an Msys2 shell just fine. Even Msys and MinGW32 works just fine. Please see here.

@Karlson2k
Contributor

The truth is opposite. Currently path is hardcoded in testsuite as testsuite depends on that any Msys path must starts with "/cygdrive".
The content of /etc/fstab on freshly installed Msys2:

# For a description of the file format, see the Users Guide
# http://cygwin.com/cygwin-ug-net/using.html#mount-table

# DO NOT REMOVE NEXT LINE. It remove cygdrive prefix from path
none / cygdrive binary,posix=0,noacl,user 0 0

Moreover, for example, I mount my development directory as /dvlp root path. More variants is possible.
Currently testsuite can't run on freshly installed Msys2/MinGW64 even with default settings.
cygpath is core Cygwin/Msys program, hardly you can find Msys with default ln, ls, rm, tr, but without cygpath.

@jay
Member
jay commented Feb 23, 2016

I can confirm msys2 /cygdrive/ paths don't work but I can also confirm original mingw32 (mine at least) doesn't come with a cygpath utility. We probably could use a function to transform the path depending on what's available.

@Karlson2k
Contributor

@jay, confirming, there are old Msys/MinGW without cygpath.
Developing more flexible patch, will update this PR.

@mback2k mback2k commented on an outdated diff Feb 23, 2016
tests/secureserver.pl
@@ -230,8 +230,8 @@ sub exit_signal_handler {
$tstunnel_windows = 1;
# replace Cygwin and MinGW drives within paths
- $capath =~ s/^(\/cygdrive)?\/(\w)\//$2\:\//;
- $certfile =~ s/^(\/cygdrive)?\/(\w)\//$2\:\//;
+ chomp($capath = `cygpath -m -a $capath`);
+ chomp($certfile = `cygpath -m -a $certfile`);
@mback2k
mback2k Feb 23, 2016 Member

I think these do not need to be changed, because this a conversion from Cygwin and msys to Windows style paths. The cygdrive part is actually optional here.

@mback2k mback2k commented on an outdated diff Feb 23, 2016
tests/sshserver.pl
@@ -393,9 +393,9 @@
if ($^O eq 'MSWin32' || $^O eq 'cygwin' || $^O eq 'msys') {
# convert MinGW drive paths to Cygwin drive paths
- $clipubkeyf_config =~ s/^\/(\w)\//\/cygdrive\/$1\//;
- $hstprvkeyf_config =~ s/^\/(\w)\//\/cygdrive\/$1\//;
- $pidfile_config =~ s/^\/(\w)\//\/cygdrive\/$1\//;
+ chomp($clipubkeyf_config = `cygpath -u -a $clipubkeyf_config`);
+ chomp($hstprvkeyf_config = `cygpath -u -a $hstprvkeyf_config`);
+ chomp($pidfile_config = `cygpath -u -a $pidfile_config`);
@mback2k
mback2k Feb 23, 2016 Member

These lines require a more flexible patch that does not require the cygpath utility.

@mback2k mback2k commented on an outdated diff Feb 23, 2016
tests/sshserver.pl
@@ -764,8 +769,8 @@ sub sshd_supports_opt {
if ($^O eq 'MSWin32' || $^O eq 'cygwin' || $^O eq 'msys') {
# convert MinGW drive paths to Cygwin drive paths
- $identity_config =~ s/^\/(\w)\//\/cygdrive\/$1\//;
- $knownhosts_config =~ s/^\/(\w)\//\/cygdrive\/$1\//;
+ chomp($identity_config = `cygpath -u -a $identity_config`);
+ chomp($knownhosts_config = `cygpath -u -a $knownhosts_config`);
@mback2k
mback2k Feb 23, 2016 Member

These lines require a more flexible patch that does not require the cygpath utility.

@bagder
Member
bagder commented Mar 14, 2016

Closing with 20 days with no comments to the review remarks. Feel free to re-open when/if we come back to this work again.

@bagder bagder closed this Mar 14, 2016
@Karlson2k
Contributor

It's not abandoned. I had to learn Perl to write module which is flexible enough to handle all Msys/Cygwin configurations. Will publish it soon, but I can't reopen this PR - only repo owners can do this.

@bagder
Member
bagder commented Mar 15, 2016

Well, if it is a different fix then it could warrant a new separate pull request. But sure, I'll reopen this if you think it is better.

@bagder bagder reopened this Mar 15, 2016
@gvanem
Contributor
gvanem commented Mar 15, 2016

@Karlson2k I'm curious about the test on 'MSWin32' in runtests.pl. This includes e.g. Strawberry Perl, but the .pl script(s) depends on a working fork which to me seems only Cygwin32/64 Perl has. Have you tried other Perls on Windows? Since I'm a Perl n00b myself, I wonder how runtest.pl could be made to work with Strawberry Perl on Windows.

@Karlson2k
Contributor

@gvanem, Strawberry Perl as well as ActivePerl have fork emulation on Windows: http://perldoc.perl.org/perlfork.html
But testsuite depends on other GNU tools and process paths in Unix-style. So test can be run only on Msys/Cygwin environment with Perl build for Msys/Cygwin. But I think that support for native Windows Perl can be added without massive overhauling.

@gvanem
Contributor
gvanem commented Mar 15, 2016

So test can be run only on Msys/Cygwin environment with Perl build for Msys/Cygwin.

Thanks. Just what I thought.

@Karlson2k
Contributor

Updated with new Perl helper module.
Thoroughly tested path conversion in any possible combinations under Msys/Msys2/Cygwin.

@bagder bagder changed the title from Some fixes for testsuite, mostly Cygwin/MinGW to Path fixes for running tests om Cygwin/MinGW Mar 15, 2016
@Karlson2k
Contributor

Updated with improved compatibility with older Perl.

@bagder bagder closed this in 50129e6 Apr 29, 2016
@bagder
Member
bagder commented Apr 29, 2016

thanks!

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