Patchwork zstd: fix compilation with Solaris Studio

login
register
mail settings
Submitter Danek Duvall
Date Nov. 22, 2016, 9:34 p.m.
Message ID <b09fb7f66e9e680358b8.1479850462@smelly>
Download mbox | patch
Permalink /patch/17714/
State Accepted
Headers show

Comments

Danek Duvall - Nov. 22, 2016, 9:34 p.m.
# HG changeset patch
# User Danek Duvall <danek.duvall@oracle.com>
# Date 1479850325 28800
#      Tue Nov 22 13:32:05 2016 -0800
# Node ID b09fb7f66e9e680358b8fb359be24a14fd6b3cfb
# Parent  8836f13e3c5b8eae765372708b659c55a044cbb4
zstd: fix compilation with Solaris Studio

Without these changes, Solaris Studio (12.4) gives us "syntax error: empty
declaration" on these two lines.
Gregory Szorc - Nov. 22, 2016, 10:07 p.m.
On Tue, Nov 22, 2016 at 1:34 PM, <danek.duvall@oracle.com> wrote:

> # HG changeset patch
> # User Danek Duvall <danek.duvall@oracle.com>
> # Date 1479850325 28800
> #      Tue Nov 22 13:32:05 2016 -0800
> # Node ID b09fb7f66e9e680358b8fb359be24a14fd6b3cfb
> # Parent  8836f13e3c5b8eae765372708b659c55a044cbb4
> zstd: fix compilation with Solaris Studio
>
> Without these changes, Solaris Studio (12.4) gives us "syntax error: empty
> declaration" on these two lines.
>

Doh.

This patch LGTM. I've applied this upstream as well and it will be in the
next python-zstandard release. The canonical upstream is
https://github.com/indygreg/python-zstandard by the way (although I haven't
pushed the fix there yet).


>
> diff --git a/contrib/python-zstandard/c-ext/compressionparams.c
> b/contrib/python-zstandard/c-ext/compressionparams.c
> --- a/contrib/python-zstandard/c-ext/compressionparams.c
> +++ b/contrib/python-zstandard/c-ext/compressionparams.c
> @@ -136,7 +136,7 @@ static void CompressionParameters_deallo
>
>  static Py_ssize_t CompressionParameters_length(PyObject* self) {
>         return 7;
> -};
> +}
>
>  static PyObject* CompressionParameters_item(PyObject* o, Py_ssize_t i) {
>         CompressionParametersObject* self = (CompressionParametersObject*)
> o;
> diff --git a/contrib/python-zstandard/c-ext/dictparams.c
> b/contrib/python-zstandard/c-ext/dictparams.c
> --- a/contrib/python-zstandard/c-ext/dictparams.c
> +++ b/contrib/python-zstandard/c-ext/dictparams.c
> @@ -42,7 +42,7 @@ static void DictParameters_dealloc(PyObj
>
>  static Py_ssize_t DictParameters_length(PyObject* self) {
>         return 4;
> -};
> +}
>
>  static PyObject* DictParameters_item(PyObject* o, Py_ssize_t i) {
>         DictParametersObject* self = (DictParametersObject*)o;
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Danek Duvall - Nov. 22, 2016, 10:19 p.m.
Gregory Szorc wrote:

> On Tue, Nov 22, 2016 at 1:34 PM, <danek.duvall@oracle.com> wrote:
> 
> > # HG changeset patch
> > # User Danek Duvall <danek.duvall@oracle.com>
> > # Date 1479850325 28800
> > #      Tue Nov 22 13:32:05 2016 -0800
> > # Node ID b09fb7f66e9e680358b8fb359be24a14fd6b3cfb
> > # Parent  8836f13e3c5b8eae765372708b659c55a044cbb4
> > zstd: fix compilation with Solaris Studio
> >
> > Without these changes, Solaris Studio (12.4) gives us "syntax error: empty
> > declaration" on these two lines.
> >
> 
> Doh.
> 
> This patch LGTM. I've applied this upstream as well and it will be in the
> next python-zstandard release. The canonical upstream is
> https://github.com/indygreg/python-zstandard by the way (although I haven't
> pushed the fix there yet).

Cool.  FWIW, the mercurial build didn't fail when I got this failure, which
I found a bit surprising.  I get the desire to make sure that the zstd
support isn't critical, but if I hadn't known to go look for this, I would
have missed it entirely until at some point I checked to see that the zstd
bits were, in fact, being delivered.  There's probably a happy medium
somewhere.

Thanks,
Danek
Gregory Szorc - Nov. 23, 2016, 1:59 a.m.
On Tue, Nov 22, 2016 at 2:19 PM, Danek Duvall <danek.duvall@oracle.com>
wrote:

> Gregory Szorc wrote:
>
> > On Tue, Nov 22, 2016 at 1:34 PM, <danek.duvall@oracle.com> wrote:
> >
> > > # HG changeset patch
> > > # User Danek Duvall <danek.duvall@oracle.com>
> > > # Date 1479850325 28800
> > > #      Tue Nov 22 13:32:05 2016 -0800
> > > # Node ID b09fb7f66e9e680358b8fb359be24a14fd6b3cfb
> > > # Parent  8836f13e3c5b8eae765372708b659c55a044cbb4
> > > zstd: fix compilation with Solaris Studio
> > >
> > > Without these changes, Solaris Studio (12.4) gives us "syntax error:
> empty
> > > declaration" on these two lines.
> > >
> >
> > Doh.
> >
> > This patch LGTM. I've applied this upstream as well and it will be in the
> > next python-zstandard release. The canonical upstream is
> > https://github.com/indygreg/python-zstandard by the way (although I
> haven't
> > pushed the fix there yet).
>
> Cool.  FWIW, the mercurial build didn't fail when I got this failure, which
> I found a bit surprising.  I get the desire to make sure that the zstd
> support isn't critical, but if I hadn't known to go look for this, I would
> have missed it entirely until at some point I checked to see that the zstd
> bits were, in fact, being delivered.  There's probably a happy medium
> somewhere.
>

