Re: gnulib integration how to

[Paul Eggert]

On 11/15/11 18:48, Bruno Haible wrote:
> I'm running out of time on this now; someone else can comment, please?

I agree with your comment about it needing a date and version info.

Also, it'd be nice for it to have a "References" or "See also"
section at the bottom, that points to the gnulib manual,
the gnulib mailing list archive, and download instructions.

I just took a census and found the following license count
for gnulib modules:

775 LGPL
398 LGPLv2+
234 GPL
 25 GPLed build tool
 17 unlimited
  4 public domain

so, although the comment about most being *GPL3 is correct,
there's still a lot of LGPLv2+ code in there and some
gnulib-using projects are LGPLv2 so I would reword the
licensing section a bit to make it a bit less discouraging.

I looked for other similar pages, that the gnulib documentation
could point to.  The OpenCSW page seemed by far the best.

Re: signbit #define'd causes compilation error in octave

[Philipp Thomas]

* Eric Blake (ebl...@redhat.com) [20111018 23:07]:

> But since octave is written in C++, we could avoid the macros and
> instead have three overloaded functions named signbit which operate
> on the correct types, so as not to pollute the namespace with a
> macro. 

That's why the C++ standard has replacement headers for a few standard C
headers, exactly to replace macros with real functions.  Alas, AFAIK MS
didn't implement that part of the standard.

Philipp

Re: openat requires lstat.m4

[Ben Walton]

Excerpts from Bruno Haible's message of Tue Nov 15 21:12:06 -0500 2011:

> I'm applying this. The "tiny change" is not meant to be a derogatory
> designation; it merely documents that your contribution to gnulib so
> far did not require the exchange of copyright assignment paperwork.

Perfect.  Thanks for that.

-Ben
--
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302

FYI, new, rare failure on rawhide

[Jim Meyering]

FYI, I'm seeing this in a rawhide VM using ext4, but only twice,
and since then 20 trials with no failure.

FAIL: test-fdutimensat (exit: 134)
==

test-utimens.h:109: assertion failed

It never fails when run by itself.
I.e., the failure happens only when running many in parallel via e.g.,
make -j20 check -C gnulib-tests

I'll add some instrumentation eventually.

Here's the relevant code.  Line 109 is the final ASSERT:

  /* Set both times.  */
  {
struct timespec ts[2] = { { Y2K, BILLION / 2 - 1 }, { Y2K, BILLION - 1 } };
ASSERT (func (BASE "file", ts) == 0);
ASSERT (stat (BASE "file", &st2) == 0);
ASSERT (st2.st_atime == Y2K);
ASSERT (0 <= get_stat_atime_ns (&st2));
ASSERT (get_stat_atime_ns (&st2) < BILLION / 2);
ASSERT (st2.st_mtime == Y2K);
ASSERT (0 <= get_stat_mtime_ns (&st2));
ASSERT (get_stat_mtime_ns (&st2) < BILLION);
if (check_ctime)
  ASSERT (st1.st_ctime < st2.st_ctime
  || (st1.st_ctime == st2.st_ctime
  && get_stat_ctime_ns (&st1) < get_stat_ctime_ns (&st2)));
  }

Re: FYI, new, rare failure on rawhide

[Paul Eggert]

Possibly a bug in the file system or kernel?
An earlier part of the code does this:

/* lines 57 and 58 of test-utimens.h */
ASSERT (func (BASE "file", ts) == 0);
ASSERT (stat (BASE "file", &st1) == 0);

and this relies on st_ctime being marked for update in the first
line, and being actually updated before the second line takes effect.
This is pretty easy to get wrong, and many file systems do get similar
things wrong, particularly under high load.  If my guess is right,
it should be easy to verify by instrumenting those lines.

There is a similar issue here:

