Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cygwin cygdrive prefix test #15076

Closed
p5pRT opened this issue Dec 6, 2015 · 25 comments
Closed

Cygwin cygdrive prefix test #15076

p5pRT opened this issue Dec 6, 2015 · 25 comments

Comments

@p5pRT
Copy link

p5pRT commented Dec 6, 2015

Migrated from rt.perl.org#126834 (status was 'resolved')

Searchable as RT126834$

@p5pRT
Copy link
Author

p5pRT commented Dec 6, 2015

From Stromeko@nexgo.de

The test for the cygdrive prefix can be simplified by using the cygpath
executable instead of parsing the output from df or mount. An
alternative for modern Cygwin would be to use

my $prefix2 = readlink "/proc/cygdrive"

if a fork and some parsing should be avoided and supporting older Cygwin
versions is not considered important.

@p5pRT
Copy link
Author

p5pRT commented Dec 6, 2015

From Stromeko@nexgo.de

perl.fix-636efde.patch
--- t/lib/cygwin.t.orig	2015-12-06 18:29:01.970308000 +0100
+++ t/lib/cygwin.t	2015-12-06 18:44:29.329880800 +0100
@@ -51,18 +51,14 @@
 
 # Cygdrive mount prefix
 my @flags = split(/,/, Cygwin::mount_flags('/cygdrive'));
-my $prefix = pop(@flags);
-ok($prefix, "cygdrive mount prefix = " . (($prefix) ? $prefix : '<none>'));
-chomp(my $prefix2 = `df -a | grep -i '^c: ' | cut -d% -f2 | xargs`);
-# we get something like "C: - - - - /cygdrive" if this isn't the entry
-# df displays free space info for
-$prefix2 =~ s/.* //;
-$prefix2 =~ s/\/c$//i;
+my $prefix1 = pop(@flags);
+ok($prefix1, "cygdrive mount prefix1 = " . (($prefix1) ? $prefix1 : '<none>'));
+chomp(my $prefix2 = `cygpath C:`); # the drive need not actually exist, so this will always work
+$prefix2 =~ s/\/c$//;
+$prefix2 = "/" unless $prefix2;
 SKIP:
 {
-    $prefix2
-       or skip("No C: entry found in df output", 1);
-    is($prefix, $prefix2, 'cygdrive mount prefix');
+    is($prefix1, $prefix2, 'cygdrive mount prefix2 = ' . $prefix2);
 }
 
 my @mnttbl = Cygwin::mount_table();

@p5pRT
Copy link
Author

p5pRT commented Dec 6, 2015

From Stromeko@nexgo.de

Regards,
Achim.
--
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Wavetables for the Terratec KOMPLEXER​:
http​://Synth.Stromeko.net/Downloads.html#KomplexerWaves

@p5pRT
Copy link
Author

p5pRT commented Dec 6, 2015

From @tonycoz

On Sun Dec 06 09​:57​:49 2015, Stromeko@​nexgo.de wrote​:

The test for the cygdrive prefix can be simplified by using the cygpath
executable instead of parsing the output from df or mount. An
alternative for modern Cygwin would be to use

my $prefix2 = readlink "/proc/cygdrive"

if a fork and some parsing should be avoided and supporting older Cygwin
versions is not considered important.

The change looks sane, though I'm not sure why you renamed $prefix.

Can you supply this patch as a "git format-patch" style patch?

Tony

@p5pRT
Copy link
Author

p5pRT commented Dec 6, 2015

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Dec 7, 2015

From Stromeko@nexgo.de

Tony Cook via RT writes​:

The change looks sane, though I'm not sure why you renamed $prefix.

Just for symmetry reasons, but I have no problem with keeping it named
just $prefix.

Can you supply this patch as a "git format-patch" style patch?

Can do.

Regards,
Achim.
--
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Samples for the Waldorf Blofeld​:
http​://Synth.Stromeko.net/Downloads.html#BlofeldSamplesExtra