Uhh - that shouldn't happen!

distutils does support optional extensions, but we don't have that flag set
in setup.py. When I `make local` or `python setup.py build_ext` with a
"bad" .c file from python-zstandard/c-ext, I get an "error: command 'gcc'
failed with exit status 1" and setup.py aborts. I'm really curious what
your machine is up to!
Yuya Nishihara - Nov. 23, 2016, 2:45 a.m.
On Tue, 22 Nov 2016 14:07:51 -0800, Gregory Szorc wrote:
> On Tue, Nov 22, 2016 at 1:34 PM, <danek.duvall@oracle.com> wrote:
> > # HG changeset patch
> > # User Danek Duvall <danek.duvall@oracle.com>
> > # Date 1479850325 28800
> > #      Tue Nov 22 13:32:05 2016 -0800
> > # Node ID b09fb7f66e9e680358b8fb359be24a14fd6b3cfb
> > # Parent  8836f13e3c5b8eae765372708b659c55a044cbb4
> > zstd: fix compilation with Solaris Studio
> >
> > Without these changes, Solaris Studio (12.4) gives us "syntax error: empty
> > declaration" on these two lines.
> >
> 
> Doh.
> 
> This patch LGTM. I've applied this upstream as well and it will be in the
> next python-zstandard release. The canonical upstream is
> https://github.com/indygreg/python-zstandard by the way (although I haven't
> pushed the fix there yet).

Queued this, thanks.

Just for reference, this kind of error can be found by CFLAGS=-pedantic with
gcc.
Danek Duvall - Nov. 23, 2016, 2:54 a.m.
Gregory Szorc wrote:

> > Cool.  FWIW, the mercurial build didn't fail when I got this failure, which
> > I found a bit surprising.  I get the desire to make sure that the zstd
> > support isn't critical, but if I hadn't known to go look for this, I would
> > have missed it entirely until at some point I checked to see that the zstd
> > bits were, in fact, being delivered.  There's probably a happy medium
> > somewhere.
> >
> 
> Uhh - that shouldn't happen!
> 
> distutils does support optional extensions, but we don't have that flag set
> in setup.py. When I `make local` or `python setup.py build_ext` with a
> "bad" .c file from python-zstandard/c-ext, I get an "error: command 'gcc'
> failed with exit status 1" and setup.py aborts. I'm really curious what
> your machine is up to!

Oh.  Because it's a *warning*, so the exit code is 0.  Here's the full
error:

    ".../compressionparams.c", line 139: warning: syntax error:  empty declaration

The module didn't show up in my manifest because the new manifest wasn't
generated due to (hopefully unrelated) test failures.

So uh, the patch isn't critical, but we could still do without the warning.

Now to figure out the test failures.  :-/  I should really get that
buildbot instance up so it's not all on me.  :)

