Fix --no-undefined in Makefile on non-Darwin

[alyssais]

As far as I can tell, -undefined error was introduced mistakenly to replace --no-undefined, which doesn't work Apple's cctools' ld. The problem is, -undefined error only works on cctools' ld. This is much less noticeable than the original problem, because GCC's ld won't complain, it just won't do the right thing. The Solaris linker, on the other hand, will complain:

ld: fatal: file error: open failed: No such file or directory

This is a misleading error message, but what it is actually telling us is that it can't open a file named "error", which it tries to do because it doesn't recognise "-undefined" as an option that can take an argument.

We can demonstrate that this doesn't have the desired effect in GCC too. Consider this a C source file, main.c, containing the following:

extern void hello(void);

int main(void) {
    hello();
}

If we compile this using the command gcc -fPIC -shared main.c, we will get no indication that the hello function we are trying to call has never been defined. This is for, as the GCC documentation puts it, "historial reasons".

If we don't want this, we can pass --no-undefined to the linker, like this:

gcc -fPIC -shared -Wl,--no-undefined main.c

This is what the Makefile used to do, before being changed. Running this command will produce an error informing us of our mistake:

/tmp/ccGRJkJ8.o: In function `main':
main.c:(.text+0xc): undefined reference to `hello'
collect2: error: ld returned 1 exit status

However, trying to use the cctools' ld's equivalent reveals that GCC's ld definitely does not interpret it the same way. This command will succeed, just the same as the one where we specified no explicit linker options at all. I don't want to speculate on what effect, if any, -undefined error has on GCC's ld, but it definitely isn't the right one.

The fix, then, is to use cctools's ld's -undefined error on Darwin, and GCC's ld's --no-undefined on everything else. Checking operating system isn't bulletproof — it is completely possible to use GCC's ld on Darwin, but it is nevertheless better than the status quo. GCC ld users will get the desired errors, and linking will no longer outright fail on Solaris. I'm not sure what effect this would have on BSD systems, having never used one, but it's conceivable to me that they use a similar linker to Darwin. If this turns out to be the case, the fix is to change the operating system check to use the cctools variant on BSD as well as Darwin.

[alyssais]

Fixed Travis failure on Darwin – I was using UNAME in the Makefile before it had been defined. Should have used OS instead.

AppVeyor looks like it just hung and then timed out. 🤷‍♀️

[glebm]

@alyssais Perhaps we should detect the compiler / linker flavour instead of the operating system? I use clang on Linux and this PR would break it for me, because the clang linker does not support --no-undefined.

Not sure what the best way to detect the compiler toolchain is. Perhaps via the output of $(CC) --version? If it matches 'gcc' use GCC_LDFLAGS, else if matches 'clang' use CLANG_CFLAGS, otherwise use empty CFLAGS.

[xzyfer]

Kicked off a new appveyor build. I'm heading to sleep now (Australian time) and will check back in tomorrow.

[mgreter]

Maybe something like this could be used to detect clang vs gcc (seems to work on windows):

COMPILER=$(if $(filter $(shell $(CC) -v 2>&1),clang),clang,gcc)

In the worst case we could add a small c file using compiler macros to do the detection, which needs to be compiled first and can then be executed to return the used compiler. Although I don't really like to go down this route, since we still have the autotools way ...

#if defined(__clang__)
	/* Clang/LLVM. ---------------------------------------------- */

#elif defined(__ICC) || defined(__INTEL_COMPILER)
	/* Intel ICC/ICPC. ------------------------------------------ */

#elif defined(__GNUC__) || defined(__GNUG__)
	/* GNU GCC/G++. --------------------------------------------- */

#elif defined(__HP_cc) || defined(__HP_aCC)
	/* Hewlett-Packard C/aC++. ---------------------------------- */

#elif defined(__IBMC__) || defined(__IBMCPP__)
	/* IBM XL C/C++. -------------------------------------------- */

#elif defined(_MSC_VER)
	/* Microsoft Visual Studio. --------------------------------- */

#elif defined(__PGI)
	/* Portland Group PGCC/PGCPP. ------------------------------- */

#elif defined(__SUNPRO_C) || defined(__SUNPRO_CC)
	/* Oracle Solaris Studio. ----------------------------------- */

#endif

