lh3 / bwa

Burrow-Wheeler Aligner for short-read alignment (see minimap2 for long-read alignment)
GNU General Public License v3.0
1.55k stars 556 forks source link

GCC 10 and Makefile fixes #267

Closed SoapGentoo closed 4 years ago

SoapGentoo commented 4 years ago

Hi @lh3, This PR includes a patch for the upcoming GCC 10, which imports C++'s ODR rule to C too. I've also fixed the Makefile a bit so we don't have to patch it downstream. CC and AR don't need to be defined, make will use cc and gcc in their place. By omitting the definition, we don't have to hack the Makefile because we export CC and AR to the environment, which Make looks for before using the fallbacks.

jmarshall commented 4 years ago

The Makefile changes are already PR #263. (Further Makefile breakage to use environment variables first is unnecessary — even though bwa has no configure script — as your recipe would use make -e or make CC="$CC" AR="$AR" etc. Cf samtools/samtools#533.)

SoapGentoo commented 4 years ago

most build environments export CC without calling make CC="${CC}" explicitly, which just means you need to override the whole default build rule again.

The changes in PR #263 are good, but I'd like to see CC and AR removed, because there is absolutely no need to define them, and removing them breaks nothing.

SoapGentoo commented 4 years ago

@jmarshall also, thanks for your kind words on Twitter, pretty rich coming from someone who can't write a build system that builds out of source, or write portable Makefiles, or just plain Autoconf, which you seem to be advocating, yet can't seem to get right yourself (:wave: https://github.com/samtools/htslib/commit/34724943c803d345037abd3d15a076c00bf36563#diff-67e997bcfdac55191033d57a16d1408a).

jmarshall commented 4 years ago

That htslib commit fixed a minor bug introduced by someone else.

I suggest you keep any discussion here on topic for bwa.

lh3 commented 4 years ago

@jmarshall & @SoapGentoo I wonder if Makefile in minimap2 solves the problem. If so, I may change bwa makefile to the minimap2 way.

SoapGentoo commented 4 years ago

@lh3 that Makefile is fine as well, as it doesn't explicitly set the CC variable :+1:

SoapGentoo commented 4 years ago

@lh3 I've dropped the Makefile changes, could you merge the GCC 10 fix?

lh3 commented 4 years ago

Thanks.