/* lines 98 and 99 (/
ASSERT (func (BASE "file", ts) == 0);
ASSERT (stat (BASE "file", &st2) == 0);

Of course this is just a long-distance guess

Re: [PATCH 1/2] gitlog-to-changelog: support multi-author commits.

[Jim Meyering]

Gary V. Vaughan wrote:
...
> the parts that didn't work OOTB on my Mac to be portable.  Feel free to crib
> those portable parts of this one into coreutils, or reformat this one to
> coreutils style as you prefer.
>
>> An alternative is to accept anything after the ":" and then, to use
>> "s/\s*>
>>> +  s/^Co-authored-by:\s*([^<]+)\s+>
>> Please use $1, not \1.
>
> I thought $1 was a positional parameter?  I'm not sure what the advantage of
> choosing a different back-reference syntax to other unix regexp using tools
> is, but no matter... after taking your other suggestions, this line doesn't
> exist any more :)

$1 is preferred in this context.
I'm pretty sure Perl style guides say that, too.

...
> Okay to push the following?

Almost.

> From 73131d956d8da994bae5f59e321548ba26ddd566 Mon Sep 17 00:00:00 2001
> From: "Gary V. Vaughan" 
> Date: Tue, 1 Nov 2011 17:58:37 +0700
> Subject: [PATCH 1/2] gitlog-to-changelog: support multi-author commits.
...
> +  my @coauthors = grep /^Co-authored-by:.*$/, @line;
> +  for (@coauthors)
> +{
> +  s/^Co-authored-by:\s*/\t/;
> +  s/\s* +
> +  $_ =~ /<.*@.*\..*>/
> +or warn "$ME: warning: Missing email address for "
> +  . substr ($_, 5) . ".\n";

There's no point in using $_ there.
You didn't just above, either, so removing it makes this consistent.
(preferred style, too, though in 95% of foreach loops I write I do
not use $_, due to the risk -- didn't seem like worth trying to
work around in this case)

I also changed the first ".*" to ".*?", mostly on reflex.

No capital "M", and no period at end of diagnostic.

Apply the following and you may push the result,
but without the commit-msg script addition.

diff --git a/build-aux/gitlog-to-changelog b/build-aux/gitlog-to-changelog
index 9d98b82..f5f6023 100755
--- a/build-aux/gitlog-to-changelog
+++ b/build-aux/gitlog-to-changelog
@@ -259,9 +259,9 @@ sub parse_amend_file($)
   s/^Co-authored-by:\s*/\t/;
   s/\s*/
-or warn "$ME: warning: Missing email address for "
-  . substr ($_, 5) . ".\n";
+  /<.*?@.*\..*>/
+or warn "$ME: warning: missing email address for "
+  . substr ($_, 5) . "\n";
 }

   # If this header would be the same as the previous date/name/email/

...
> diff --git a/scripts/git-hooks/commit-msg b/scripts/git-hooks/commit-msg
> new file mode 100755
> index 000..5efdf1f
> --- /dev/null
> +++ b/scripts/git-hooks/commit-msg
> @@ -0,0 +1,85 @@
> +#!/bin/sh
> +# An example hook script for catching duplicate or malformed
> +# Co-authored-by lines in the commit message.

Re: gnulib integration how to

[Ben Walton]

Excerpts from Bruno Haible's message of Tue Nov 15 21:48:30 -0500 2011:

Hi Bruno,

> Two things:
> 
> 1) From the perspective of a distributor of packages for a specific
>platform, the libposix subproject of gnulib (see
>
> 
>and gnulib/STATUS-libposix) should be of interest to you.
>In the long run, this will be more economic for you than modifying
>the configuration of dozens of packages.

Yes, this does look very appealing.  I'll see what I can do to
leverage this.

> 2) Your writeup has ca. 80% overlap with the gnulib documentation,
>10% is specific to OpenCSW, and 10% is generic text not yet handled
>by the gnulib documentation.
> 
>What is, in general, the value of a smaller documentation when a more
>extensive documentation already exists? Answer: The fact that it is
>smaller. Faster to read. So, there is a value in linking from the gnulib
>manual to your writeup.

Certainly.  It's not meant to be exhaustive but rather to get people
up and running quickly.  It's a distillation of the larger
documentation that captures the parts I found important and
pertinent.

>But if we have a GNU package's documentation that starts to link
>to related doc pieces, it will be likely that some of these
>linked-to docs become out-of-date. So, before our manual can link
>to your doc, your text should make it prominently clear when it
>was last updated. IMO the currently location of that hint (near
>the bottom, between a Google ad and a Wikidot ad, with a tiny
>font) is not sufficient.  The right place for such a note is
>between the title and the body.