[glebm]

I'm not sure if 2 > &1 will work on Windows. I'll try the C program way.

[glebm]

Sent #2729

[xzyfer]

Catching up. Either approach seems reasonable to me. Is there consensus on which we should go with?

[alyssais]

I’m confused — there are autotools files in the repo, but the Makefile isn’t generated using autotools? What’s the reason for that — are you trying to move away from it?

Anyway, compiling a small C program is probably (just) more reliable than trying to parse compiler output. Although I wonder if it might be better to take the simpler option of just removing the problematic flags entirely and avoiding the whole problem…

[glebm]

The Makefile is provided so that it can be built without autotools. This is useful when this library is built for a C extension for Python or Ruby, where autotools is not a requirement of the build environment AFAIK.

Anyway, compiling a small C program is probably (just) more reliable than trying to parse compiler output.

It's also fast (30ms compile+run), so I'd prefer we do that instead of attempting to parse the --verbose output.

Although I wonder if it might be better to take the simpler option of just removing the problematic flags entirely and avoiding the whole problem…

Yeah, how useful are these flags?
Anyway, up to @mgreter and @xzyfer now

[xzyfer]

I'm not informed enough to make a call either way. Much of this either predates me or was driven by contributors that have since moved on. I'm open to informed recommendations.

…

On Tue., 13 Nov. 2018, 10:51 pm Gleb Mazovetskiy ***@***.*** wrote: The Makefile is provided so that it can be built without autotools. This is useful when this library is built for a C extension for Python or Ruby, where autotools is not a requirement of the build environment. Anyway, compiling a small C program is probably (just) more reliable than trying to parse compiler output. It's also fast (30ms compile+run), so I'd prefer we do that instead of parsing the compiler output. Although I wonder if it might be better to take the simpler option of just removing the problematic flags entirely and avoiding the whole problem… Yeah, how useful are these flags? Anyway, up to mgreter@ and xzyfer@ now — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2720 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAjZWOiUXetXj7eudUfgA3GbUaRRS3hcks5uurIxgaJpZM4W27Rl> .

[mgreter]

I'm fine with any solution. Since @glebm already provided a PR for the second approach (thx), I'd say just go with that. It should also be more robust than parsing/matching the version output.

[mgreter]

@alyssais the Makefile is also used on windows when compiling with mingw-gcc without msys2/cygwin environment. I don't think there is any native windows autotools buildchain.

[glebm]

So the compiler check approach turned out to be wrong, because as @alyssais has pointed out in #2729 (comment), the compiler and the linker can be (and often are) different.

Let's just remove these flags?

[xzyfer]

I'm fine with removing these flags.

[alyssais]

I've amended my commit to just remove these flags.

[alyssais]

Side note: I keep having to rebase this PR so it can be merged, because a new commit has been added to master, even though there aren't any conflicts or anything. Maybe have a think about whether that needs to be a hard "no"? IME it often doesn't, and in this sort of case where the change is very unlikely to conflict with anything else it gets annoying fast.

[xzyfer]

Apologies GitHub's UI doesn't make it clear. We're still able to do the merge for cases like this when we know there are no conflicts.

…

On Thu., 15 Nov. 2018, 1:48 am Alyssa Ross ***@***.*** wrote: Side note: I keep having to rebase this PR so it can be merged, because a new commit has been added to master, even though there aren't any conflicts or anything. Maybe have a think about whether that needs to be a hard "no"? IME it often doesn't, and in this sort of case where the change is very unlikely to conflict with anything else it gets annoying fast. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2720 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAjZWH5lbjddFHGP6yl5BfK16jTtLe5Eks5uvC09gaJpZM4W27Rl> .

[xzyfer]

Thanks for all your patience

[alyssais]

Wonderful, thank you! Do you have any plans for a release?

[xzyfer]

Hopefully within a week or so. We've been cutting releases from a feature branch for over a year so there is some QA needed as we migrate back to cutting releases from master.

…

On Thu., 15 Nov. 2018, 8:16 pm Alyssa Ross ***@***.*** wrote: Wonderful, thank you! Do you have any plans for a release? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#2720 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAjZWEcSXw_mcp3q0Bfqnx-RcTOgXDtqks5uvTDwgaJpZM4W27Rl> .