Thanks,
Danek
Pierre-Yves David - Jan. 9, 2017, 10:26 a.m.
On 11/23/2016 03:45 AM, Yuya Nishihara wrote:
> On Tue, 22 Nov 2016 14:07:51 -0800, Gregory Szorc wrote:
>> On Tue, Nov 22, 2016 at 1:34 PM, <danek.duvall@oracle.com> wrote:
>>> # HG changeset patch
>>> # User Danek Duvall <danek.duvall@oracle.com>
>>> # Date 1479850325 28800
>>> #      Tue Nov 22 13:32:05 2016 -0800
>>> # Node ID b09fb7f66e9e680358b8fb359be24a14fd6b3cfb
>>> # Parent  8836f13e3c5b8eae765372708b659c55a044cbb4
>>> zstd: fix compilation with Solaris Studio
>>>
>>> Without these changes, Solaris Studio (12.4) gives us "syntax error: empty
>>> declaration" on these two lines.
>>>
>>
>> Doh.
>>
>> This patch LGTM. I've applied this upstream as well and it will be in the
>> next python-zstandard release. The canonical upstream is
>> https://github.com/indygreg/python-zstandard by the way (although I haven't
>> pushed the fix there yet).
>
> Queued this, thanks.
>
> Just for reference, this kind of error can be found by CFLAGS=-pedantic with
> gcc.

Should we run our continuous integration build (or even default build) 
with that option to catch such errors earlier?

Cheers,
Yuya Nishihara - Jan. 9, 2017, 12:58 p.m.
On Mon, 9 Jan 2017 11:26:23 +0100, Pierre-Yves David wrote:
> On 11/23/2016 03:45 AM, Yuya Nishihara wrote:
> > On Tue, 22 Nov 2016 14:07:51 -0800, Gregory Szorc wrote:
> >> On Tue, Nov 22, 2016 at 1:34 PM, <danek.duvall@oracle.com> wrote:
> >>> # HG changeset patch
> >>> # User Danek Duvall <danek.duvall@oracle.com>
> >>> # Date 1479850325 28800
> >>> #      Tue Nov 22 13:32:05 2016 -0800
> >>> # Node ID b09fb7f66e9e680358b8fb359be24a14fd6b3cfb
> >>> # Parent  8836f13e3c5b8eae765372708b659c55a044cbb4
> >>> zstd: fix compilation with Solaris Studio
> >>>
> >>> Without these changes, Solaris Studio (12.4) gives us "syntax error: empty
> >>> declaration" on these two lines.
> >>
> >> Doh.
> >>
> >> This patch LGTM. I've applied this upstream as well and it will be in the
> >> next python-zstandard release. The canonical upstream is
> >> https://github.com/indygreg/python-zstandard by the way (although I haven't
> >> pushed the fix there yet).
> >
> > Queued this, thanks.
> >
> > Just for reference, this kind of error can be found by CFLAGS=-pedantic with
> > gcc.
> 
> Should we run our continuous integration build (or even default build) 
> with that option to catch such errors earlier?

This wasn't actually an error so the CI might help catching compiler warnings
but it wouldn't that important.

Patch

diff --git a/contrib/python-zstandard/c-ext/compressionparams.c b/contrib/python-zstandard/c-ext/compressionparams.c
--- a/contrib/python-zstandard/c-ext/compressionparams.c
+++ b/contrib/python-zstandard/c-ext/compressionparams.c
@@ -136,7 +136,7 @@  static void CompressionParameters_deallo
 
 static Py_ssize_t CompressionParameters_length(PyObject* self) {
 	return 7;
-};
+}
 
 static PyObject* CompressionParameters_item(PyObject* o, Py_ssize_t i) {
 	CompressionParametersObject* self = (CompressionParametersObject*)o;
diff --git a/contrib/python-zstandard/c-ext/dictparams.c b/contrib/python-zstandard/c-ext/dictparams.c
--- a/contrib/python-zstandard/c-ext/dictparams.c
+++ b/contrib/python-zstandard/c-ext/dictparams.c
@@ -42,7 +42,7 @@  static void DictParameters_dealloc(PyObj
 
 static Py_ssize_t DictParameters_length(PyObject* self) {
 	return 4;
-};
+}
 
 static PyObject* DictParameters_item(PyObject* o, Py_ssize_t i) {
 	DictParametersObject* self = (DictParametersObject*)o;