Windows patches for the next release

classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|

Windows patches for the next release

Paul Smith-20
Hi all;

There are some decisions to be made on outstanding Windows patches.
Other than these I believe the next release is ready to go... not
everything is fixed but at some point you have to pull the trigger.  I'm
willing to apply the ones that are appropriate, but need some guidance
from Eli.  Also, do you need another release tarball for testing, or is
Git good enough?


Things that I know of:

Spawn patch: at this point I think we'll have to leave this one out; it
can continue to be applied as an external patch for now by those who
want it and we can try to get an official patch for the next release.

abspath / DOS paths fix


Also, from Savannah:

https://savannah.gnu.org/bugs/index.php?30323
Not sure what the final word on this one is.

https://savannah.gnu.org/bugs/index.php?15718
Don't know if this is fixed, but probably too much for this release.

Other stuff?


_______________________________________________
Make-w32 mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/make-w32
Reply | Threaded
Open this post in threaded view
|

Re: Windows patches for the next release

Eli Zaretskii
> From: Paul Smith <[hidden email]>
> Cc: Denis Excoffier <[hidden email]>, Pavel Fedin
>  <[hidden email]>, Chris Faylor
>  <[hidden email]>,  [hidden email]
> Date: Mon, 30 Sep 2013 08:47:39 -0400
>
> There are some decisions to be made on outstanding Windows patches.
> Other than these I believe the next release is ready to go... not
> everything is fixed but at some point you have to pull the trigger.  I'm
> willing to apply the ones that are appropriate, but need some guidance
> from Eli.  Also, do you need another release tarball for testing, or is
> Git good enough?

I suggest another RC, I think the changes since the last one are
non-trivial.

> Things that I know of:
>
> Spawn patch: at this point I think we'll have to leave this one out; it
> can continue to be applied as an external patch for now by those who
> want it and we can try to get an official patch for the next release.

I agree.

> abspath / DOS paths fix

I'd like to fix this one, it should be easy.

> Also, from Savannah:
>
> https://savannah.gnu.org/bugs/index.php?30323
> Not sure what the final word on this one is.
>
> https://savannah.gnu.org/bugs/index.php?15718
> Don't know if this is fixed, but probably too much for this release.

I'll take a look soon.

_______________________________________________
Make-w32 mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/make-w32
Reply | Threaded
Open this post in threaded view
|

Re: Windows patches for the next release

Paul Smith-20
On Mon, 2013-09-30 at 18:15 +0300, Eli Zaretskii wrote:
> I suggest another RC, I think the changes since the last one are
> non-trivial.

OK, I created a new RC, available now on make-alpha.

I don't personally plan to make any more changes, so as soon as you feel
you're happy with the Windows support I will release.

Unless crazy new critical issues come to light.


_______________________________________________
Make-w32 mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/make-w32
Reply | Threaded
Open this post in threaded view
|

Re: Windows patches for the next release

Eli Zaretskii
> From: Paul Smith <[hidden email]>
> Cc: [hidden email], [hidden email], [hidden email]
> Date: Tue, 01 Oct 2013 01:08:32 -0400
>
> On Mon, 2013-09-30 at 18:15 +0300, Eli Zaretskii wrote:
> > I suggest another RC, I think the changes since the last one are
> > non-trivial.
>
> OK, I created a new RC, available now on make-alpha.

Thanks.  It still builds fine on Windows using MinGW GCC.

> I don't personally plan to make any more changes, so as soon as you feel
> you're happy with the Windows support I will release.

Here's the patch for $abspath that should fix the Cygwin build.  Would
Cygwin people please test it, and in particular see if the related
failures in the test suite are gone?  I don't have a Cygwin
environment to even compile with Cygwin.

I'd especially appreciate if Chris could at least eyeball the patch.

Thanks in advance.

Btw, Paul: I see that the sources don't consistently use STOP_SET
where characters are tested for being directory separators; sometimes
they actually test for both slashes and backslashes.  Should we fix
that throughout the sources?

=======================================================================

