After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 761072 - Various build fixes
Various build fixes
Status: RESOLVED FIXED
Product: gjs
Classification: Bindings
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: Philip Chimento
gjs-maint
Depends on:
Blocks:
 
 
Reported: 2016-01-25 03:14 UTC by Philip Chimento
Modified: 2016-02-10 07:16 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
coverage: Don't mangle internal coverage symbols (1.08 KB, patch)
2016-01-25 03:14 UTC, Philip Chimento
committed Details | Review
build: Untangle (AM_) CFLAGS, CPPFLAGS, CXXFLAGS (5.80 KB, patch)
2016-01-25 03:14 UTC, Philip Chimento
committed Details | Review
tests: Don't assume localeCompare() returns +/-1 (1.62 KB, patch)
2016-01-25 03:15 UTC, Philip Chimento
committed Details | Review
coverage test: Don't use %as specifier in sscanf() (3.57 KB, patch)
2016-01-25 03:15 UTC, Philip Chimento
committed Details | Review
tests: Fallback for uuidgen -r flag (1.96 KB, patch)
2016-01-25 03:15 UTC, Philip Chimento
none Details | Review
tests: Add option not to use DBus (2.92 KB, patch)
2016-01-25 03:15 UTC, Philip Chimento
committed Details | Review
tests: Add option not to use Xvfb (3.66 KB, patch)
2016-01-25 03:15 UTC, Philip Chimento
committed Details | Review
tests: Fallback for uuidgen -r flag (2.04 KB, patch)
2016-02-10 04:33 UTC, Philip Chimento
committed Details | Review

Description Philip Chimento 2016-01-25 03:14:50 UTC
Here are 7 patches to make the build system and the tests more portable
so that I can build and run GJS on OS X.
Comment 1 Philip Chimento 2016-01-25 03:14:54 UTC
Created attachment 319648 [details] [review]
coverage: Don't mangle internal coverage symbols

These symbols need to be referenced when linking to the coverage test
program, so surround them with G_BEGIN_DECLS/G_END_DECLS so that they
don't get mangled.
Comment 2 Philip Chimento 2016-01-25 03:14:58 UTC
Created attachment 319649 [details] [review]
build: Untangle (AM_) CFLAGS, CPPFLAGS, CXXFLAGS

In Automake, CPPFLAGS stands for C PreProcessor flags. C source files are
compiled with CPPFLAGS and CFLAGS, while C++ source files are compiled
with CPPFLAGS and CXXFLAGS. The exception to this naming convention is
the PKG_CHECK_MODULES macro which substs a variable named <FOO>_CFLAGS
which should properly be called <FOO>_CPPFLAGS.

This untangles all the flags definitions so that -I and -D flags, which
are preprocessor arguments, are put into CPPFLAGS variables, not into
CFLAGS or CXXFLAGS. Otherwise they won't apply to both C and C++ source
files.

