Skip to content

Switch to defining _XOPEN_SOURCE=600 for htscodecs tests#1608

Closed
daviesrob wants to merge 1 commit into
samtools:developfrom
daviesrob:xopen2
Closed

Switch to defining _XOPEN_SOURCE=600 for htscodecs tests#1608
daviesrob wants to merge 1 commit into
samtools:developfrom
daviesrob:xopen2

Conversation

@daviesrob

Copy link
Copy Markdown
Member

Required for gettimeofday() on FreeBSD. This needs to be set on the command line as htscodecs tests currently don't include config.h.

Fixes #1606

This is a minimal fix. A more complete one will require some changes to htscodecs.

Required for gettimeofday() on FreeBSD.  This needs to be set
on the command line as htscodecs tests currently don't include
config.h.
@jkbonfield

Copy link
Copy Markdown
Contributor

The lines with -D_POSIX_C_SOURCE in appeared in 878c71b, but there is no indication in the commit message as to why it's necessary. Certainly on FreeBSD just deleting the definition is sufficient to make it compile without warnings.

While _XOPEN_SOURCE=600 doesn't harm, I'm curious why either of these are required. We ought to get it documented somewhere so we know what system we need to tiptoe around.

@daviesrob

Copy link
Copy Markdown
Member Author

It's needed to get getopt() if building in strict-c99 mode (i.e. -std=c99 -pedantic). Example:

gcc -Wall -g -O3 -std=c99 -pedantic -Wformat=2 -fvisibility=hidden -Werror  -I. -I/root/libdeflate -Ihtscodecs -c -o htscodecs/tests/rANS_static_test.o htscodecs/tests/rANS_static_test.c
htscodecs/tests/rANS_static_test.c: In function 'main':
htscodecs/tests/rANS_static_test.c:110:19: error: implicit declaration of function 'getopt' [-Werror=implicit-function-declaration]
  110 |     while ((opt = getopt(argc, argv, "o:dtr")) != -1) {
      |                   ^~~~~~
cc1: all warnings being treated as errors

@jkbonfield

Copy link
Copy Markdown
Contributor

Arguably you could say if the user is restricting the C specification to a specific hard-line version with extra pedantry, then they also need to adjust C flags. Doing one without the other is just a problem of their own making, although to be fair we don't document the specific standards we require.

Also, why ios that only needed for htscodecs? We use getopt in other places too. Eg:

$ gcc -Wall -g -O2 -std=c99 -pedantic -fvisibility=hidden  -I.  -I/nfs/users/nfs_j/jkb/ftp/compression/libdeflate -I/nfs/users/nfs_j/jkb/scratch/src/zstd/lib -c -o bgzip.o bgzip.c
bgzip.c: In function 'main':
bgzip.c:230:37: warning: implicit declaration of function 'fileno'; did you mean 'fopen'? [-Wimplicit-function-declaration]
         else if (!pstdout && isatty(fileno((FILE *)stdout)) )
                                     ^~~~~~
                                     fopen
bgzip.c:437:24: warning: implicit declaration of function 'strdup'; did you mean 'strcmp'? [-Wimplicit-function-declaration]
                 name = strdup(argv[optind]);
                        ^~~~~~
                        strcmp
bgzip.c:437:22: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
                 name = strdup(argv[optind]);

Adding -D_XOPEN_SOURCE=600 to the flags cures that too. So what's true for one bit of code should also apply to the other, no? This was also my logic behind this comment: https://github.com/samtools/htslib/blob/develop/Makefile#L37

If we wish to accept a build where the user is being overly restrictive demanding C99 and only C99, without permitting XOPEN functions, and we wish to build without warnings, then we would need to amend the Makefile so it's globally defined. This would be preferable to adding it only for htscodecs files. What am I missing here?

@daviesrob

Copy link
Copy Markdown
Member Author

I think the difference is that the htscodecs test sources don't include config.h, while bgzip.c does. So if you do ./configure CFLAGS='-std=c99 -pedantic' _XOPEN_SOURCE gets set by the PTHREAD_MUTEX_RECURSIVE test (which is a bit obscure, but it does the trick) which fixes up htslib, but the necessary value doesn't get to the htscodecs test code.

There's no similar test on the htscodecs side so you don't see the problem there.

It is arguable that the htscodecs test .c files should be updated to include config.h, and the configure tests for both packages should be improved to ensure the correct feature test macros are set in config.h. This was a quick fix for the issue, but it may be better to head over to htscodecs and sort it out properly...

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

Successfully merging this pull request may close these issues.

undeclared: gettimeofday

2 participants