@p5pRT
Copy link
Author

p5pRT commented Dec 7, 2015

From Stromeko@nexgo.de

Tony Cook via RT writes​:

Can you supply this patch as a "git format-patch" style patch?

Attached.

@p5pRT
Copy link
Author

p5pRT commented Dec 7, 2015

From Stromeko@nexgo.de

0001-perl-126834-Cygwin-cygdrive-prefix-test.patch
From b12ceb2c68415fbe6e46390f71870fb73854f24a Mon Sep 17 00:00:00 2001
From: Achim Gratz <Stromeko@NexGo.DE>
Date: Mon, 7 Dec 2015 19:42:37 +0100
Subject: [PATCH] [perl #126834] Cygwin cygdrive prefix test

* t/lib/cygwin.t: The test for the cygdrive prefix is simplified by
using the cygpath executable instead of parsing the output from df or
mount.

An alternative for modern Cygwin would be to use

my $prefix2 = readlink "/proc/cygdrive"

if a fork and some parsing should be avoided and supporting older Cygwin
versions is not considered important.
---
 t/lib/cygwin.t | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/t/lib/cygwin.t b/t/lib/cygwin.t
index 9033d3f..174f3bd 100644
--- a/t/lib/cygwin.t
+++ b/t/lib/cygwin.t
@@ -52,13 +52,14 @@ is(Cygwin::mount_flags("/cygdrive") =~ /,cygdrive/,  1, "check cygdrive mount_fl
 # Cygdrive mount prefix
 my @flags = split(/,/, Cygwin::mount_flags('/cygdrive'));
 my $prefix = pop(@flags);
-ok($prefix, "cygdrive mount prefix = " . (($prefix) ? $prefix : '<none>'));
-chomp(my $prefix2 = `df | grep -i '^c: ' | cut -d% -f2 | xargs`);
-$prefix2 =~ s/\/c$//i;
-if (! $prefix2) {
-    $prefix2 = '/';
+ok($prefix, "cygdrive mount prefix  = " . (($prefix) ? $prefix : '<none>'));
+chomp(my $prefix2 = `cygpath C:`); # the drive need not actually exist, so this will always work
+$prefix2 =~ s/\/c$//;
+$prefix2 = "/" unless $prefix2;
+SKIP:
+{
+    is($prefix, $prefix2, 'cygdrive mount prefix2 = ' . $prefix2);
 }
-is($prefix, $prefix2, 'cygdrive mount prefix');
 
 my @mnttbl = Cygwin::mount_table();
 ok(@mnttbl > 0, "non empty mount_table");
-- 
2.6.2

@p5pRT
Copy link
Author

p5pRT commented Dec 7, 2015

From Stromeko@nexgo.de

Regards,
Achim.
--
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

SD adaptation for Waldorf rackAttack V1.04R1​:
http​://Synth.Stromeko.net/Downloads.html#WaldorfSDada

@p5pRT
Copy link
Author

p5pRT commented Dec 7, 2015

From vano@mail.mipt.ru

`cygpath C​:\' isn't completely fool-proof as C​:\ might be mounted independently.
(you decide if it's fool-proof enough for the case at hand)

`readlink /proc/cygdrive' does look like the best solution.

So, the best way I see is to use it and fall back to the heuristics if it's
missing.

"Shelling out" can't be avoided unless you wish to check the registry by hand​:
initially (in 2000), there was no dedicated utility to read the prefix at all,
the docs pointed directly to the registry
(http​://web.archive.org/web/20001213205800/http​://www.cygwin.com/cygwin-ug-net/using.html;
http​://web.archive.org/web/20001213195700/http​://www.cygwin.com/cygwin-ug-net/using-utils.html#MOUNT).

I'll now check the Cygwin repo for /proc/cygdrive addition time vs. registry
setting elimination time to see if one can avoid the heuristics entirely and
skip from the former directly to the latter.

--
Best regards,
Ivan mailto​:vano@​mail.mipt.ru

@p5pRT
Copy link
Author

p5pRT commented Dec 7, 2015

From Stromeko@nexgo.de

Ivan Pozdeev via perl5-porters writes​:

`cygpath C​:\' isn't completely fool-proof as C​:\ might be mounted independently.
(you decide if it's fool-proof enough for the case at hand)

In which case the former code would fail just the same. If you wanted,
you could loop through all 26 drive letters and see which prefix comes
up the most often and pick that as the cygdrive prefix, but it could
still fail in some contrived situations.

`readlink /proc/cygdrive' does look like the best solution.

So, the best way I see is to use it and fall back to the heuristics if it's
missing.

Well, I can get behind that idea; thanks for mentioning. How about the
attached?

@p5pRT
Copy link
Author

p5pRT commented Dec 7, 2015

From Stromeko@nexgo.de

0001-perl-126834-Cygwin-cygdrive-prefix-test.patch
From bcfee9939cab7b09a129ee72db7281c27d1ff9c3 Mon Sep 17 00:00:00 2001
From: Achim Gratz <Stromeko@NexGo.DE>
Date: Mon, 7 Dec 2015 19:42:37 +0100
Subject: [PATCH] [perl #126834] Cygwin cygdrive prefix test

* t/lib/cygwin.t: Use the /proc virtual filesystem to determine the
  cygdrive prefix.  If that isn't available, fall back to using the
  cygpath executable instead of parsing the output from df or mount
  for older Cygwin.  That fallback can fail if C:\ is manually mounted
  someplace else, but the former code had the same problem.
---
 t/lib/cygwin.t | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/t/lib/cygwin.t b/t/lib/cygwin.t
index 9033d3f..dd2e31e 100644
--- a/t/lib/cygwin.t
+++ b/t/lib/cygwin.t
@@ -52,13 +52,16 @@ is(Cygwin::mount_flags("/cygdrive") =~ /,cygdrive/,  1, "check cygdrive mount_fl
 # Cygdrive mount prefix
 my @flags = split(/,/, Cygwin::mount_flags('/cygdrive'));
 my $prefix = pop(@flags);
-ok($prefix, "cygdrive mount prefix = " . (($prefix) ? $prefix : '<none>'));
-chomp(my $prefix2 = `df | grep -i '^c: ' | cut -d% -f2 | xargs`);
-$prefix2 =~ s/\/c$//i;
-if (! $prefix2) {
-    $prefix2 = '/';
+ok($prefix, "cygdrive mount prefix  = " . (($prefix) ? $prefix : '<none>'));
+my $prefix2 = readlink "/pro/cygdrive";
+unless ($prefix2) {
+    # fallback to old Cygwin, the drive need not actually exist, so
+    # this will always work (but might return the wrong prefix if the
+    # user re-mounted C:\
+    chomp($prefix2 = `cygpath C:`);
+    $prefix2 = substr($prefix2, 0, -1-(length($prefix2)>2));
 }
-is($prefix, $prefix2, 'cygdrive mount prefix');
+is($prefix, $prefix2, 'cygdrive mount prefix2 = ' . $prefix2);
 
 my @mnttbl = Cygwin::mount_table();
 ok(@mnttbl > 0, "non empty mount_table");
-- 
2.6.2

@p5pRT
Copy link
Author

p5pRT commented Dec 7, 2015

From Stromeko@nexgo.de

Regards,
Achim.
--
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Factory and User Sound Singles for Waldorf Q+, Q and microQ​:
http​://Synth.Stromeko.net/Downloads.html#WaldorfSounds

@p5pRT
Copy link
Author

p5pRT commented Dec 7, 2015

From Stromeko@nexgo.de

Achim Gratz writes​:

Well, I can get behind that idea; thanks for mentioning. How about the
attached?

Sorry, the patch still had a change in I made to test the fallback…
Here's the correct one.

@p5pRT
Copy link
Author

p5pRT commented Dec 7, 2015

From Stromeko@nexgo.de

0001-perl-126834-Cygwin-cygdrive-prefix-test.patch
From bcfee9939cab7b09a129ee72db7281c27d1ff9c3 Mon Sep 17 00:00:00 2001
From: Achim Gratz <Stromeko@NexGo.DE>
Date: Mon, 7 Dec 2015 19:42:37 +0100
Subject: [PATCH] [perl #126834] Cygwin cygdrive prefix test

* t/lib/cygwin.t: Use the /proc virtual filesystem to determine the
  cygdrive prefix.  If that isn't available, fall back to using the
  cygpath executable instead of parsing the output from df or mount
  for older Cygwin.  That fallback can fail if C:\ is manually mounted
  someplace else, but the former code had the same problem.
---
 t/lib/cygwin.t | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/t/lib/cygwin.t b/t/lib/cygwin.t
index 9033d3f..dd2e31e 100644
--- a/t/lib/cygwin.t
+++ b/t/lib/cygwin.t
@@ -52,13 +52,16 @@ is(Cygwin::mount_flags("/cygdrive") =~ /,cygdrive/,  1, "check cygdrive mount_fl
 # Cygdrive mount prefix
 my @flags = split(/,/, Cygwin::mount_flags('/cygdrive'));
 my $prefix = pop(@flags);
-ok($prefix, "cygdrive mount prefix = " . (($prefix) ? $prefix : '<none>'));
-chomp(my $prefix2 = `df | grep -i '^c: ' | cut -d% -f2 | xargs`);
-$prefix2 =~ s/\/c$//i;
-if (! $prefix2) {
-    $prefix2 = '/';
+ok($prefix, "cygdrive mount prefix  = " . (($prefix) ? $prefix : '<none>'));
+my $prefix2 = readlink "/proc/cygdrive";
+unless ($prefix2) {
+    # fallback to old Cygwin, the drive need not actually exist, so
+    # this will always work (but might return the wrong prefix if the
+    # user re-mounted C:\
+    chomp($prefix2 = `cygpath C:`);
+    $prefix2 = substr($prefix2, 0, -1-(length($prefix2)>2));
 }
-is($prefix, $prefix2, 'cygdrive mount prefix');
+is($prefix, $prefix2, 'cygdrive mount prefix2 = ' . $prefix2);
 
 my @mnttbl = Cygwin::mount_table();
 ok(@mnttbl > 0, "non empty mount_table");
-- 
2.6.2

@p5pRT
Copy link
Author

p5pRT commented Dec 7, 2015

From Stromeko@nexgo.de

Regards,
Achim.
--
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Factory and User Sound Singles for Waldorf Q+, Q and microQ​:
http​://Synth.Stromeko.net/Downloads.html#WaldorfSounds

@p5pRT
Copy link
Author

p5pRT commented Dec 9, 2015

From @tonycoz

On Mon Dec 07 12​:44​:47 2015, Stromeko@​nexgo.de wrote​:

Achim Gratz writes​:

Well, I can get behind that idea; thanks for mentioning. How about the
attached?

Sorry, the patch still had a change in I made to test the fallback…
Here's the correct one.

This patch doesn't seem to be against blead perl.

Tony

@p5pRT
Copy link
Author

p5pRT commented Dec 9, 2015

From Stromeko@nexgo.de

Tony Cook via RT writes​:

This patch doesn't seem to be against blead perl.

Sorry, you're right -- should be fixed now.

@p5pRT
Copy link
Author

p5pRT commented Dec 9, 2015

From Stromeko@nexgo.de

0001-perl-126834-Cygwin-cygdrive-prefix-test.patch
From 91acd29f061acccda0c13e8f47e0d82f45eea6c5 Mon Sep 17 00:00:00 2001
From: Achim Gratz <Achim.Gratz@Stromeko.DE>
Date: Wed, 9 Dec 2015 18:59:03 +0100
Subject: [PATCH] [perl #126834] Cygwin cygdrive prefix test

* t/lib/cygwin.t: Use the /proc virtual filesystem to determine the
  cygdrive prefix.  If that isn't available, fall back to using the
  cygpath executable instead of parsing the output from df or mount
  for older Cygwin.  That fallback can fail if C:\ is manually mounted
  someplace else, but the former code had the same problem.
---
 t/lib/cygwin.t | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/t/lib/cygwin.t b/t/lib/cygwin.t
index a619e40..ba86170 100644
--- a/t/lib/cygwin.t
+++ b/t/lib/cygwin.t
@@ -52,18 +52,16 @@ is(Cygwin::mount_flags("/cygdrive") =~ /,cygdrive/,  1, "check cygdrive mount_fl
 # Cygdrive mount prefix
 my @flags = split(/,/, Cygwin::mount_flags('/cygdrive'));
 my $prefix = pop(@flags);
-ok($prefix, "cygdrive mount prefix = " . (($prefix) ? $prefix : '<none>'));
-chomp(my $prefix2 = `df -a | grep -i '^c: ' | cut -d% -f2 | xargs`);
-# we get something like "C: - - - - /cygdrive" if this isn't the entry
-# df displays free space info for
-$prefix2 =~ s/.* //;
-$prefix2 =~ s/\/c$//i;
-SKIP:
-{
-    $prefix2
-       or skip("No C: entry found in df output", 1);
-    is($prefix, $prefix2, 'cygdrive mount prefix');
+ok($prefix, "cygdrive mount prefix  = " . (($prefix) ? $prefix : '<none>'));
+my $prefix2 = readlink "/proc/cygdrive";
+unless ($prefix2) {
+    # fallback to old Cygwin, the drive need not actually exist, so
+    # this will always work (but might return the wrong prefix if the
+    # user re-mounted C:\
+    chomp($prefix2 = `cygpath C:`);
+    $prefix2 = substr($prefix2, 0, -1-(length($prefix2)>2));
 }
+is($prefix, $prefix2, 'cygdrive mount prefix2 = ' . $prefix2);
 
 my @mnttbl = Cygwin::mount_table();
 ok(@mnttbl > 0, "non empty mount_table");
-- 
2.6.2

@p5pRT
Copy link
Author

p5pRT commented Dec 9, 2015

From Stromeko@nexgo.de

Regards,
Achim.
--
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Wavetables for the Waldorf Blofeld​:
http​://Synth.Stromeko.net/Downloads.html#BlofeldUserWavetables

@p5pRT
Copy link
Author

p5pRT commented Dec 9, 2015

From @tonycoz

On Wed Dec 09 10​:02​:45 2015, Stromeko@​nexgo.de wrote​:

Tony Cook via RT writes​:

This patch doesn't seem to be against blead perl.

Sorry, you're right -- should be fixed now.

Thanks, applied as 03e4d23.

Added you as an author in 772a2be.

Tony

@p5pRT
Copy link
Author

p5pRT commented Dec 9, 2015

@tonycoz - Status changed from 'open' to 'pending release'

@p5pRT
Copy link
Author

p5pRT commented Dec 10, 2015

From Stromeko@nexgo.de

Tony Cook via RT writes​:

Thanks, applied as 03e4d23.

Thank you.

Regards,
Achim.
--
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Factory and User Sound Singles for Waldorf rackAttack​:
http​://Synth.Stromeko.net/Downloads.html#WaldorfSounds

@p5pRT
Copy link
Author

p5pRT commented May 13, 2016

From @khwilliamson

Thank you for submitting this report. You have helped make Perl better.
 
With the release of Perl 5.24.0 on May 9, 2016, this and 149 other issues have been resolved.

Perl 5.24.0 may be downloaded via https://metacpan.org/release/RJBS/perl-5.24.0

@p5pRT
Copy link
Author

p5pRT commented May 13, 2016

@khwilliamson - Status changed from 'pending release' to 'resolved'

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

No branches or pull requests

1 participant