Ok.  (Sorry about those ads.  I'm always logged in and forget that
most people get those.)  I've added a Last Updated and Version tag to
the top of the document.  I'll update these as I make changes.

>Other than that, the idea of section "Knowing When gnulib is
>Helpful" could be helpful for the Gnulib section "2.1 Finding
>modules". But that section should also mention GNULIB_POSIXCHECK,
>since that is the preferred way of finding which gnulib modules
>are helpful (with this method, you don't need e.g. a HP-UX
>machine to find which modules are needed for HP-UX).

Ok.  I'll see about a patch for the docs in this area too.

Your feedback is appreciated.

Thanks
-Ben
--
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302

Re: gnulib integration how to

[Ben Walton]

Excerpts from Paul Eggert's message of Wed Nov 16 03:10:40 -0500 2011:

> I agree with your comment about it needing a date and version info.

Added.  I'll maintain this going forward.

> Also, it'd be nice for it to have a "References" or "See also"
> section at the bottom, that points to the gnulib manual, the gnulib
> mailing list archive, and download instructions.

Will add.

> I just took a census and found the following license count
> for gnulib modules:
> 
> 775 LGPL
> 398 LGPLv2+
> 234 GPL
>  25 GPLed build tool
>  17 unlimited
>   4 public domain
> 
> so, although the comment about most being *GPL3 is correct, there's
> still a lot of LGPLv2+ code in there and some gnulib-using projects
> are LGPLv2 so I would reword the licensing section a bit to make it
> a bit less discouraging.

Ok, that sounds good to me.

> I looked for other similar pages, that the gnulib documentation
> could point to.  The OpenCSW page seemed by far the best.

Glad to hear it! :)

I'll bump this thread again after making the suggested changes.

Thanks
-Ben
--
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302

Re: FYI, new, rare failure on rawhide

[Jim Meyering]

Paul Eggert wrote:
> Possibly a bug in the file system or kernel?
> An earlier part of the code does this:
>
> /* lines 57 and 58 of test-utimens.h */
> ASSERT (func (BASE "file", ts) == 0);
> ASSERT (stat (BASE "file", &st1) == 0);
>
> and this relies on st_ctime being marked for update in the first
> line, and being actually updated before the second line takes effect.
> This is pretty easy to get wrong, and many file systems do get similar
> things wrong, particularly under high load.  If my guess is right,
> it should be easy to verify by instrumenting those lines.
>
> There is a similar issue here:
>
> /* lines 98 and 99 (/
> ASSERT (func (BASE "file", ts) == 0);
> ASSERT (stat (BASE "file", &st2) == 0);
>
> Of course this is just a long-distance guess

I think it comes down to either a bug, or
that somehow "nap()" is not sleeping long enough.

Details on how this file system is mounted:
(it's SSD-backed in a rawhide guest, 3.2.0-0.rc1.git4.1.fc17.x86_64)

  $ grep vda4 /proc/mounts
  /dev/vda4 / ext4 rw,seclabel,relatime,user_xattr,barrier=1,data=ordered 0 0

There seem to be at least three points at which I'm seeing failed assertions.
I caught this by changing the abort() in macros.h's ASSERT definition to
be sleep(999) and by running parallel "make check" jobs until one hung.
Then attached with gdb:

#2  0x00403c28 in test_lutimens (func=0x401e10 ,
print=false) at test-lutimens.h:66
(gdb) l
61}
62ASSERT (stat (BASE "file", &st2) == 0);
63ASSERT (st2.st_atime == Y2K);
64ASSERT (st2.st_mtime == Y2K);
65if (check_ctime)
66  ASSERT (st1.st_ctime < st2.st_ctime
67  || (st1.st_ctime == st2.st_ctime
68  && get_stat_ctime_ns (&st1) < get_stat_ctime_ns 
(&st2)));
69
70/* Play with symlink timestamps.  */

