Preprocessor definitions with quotation marks

[lczech]

Hi there,

I'm having a weird issue in the interaction of my GitHub Actions CI environment and htslib.

The htslib Makefile defines some preprocessor values using quotation marks such as

echo '#define HTS_LDFLAGS "$(LDFLAGS)"' >> $@

For my CI, I'm using GitHub Actions with the setup-cpp action to access various compilers. For their clang/llvm setup however, they also use quotation marks around flags such as

addEnv("LDFLAGS", `-L"${directory}/lib"`),

which causes the preprocessor value to become for example

#define HTS_LDFLAGS "-L"/home/runner/llvm/lib" -fvisibility=hidden "

That of course fails to compile, as the path in there is not part of the quoted string.

I'm currently working around this by having

export LDFLAGS=`echo ${LDFLAGS} | sed 's/"//g'`
export CPPFLAGS=`echo ${CPPFLAGS} | sed 's/"//g'`

in my CI setup, but that does not seam ideal.

Honestly, I'm not entirely sure who is right here. On the one hand, putting a path in quotation marks seems okay on the setup-cpp side. On the other hand, having flags in a quoted string seems necessary from the point of view of htslib. My guess is that htslib should escape quotation marks within the $(LDFLAGS) when processing the Makefile. Still, I've opened an issue on setup-cpp's end as well, as I am not sure that paths should necessarily be quoted.

Cheers and thanks Lucas

[jkbonfield]

Interesting issue. I haven't looked in detail yet, but I've used bash variable editing tweaks for this in other contexts. Eg in bash:

@ seq4c[nfs_j/jkb]; a='fo"o'
@ seq4c[nfs_j/jkb]; echo $a ${a/\"/\\\"}
fo"o fo\"o

POSIX defines a bunch of parameter expansions here, but it sadly doesn't include this one. I guess it could be something we pipe through sed 's/[\\"]/\\&/g'. Needs thinking about more.

[jmarshall]

I don't remember if this was discussed at the time, but I think I for one was vaguely assuming people using quoting inside these variables could be ‘encouraged’ to use single quotes.

These are used only in the reporting functions for basically debugging purposes, so don't need to be 100% accurate. HTSlib's Makefile already uses some GNU-Make-specific stuff, so tweaking the value as follows ought to suffice (though I haven't tested it):

echo '#define HTS_LDFLAGS "$(subst ",',$(LDFLAGS))"' >> $@

(…Though I see setup-cpp has already done something to work around this.)

[jkbonfield]

I don't understand how this quoting is working. On the command line, quotes aren't accepted.

@ seq4c[samtools.../htslib]1; cat _.c
#include <libdeflate.h>
int main(void) {return 0;}

# Failing
@ seq4c[samtools.../htslib]; clang13 -o conftest  -Wall -g -O2 -fvisibility=hidden '-I"/nfs/users/nfs_j/jkb/ftp/compression/libdeflate"' _.c
_.c:1:10: fatal error: 'libdeflate.h' file not found
#include <libdeflate.h>
         ^~~~~~~~~~~~~~
1 error generated.
@ seq4c[samtools.../htslib]1; gcc12 -o conftest  -Wall -g -O2 -fvisibility=hidden '-I"/nfs/users/nfs_j/jkb/ftp/compression/libdeflate"' _.c
_.c:1:10: fatal error: libdeflate.h: No such file or directory
    1 | #include <libdeflate.h>
      |          ^~~~~~~~~~~~~~
compilation terminated.

# Working
@ seq4c[samtools.../htslib]1; clang13 -o conftest  -Wall -g -O2 -fvisibility=hidden '-I/nfs/users/nfs_j/jkb/ftp/compression/libdeflate' _.c

So at the level of executing commands, the quotes must have been already stripped out. Hence all my attempts at getting quotes into these variables in the configure script have been met by the configure script failing due to the compiler rejecting them.

What am I missing here?

[jkbonfield]

Ah no matter, I understand it now. I assume you modifying the make command line here? I don't use github actions so I'm not sure what those commands you listed translate to in terms of command-line activities.

I see modifying vars after the make command offers an extra level of indirection (which for some reason wasn't working on my ./configure tests). Eg

make CPPFLAGS='-I"/nfs/users/nfs_j/jkb/ftp/compression/libdeflate"'

removes the outer-quotes and so the command line is then valid once more.

The additional double-quotes do indeed serve a purpose for when the directories involved contain spaces (for example), so I can understand the desire for this functionality to work.