In addition, when using per-target flags, make sure to include the AM_
prefixed variable as well, because otherwise per-target flags will
override it. Conversely, if an assignment sets a per-target flags variable
to only be the AM_ prefixed variable (e.g., "something_CFLAGS =
$(AM_CFLAGS)") then it is redundant.
Comment 3 Philip Chimento 2016-01-25 03:15:02 UTC
Created attachment 319650 [details] [review]
tests: Don't assume localeCompare() returns +/-1

Per https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/localeCompare,
"DO NOT rely on exact return values of -1 or 1. Negative and positive
integer results vary between browsers (as well as between browser
versions) because the W3C specification only mandates negative and
positive values. Some browsers may return -2 or 2 or even some other
negative or positive value."

And indeed I observed this test fail with a value of -2.
Comment 4 Philip Chimento 2016-01-25 03:15:05 UTC
Created attachment 319651 [details] [review]
coverage test: Don't use %as specifier in sscanf()

%as was a GNU extension for automatically allocating a buffer big enough
to hold whatever string sscanf() found. In C99 %a was defined to be an
alias of %f, so %as as a GNU extension was deprecated and made unusable.
It was replaced by another GNU extension, %ms.

However, in the interests of being portable to e.g. BSD, this commit
avoids using either %as or %ms and instead pre-allocates buffers of at
least the correct sizes and passes them to sscanf(), using plain old %s.
Comment 5 Philip Chimento 2016-01-25 03:15:09 UTC
Created attachment 319652 [details] [review]
tests: Fallback for uuidgen -r flag

In order to run the tests under DBus a UUID is generated using
"uuidgen -r". The -r flag isn't standard, though; neither BSD's nor
Apple's implementations support it. Check in configure.ac whether the
flag is supported and fall back if not.
Comment 6 Philip Chimento 2016-01-25 03:15:14 UTC
Created attachment 319653 [details] [review]
tests: Add option not to use DBus

This adds a --without-dbus-tests configure option, which allows you to
skip the /js/GDBus test if you don't need DBus. The default is still to
use DBus.
Comment 7 Philip Chimento 2016-01-25 03:15:18 UTC
Created attachment 319654 [details] [review]
tests: Add option not to use Xvfb

Normally the tests are run under Xvfb because of needing to call
gtk_init(); in a headless build environment this will fail if there is no
X server.

However, requiring Xvfb doesn't make sense if using GTK with other
backends than X. This adds a configure option to specify whether the
tests should or shouldn't be run under Xvfb. The default is not to use
Xvfb, which might mean you need to configure your headless build
environment to use --with-xvfb-tests.
Comment 8 Cosimo Cecchi 2016-02-09 17:48:02 UTC
Review of attachment 319648 [details] [review]:

Looks good.
Comment 9 Cosimo Cecchi 2016-02-09 17:49:58 UTC
Review of attachment 319649 [details] [review]:

OK
Comment 10 Cosimo Cecchi 2016-02-09 17:50:59 UTC
Review of attachment 319650 [details] [review]:

Looks good.
Comment 11 Cosimo Cecchi 2016-02-09 17:53:10 UTC
Review of attachment 319651 [details] [review]:

Looks good.
Comment 12 Cosimo Cecchi 2016-02-09 17:55:30 UTC
Review of attachment 319653 [details] [review]:

Looks good.
Comment 13 Cosimo Cecchi 2016-02-09 17:56:21 UTC
Review of attachment 319652 [details] [review]:

Where exactly do you set the fallback in this patch?
Comment 14 Cosimo Cecchi 2016-02-09 17:57:28 UTC
Review of attachment 319654 [details] [review]:

Looks good.
Comment 15 Philip Chimento 2016-02-10 04:29:16 UTC
Attachment 319648 [details] pushed as 8f94e96 - coverage: Don't mangle internal coverage symbols
Attachment 319649 [details] pushed as 0f92f6b - build: Untangle (AM_) CFLAGS, CPPFLAGS, CXXFLAGS
Attachment 319650 [details] pushed as 1b418af - tests: Don't assume localeCompare() returns +/-1
Attachment 319651 [details] pushed as 684fd76 - coverage test: Don't use %as specifier in sscanf()
Attachment 319653 [details] pushed as 7b4e019 - tests: Add option not to use DBus
Attachment 319654 [details] pushed as 15ed6a6 - tests: Add option not to use Xvfb
Comment 16 Philip Chimento 2016-02-10 04:33:29 UTC
Created attachment 320764 [details] [review]
tests: Fallback for uuidgen -r flag

In order to run the tests under DBus a UUID is generated using
"uuidgen -r". The -r flag isn't standard, though; neither BSD's nor
Apple's implementations support it. Check in configure.ac whether the
flag is supported and fall back if not.
Comment 17 Philip Chimento 2016-02-10 04:35:23 UTC
(In reply to Cosimo Cecchi from comment #13)
> Review of attachment 319652 [details] [review] [review]:
> 
> Where exactly do you set the fallback in this patch?

The fallback is set by AC_PATH_PROG([UUIDGEN], [uuidgen]), which sets the UUIDGEN variable to /path/to/uuidgen with no arguments. After that the -r argument is added only if this uuidgen implementation supports it.

I've reuploaded this patch rebased on the others that I committed.
Comment 18 Cosimo Cecchi 2016-02-10 06:52:02 UTC
Review of attachment 320764 [details] [review]:

Makes sense - looks good.
Comment 19 Philip Chimento 2016-02-10 07:16:29 UTC
Attachment 320764 [details] pushed as 243002d - tests: Fallback for uuidgen -r flag