Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GCC 10 and Makefile fixes #267

Merged
merged 1 commit into from
Mar 23, 2020
Merged

GCC 10 and Makefile fixes #267

merged 1 commit into from
Mar 23, 2020

Conversation

SoapGentoo
Copy link
Contributor

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.

@SoapGentoo SoapGentoo changed the title Fixes GCC 10 and Makefile fixes Jan 25, 2020
@jmarshall
Copy link
Contributor

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
Copy link
Contributor Author

SoapGentoo commented Jan 28, 2020

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
Copy link
Contributor Author

@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: samtools/htslib@3472494#diff-67e997bcfdac55191033d57a16d1408a).

@jmarshall
Copy link
Contributor

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

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

@lh3
Copy link
Owner

lh3 commented Feb 25, 2020

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

@SoapGentoo
Copy link
Contributor Author

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

* GCC 10 defaults to `-fno-common`, which makes C behave
  more like C++ in that you can only ever have one definition
  of an object per executable.
@SoapGentoo
Copy link
Contributor Author

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

@lh3 lh3 merged commit 3072c80 into lh3:master Mar 23, 2020
@lh3
Copy link
Owner

lh3 commented Mar 23, 2020

Thanks.

@SoapGentoo SoapGentoo deleted the fixes branch March 23, 2020 15:32
jmtcsngr added a commit to jmtcsngr/miqScoreShotgunPublic that referenced this pull request May 24, 2023
fix bwa make with compiler option

picking from lh3/bwa#267
jmtcsngr added a commit to jmtcsngr/miqScoreShotgunPublic that referenced this pull request May 25, 2023
fix bwa make with compiler option

picking from lh3/bwa#267
jmtcsngr added a commit to jmtcsngr/miqScoreShotgunPublic that referenced this pull request May 25, 2023
jmtcsngr added a commit to jmtcsngr/miqScoreShotgunPublic that referenced this pull request May 25, 2023
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.

3 participants