--- function.c~0 2013-09-30 07:12:36.000000000 +0300
+++ function.c 2013-10-01 19:48:45.472000000 +0300
@@ -1949,8 +1949,12 @@ func_not (char *o, char **argv, char *fu
 
 
 #ifdef HAVE_DOS_PATHS
-#define IS_ABSOLUTE(n) (n[0] && n[1] == ':')
-#define ROOT_LEN 3
+# ifdef __CYGWIN__
+#  define IS_ABSOLUTE(n) ((n[0] && n[1] == ':') || STOP_SET (n[0], MAP_PATHSEP))
+# else
+#  define IS_ABSOLUTE(n) (n[0] && n[1] == ':')
+# endif
+# define ROOT_LEN 3
 #else
 #define IS_ABSOLUTE(n) (n[0] == '/')
 #define ROOT_LEN 1
@@ -2001,13 +2005,17 @@ abspath (const char *name, char *apath)
     }
   else
     {
+#ifdef __CYGWIN__
+      if (STOP_SET (name[0], MAP_PATHSEP))
+ root_len = 1;
+#endif
       strncpy (apath, name, root_len);
       apath[root_len] = '\0';
       dest = apath + root_len;
       /* Get past the root, since we already copied it.  */
       name += root_len;
 #ifdef HAVE_DOS_PATHS
-      if (! STOP_SET (apath[2], MAP_PATHSEP))
+      if (! STOP_SET (apath[root_len - 1], MAP_PATHSEP))
         {
           /* Convert d:foo into d:./foo and increase root_len.  */
           apath[2] = '.';
@@ -2018,7 +2026,7 @@ abspath (const char *name, char *apath)
           name--;
         }
       else
-        apath[2] = '/'; /* make sure it's a forward slash */
+        apath[root_len - 1] = '/'; /* make sure it's a forward slash */
 #endif
     }
 

_______________________________________________
Make-w32 mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/make-w32
Reply | Threaded
Open this post in threaded view
|

Re: Windows patches for the next release

Eli Zaretskii
In reply to this post by Paul Smith-20
> From: Paul Smith <[hidden email]>
> Cc: Denis Excoffier <[hidden email]>, Pavel Fedin
>  <[hidden email]>, Chris Faylor
>  <[hidden email]>,  [hidden email]
> Date: Mon, 30 Sep 2013 08:47:39 -0400
>
> abspath / DOS paths fix

I posted a patch for this.  Let's hope some Cygwin user will be able
to test it soon.

> Also, from Savannah:
>
> https://savannah.gnu.org/bugs/index.php?30323
> Not sure what the final word on this one is.

I analyzed that and posted a comment.  Bottom line is that the Windows
build behaves exactly like a Posix build, when the Make executable is
found in the current directory via PATH.

> https://savannah.gnu.org/bugs/index.php?15718
> Don't know if this is fixed, but probably too much for this release.

Whatever is left is minor and doesn't merit waiting for.

_______________________________________________
Make-w32 mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/make-w32
Reply | Threaded
Open this post in threaded view
|

Re: Windows patches for the next release

Paul Smith-20
In reply to this post by Eli Zaretskii
On Tue, 2013-10-01 at 19:52 +0300, Eli Zaretskii wrote:
> Btw, Paul: I see that the sources don't consistently use STOP_SET
> where characters are tested for being directory separators; sometimes
> they actually test for both slashes and backslashes.  Should we fix
> that throughout the sources?

STOP_SET() is basically a performance improvement.  It would be fine to
use it in more places, but I don't want to change things at this point
unless we are very confident they won't introduce any brown paper bag
bugs for the 4.0 release :-).  So if you're confident, go ahead.

Otherwise we can wait for a followup release.



_______________________________________________
Make-w32 mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/make-w32
Reply | Threaded
Open this post in threaded view
|

Re: Windows patches for the next release

Eli Zaretskii
> From: Paul Smith <[hidden email]>
> Cc: [hidden email], [hidden email], [hidden email]
> Date: Tue, 01 Oct 2013 13:37:36 -0400
>
> On Tue, 2013-10-01 at 19:52 +0300, Eli Zaretskii wrote:
> > Btw, Paul: I see that the sources don't consistently use STOP_SET
> > where characters are tested for being directory separators; sometimes
> > they actually test for both slashes and backslashes.  Should we fix
> > that throughout the sources?
>
> STOP_SET() is basically a performance improvement.  It would be fine to
> use it in more places, but I don't want to change things at this point
> unless we are very confident they won't introduce any brown paper bag
> bugs for the 4.0 release :-).  So if you're confident, go ahead.
>
> Otherwise we can wait for a followup release.

I'm confident, but it's not important enough to have even a small
risk at this point.

_______________________________________________
Make-w32 mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/make-w32
Reply | Threaded
Open this post in threaded view
|

Re: Windows patches for the next release

Denis Excoffier-4
In reply to this post by Eli Zaretskii
On 2013-10-01 18:52, Eli Zaretskii wrote:

>
> Here's the patch for $abspath that should fix the Cygwin build.  
> Would
> Cygwin people please test it, and in particular see if the related
> failures in the test suite are gone?  I don't have a Cygwin
> environment to even compile with Cygwin.
>
The abspath error is gone. However, output-sync, recursion, MAKE
remain.
Included work.tar.xz