(gdb) p st1.st_ctim
$1 = {
  tv_sec = 1321473919,
  tv_nsec = 914482118
}
(gdb) p st2.st_ctim
$2 = {
  tv_sec = 1321473919,
  tv_nsec = 914482118
}

Equal structs.  Definitely indicates failure, somewhere.

==
Trying again, I hit a sleep on the 8th trial.
Note, it's in a different file and with different var names,
but the same condition: unchanged ctime.  This time, I was using a
40-millisecond delay, just in case 20 was close but not enough.

#2  0x00402ad3 in test_utimens (func=0x401e50 , print=false)
at test-utimens.h:135
135   ASSERT (st2.st_ctime < st3.st_ctime
(gdb) p st2.st_ctim
$1 = {
  tv_sec = 1321476466,
  tv_nsec = 27092765
}
(gdb) p st3.st_ctim
$2 = {
  tv_sec = 1321476466,
  tv_nsec = 27092765
}

==
I did it one more time:

#2  0x00405153 in test_futimens (print=,
func=0x402930 ) at test-futimens.h:148
148   ASSERT (st2.st_ctime < st3.st_ctime
(gdb) p st2.st_ctim
$1 = {
  tv_sec = 1321478037,
  tv_nsec = 577808187
}
(gdb) p st3.st_ctim
$2 = {
  tv_sec = 1321478037,
  tv_nsec = 577808187
}

Then examined the ctime of the actual file:

$ stat --fo=%.Z test-utimens.tfile
1321478037.577808187

Yes, it matches.  This was also with the 40-millisecond delay.
This time it took 65 iterations.

Tomorrow I'll write a better test.
I suspect that "make check" is not exercising it thoroughly enough.

Re: FYI, new, rare failure on rawhide

[Paul Eggert]

The data that you gave are consistent with my guess,
namely, that utimens is marking st_ctime for update,
but that the update doesn't actually occur until
after stat is called (a violation of POSIX).

If my wild guess is right, increasing the nap time could
mask the bug, as the update could occur during the nap.

Re: [PATCH 1/2] gitlog-to-changelog: support multi-author commits.

[Gary V. Vaughan]

Hi Jim,

Thanks for the reviews.

On 17 Nov 2011, at 03:01, Jim Meyering wrote:
> Gary V. Vaughan wrote:
> ...
>> the parts that didn't work OOTB on my Mac to be portable.  Feel free to crib
>> those portable parts of this one into coreutils, or reformat this one to
>> coreutils style as you prefer.
>> 
>>> An alternative is to accept anything after the ":" and then, to use
>>> "s/\s*>> 
 +  s/^Co-authored-by:\s*([^<]+)\s+>> 
>>> Please use $1, not \1.
>> 
>> I thought $1 was a positional parameter?  I'm not sure what the advantage of
>> choosing a different back-reference syntax to other unix regexp using tools
>> is, but no matter... after taking your other suggestions, this line doesn't
>> exist any more :)
> 
> $1 is preferred in this context.
> I'm pretty sure Perl style guides say that, too.

Oh I'm not criticizing gnulib's use of $1 at all... but I do question the
wisdom of Perl adding yet another flavour of regexp syntax.  Still, that's
way off topic, and likely there are many layers of subtlety between my
understanding of Perl and the underlying reasons for a choice that seems
strange on the surface.

> Apply the following and you may push the result,
> but without the commit-msg script addition.
> 
> diff --git a/build-aux/gitlog-to-changelog b/build-aux/gitlog-to-changelog
> index 9d98b82..f5f6023 100755
> --- a/build-aux/gitlog-to-changelog
> +++ b/build-aux/gitlog-to-changelog
> @@ -259,9 +259,9 @@ sub parse_amend_file($)
>   s/^Co-authored-by:\s*/\t/;
>   s/\s* 
> -  $_ =~ /<.*@.*\..*>/
> -or warn "$ME: warning: Missing email address for "
> -  . substr ($_, 5) . ".\n";
> +  /<.*?@.*\..*>/
> +or warn "$ME: warning: missing email address for "
> +  . substr ($_, 5) . "\n";
> }
> 
>   # If this header would be the same as the previous date/name/email/


Done.

Cheers,
-- 
Gary V. Vaughan (gary AT gnu DOT org)