Regards,

Denis Excoffier.

_______________________________________________
Make-w32 mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/make-w32

work.tar.xz (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Windows patches for the next release

Denis Excoffier-4
On 2013-10-02 09:34, Denis Excoffier wrote:
> The abspath error is gone. However, output-sync, recursion, MAKE
> remain.
> Included work.tar.xz

I can also observe that with the following patch (below), make check
runs with no error anymore (of course, this patch is not for inclusion
in HEAD).

% cat patch
diff -uNrp make-3.99.93-original/tests/run_make_tests.pl
make-3.99.93-patched/tests/run_make_tests.pl
--- make-3.99.93-original/tests/run_make_tests.pl       2013-09-22
23:10:09.000000000 +0159
+++ make-3.99.93-patched/tests/run_make_tests.pl        2013-10-02
10:19:43.081853900 +0159
@@ -353,7 +353,7 @@ sub set_more_defaults

     if (index ($make_path, ":") != 1 && index ($make_path, "/") > 0)
     {
-      $mkpath = "$pwd$pathsep$make_path";
+      $mkpath = $make_path;
     }
     else
     {
%

Denis Excoffier.

_______________________________________________
Make-w32 mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/make-w32
Reply | Threaded
Open this post in threaded view
|

Re: Windows patches for the next release

Eli Zaretskii
In reply to this post by Denis Excoffier-4
> Date: Wed, 02 Oct 2013 09:34:06 +0200
> From: Denis Excoffier <[hidden email]>
> Cc: <[hidden email]>, <[hidden email]>, <[hidden email]>,
>  <[hidden email]>
>
> The abspath error is gone.

Thanks for testing, I will commit the changes soon.

> However, output-sync, recursion, MAKE remain.  Included work.tar.xz

All of those failures are because of the exact form of $(MAKE).  What
Cygwin produces is functionally equivalent to what the test suite
expects, so I think we can consider these failures redundant, if not
bogus.  (If Paul accepts your patch to the suite, they will be gone
altogether.)

_______________________________________________
Make-w32 mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/make-w32
Reply | Threaded
Open this post in threaded view
|

Re: Windows patches for the next release

Eli Zaretskii
> Date: Wed, 02 Oct 2013 19:17:58 +0300
> From: Eli Zaretskii <[hidden email]>
> Cc: [hidden email], [hidden email], [hidden email]
>
> > Date: Wed, 02 Oct 2013 09:34:06 +0200
> > From: Denis Excoffier <[hidden email]>
> > Cc: <[hidden email]>, <[hidden email]>, <[hidden email]>,
> >  <[hidden email]>
> >
> > The abspath error is gone.
>
> Thanks for testing, I will commit the changes soon.

Done.

_______________________________________________
Make-w32 mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/make-w32
Reply | Threaded
Open this post in threaded view
|

Re: Windows patches for the next release

Christopher Faylor-27
In reply to this post by Eli Zaretskii
On Tue, Oct 01, 2013 at 07:52:02PM +0300, Eli Zaretskii wrote:

>> From: Paul Smith <[hidden email]>
>> Cc: [hidden email], [hidden email], [hidden email]
>> Date: Tue, 01 Oct 2013 01:08:32 -0400
>>
>> On Mon, 2013-09-30 at 18:15 +0300, Eli Zaretskii wrote:
>> > I suggest another RC, I think the changes since the last one are
>> > non-trivial.
>>
>> OK, I created a new RC, available now on make-alpha.
>
>Thanks.  It still builds fine on Windows using MinGW GCC.
>
>> I don't personally plan to make any more changes, so as soon as you feel
>> you're happy with the Windows support I will release.
>
>Here's the patch for $abspath that should fix the Cygwin build.  Would
>Cygwin people please test it, and in particular see if the related
>failures in the test suite are gone?  I don't have a Cygwin
>environment to even compile with Cygwin.
>
>I'd especially appreciate if Chris could at least eyeball the patch.
>
>@@ -2001,13 +2005,17 @@ abspath (const char *name, char *apath)
>     }
>   else
>     {
>+#ifdef __CYGWIN__
>+      if (STOP_SET (name[0], MAP_PATHSEP))
>+ root_len = 1;
>+#endif

I've eyeballed the patch even though I won't be releasing a version of
make which has this turned on.

I think the above unnecessary if HAVE_DOS_PATHS is true and should be

#if defined(__CYGWIN__) && defined(HAVE_DOS_PATHS)

I'm not sure it's correct to consider \a to be an absolute path in the
same way that /a is since they mean different things to Cygwin but I
guess that's not something that I have to worry about.

cgf

_______________________________________________
Make-w32 mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/make-w32
Reply | Threaded
Open this post in threaded view
|

Re: Windows patches for the next release

Eli Zaretskii
> Date: Thu, 3 Oct 2013 01:23:15 -0400
> From: Christopher Faylor <[hidden email]>
>
> >@@ -2001,13 +2005,17 @@ abspath (const char *name, char *apath)
> >     }
> >   else
> >     {
> >+#ifdef __CYGWIN__
> >+      if (STOP_SET (name[0], MAP_PATHSEP))
> >+ root_len = 1;
> >+#endif
>
> I've eyeballed the patch even though I won't be releasing a version of
> make which has this turned on.
>
> I think the above unnecessary if HAVE_DOS_PATHS is true and should be
>
> #if defined(__CYGWIN__) && defined(HAVE_DOS_PATHS)

You are right, I pushed a fix.

> I'm not sure it's correct to consider \a to be an absolute path in the
> same way that /a is since they mean different things to Cygwin but I
> guess that's not something that I have to worry about.

I thought about that as well.  I actually think that Cygwin should not
consider a backslash as a directory separator, even if HAVE_DOS_PATHS
is defined.  What do you think?

Anyway, such a change modifies behavior (under HAVE_DOS_PATHS), and
its prerequisite is to use STOP_SET everywhere in Make.  So I think I
will make that change after the release (unless you, or someone else
object).

Thanks.

_______________________________________________
Make-w32 mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/make-w32
Reply | Threaded
Open this post in threaded view
|

Re: Windows patches for the next release

Christopher Faylor-27
On Thu, Oct 03, 2013 at 07:05:42PM +0300, Eli Zaretskii wrote:

>> Date: Thu, 3 Oct 2013 01:23:15 -0400
>> From: Christopher Faylor <[hidden email]>
>>
>> >@@ -2001,13 +2005,17 @@ abspath (const char *name, char *apath)
>> >     }
>> >   else
>> >     {
>> >+#ifdef __CYGWIN__
>> >+      if (STOP_SET (name[0], MAP_PATHSEP))
>> >+ root_len = 1;
>> >+#endif
>>
>> I've eyeballed the patch even though I won't be releasing a version of
>> make which has this turned on.
>>
>> I think the above unnecessary if HAVE_DOS_PATHS is true and should be
>>
>> #if defined(__CYGWIN__) && defined(HAVE_DOS_PATHS)
>
>You are right, I pushed a fix.

You got it even with my garbled sentence.  Thanks.

>> I'm not sure it's correct to consider \a to be an absolute path in the
>> same way that /a is since they mean different things to Cygwin but I
>> guess that's not something that I have to worry about.
>
>I thought about that as well.  I actually think that Cygwin should not
>consider a backslash as a directory separator, even if HAVE_DOS_PATHS
>is defined.  What do you think?

Hmm.  I'm not sure.  It wouldn't bother me but there may be people who
want to be able to use backslashes.

>Anyway, such a change modifies behavior (under HAVE_DOS_PATHS), and
>its prerequisite is to use STOP_SET everywhere in Make.  So I think I
>will make that change after the release (unless you, or someone else
>object).

Sounds good.

cgf

_______________________________________________
Make-w32 mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/make-w32
Reply | Threaded
Open this post in threaded view
|

Re: Windows patches for the next release

Eli Zaretskii
> Date: Fri, 4 Oct 2013 01:06:58 -0400
> From: Christopher Faylor <[hidden email]>
>
> >> I'm not sure it's correct to consider \a to be an absolute path in the
> >> same way that /a is since they mean different things to Cygwin but I
> >> guess that's not something that I have to worry about.
> >
> >I thought about that as well.  I actually think that Cygwin should not
> >consider a backslash as a directory separator, even if HAVE_DOS_PATHS
> >is defined.  What do you think?
>
> Hmm.  I'm not sure.  It wouldn't bother me but there may be people who
> want to be able to use backslashes.

I just think that a backslash will fail to do what the users expect,
certainly if the recipe's commands are invoked via the shell.

Would asking on the Cygwin list help understanding whether users will
object to such a change?

_______________________________________________
Make-w32 mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/make-w32
Reply | Threaded
Open this post in threaded view
|

Re: Windows patches for the next release

Paul Smith-20
In reply to this post by Eli Zaretskii
On Wed, 2013-10-02 at 19:17 +0300, Eli Zaretskii wrote:

> > Date: Wed, 02 Oct 2013 09:34:06 +0200
> > From: Denis Excoffier <[hidden email]>
> > Cc: <[hidden email]>, <[hidden email]>, <[hidden email]>,
> >  <[hidden email]>
> >
> > The abspath error is gone.
>
> Thanks for testing, I will commit the changes soon.
>
> > However, output-sync, recursion, MAKE remain.  Included work.tar.xz
>
> All of those failures are because of the exact form of $(MAKE).  What
> Cygwin produces is functionally equivalent to what the test suite
> expects, so I think we can consider these failures redundant, if not
> bogus.  (If Paul accepts your patch to the suite, they will be gone
> altogether.)

Personally I think this is a real bug.  Make should try to use the
fully-qualified pathname when it sets the MAKE variable, which is why
the test suite prefixes the filename with the current working directory
if the path is determined to be relative.

If MAKE is set to a relative pathname, then it will fail in recursion
when the directory is changed, since $(MAKE) will no longer be a valid
path.

Won't it?


_______________________________________________
Make-w32 mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/make-w32
Reply | Threaded
Open this post in threaded view
|

Re: Windows patches for the next release

Eli Zaretskii
> From: Paul Smith <[hidden email]>
> Cc: Denis Excoffier <[hidden email]>,
>  [hidden email],  [hidden email], [hidden email]
> Date: Sat, 05 Oct 2013 20:05:03 -0400
>
> > > However, output-sync, recursion, MAKE remain.  Included work.tar.xz
> >
> > All of those failures are because of the exact form of $(MAKE).  What
> > Cygwin produces is functionally equivalent to what the test suite
> > expects, so I think we can consider these failures redundant, if not
> > bogus.  (If Paul accepts your patch to the suite, they will be gone
> > altogether.)
>
> Personally I think this is a real bug.  Make should try to use the
> fully-qualified pathname when it sets the MAKE variable, which is why
> the test suite prefixes the filename with the current working directory
> if the path is determined to be relative.
>
> If MAKE is set to a relative pathname, then it will fail in recursion
> when the directory is changed, since $(MAKE) will no longer be a valid
> path.
>
> Won't it?

Once again, I think this is the issue with "." being on PATH, which is
always the case on Windows.  If you add "." to PATH, Posix Make fails
as well.

We need a general solution here.

OTOH, since I cannot build and debug the Cygwin port, perhaps I'm
missing something here.  It would be nice if someone who actually uses
Cygwin could run Make under a debugger in these cases, and see why you
get "../make" instead of an absolute file name.

_______________________________________________
Make-w32 mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/make-w32
Reply | Threaded
Open this post in threaded view
|

Re: Windows patches for the next release

Paul Smith-20
On Sun, 2013-10-06 at 05:52 +0300, Eli Zaretskii wrote:

> > From: Paul Smith <[hidden email]>
> > Cc: Denis Excoffier <[hidden email]>,
> >  [hidden email],  [hidden email], [hidden email]
> > Date: Sat, 05 Oct 2013 20:05:03 -0400
> >
> > > > However, output-sync, recursion, MAKE remain.  Included work.tar.xz
> > >
> > > All of those failures are because of the exact form of $(MAKE).  What
> > > Cygwin produces is functionally equivalent to what the test suite
> > > expects, so I think we can consider these failures redundant, if not
> > > bogus.  (If Paul accepts your patch to the suite, they will be gone
> > > altogether.)
> >
> > Personally I think this is a real bug.  Make should try to use the
> > fully-qualified pathname when it sets the MAKE variable, which is why
> > the test suite prefixes the filename with the current working directory
> > if the path is determined to be relative.
>
> Once again, I think this is the issue with "." being on PATH, which is
> always the case on Windows.  If you add "." to PATH, Posix Make fails
> as well.

I think this isn't the same problem.  That's a very special case, where
argv[0] has no directory separator at all.  In that case probably the
ultimately right thing to do is search PATH looking for the
fully-qualified pathname and, if THAT is relative, prepend the current
directory.  But that's not what we do and I don't have a problem with
our current behavior: if you're going to add "." to your PATH then
obviously when you change directories programs may not be found anymore.
Adding "." to your PATH is a very bad idea for all sorts of reasons.

In this case, argv[0] has a separator but is not absolute.  That's
handled differently in the code in main.c: in that case we prepend the
current working directory to the existing path.  So if you're
in /tmp/foo and you run "../make" then MAKE will be set to
"/tmp/foo/../make".  That way if you recurse into a different directory,
MAKE will still be the correct valid pathname.


_______________________________________________
Make-w32 mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/make-w32