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

Preserve user-passed CXXFLAGS with --enable-debug #6434

Merged
merged 1 commit into from Jul 17, 2015

Conversation

Projects
None yet
3 participants
@gavinandresen
Contributor

gavinandresen commented Jul 14, 2015

I was surprised that this didn't work:
./configure --enable-debug CXXFLAGS="-DDEBUG_LOCKORDER"

@theuni : I don't THINK this will have unintended consequences, is there a reason --enable-debug overwrote CFLAGS/CXXFLAGS?

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Jul 14, 2015

Member

@gavinandresen looks good to me, agreed that the behavior is unexpected. A few thoughts, though:

  • I believe it's that way because "-O2 -g" are already added by default. After this change, a ./configure --enable-debug will end up with duped flags I think. Not a problem really, the last flag wins.
  • You really want to be passing that in as CPPFLAGS=-DDEBUG_LOCKORDER anyway. Those are pre-processor flags, which exist separately so they don't get mixed up in arguments about optim flags and etc.
  • IMO There should be no need to pass -DDEBUG_LOCKORDER via a flag, that kinda defeats the purpose of having configure options to begin with. How about just adding -DDEBUG_LOCKORDER to CPPFLAGS in the --enable-debug case?
Member

theuni commented Jul 14, 2015

@gavinandresen looks good to me, agreed that the behavior is unexpected. A few thoughts, though:

  • I believe it's that way because "-O2 -g" are already added by default. After this change, a ./configure --enable-debug will end up with duped flags I think. Not a problem really, the last flag wins.
  • You really want to be passing that in as CPPFLAGS=-DDEBUG_LOCKORDER anyway. Those are pre-processor flags, which exist separately so they don't get mixed up in arguments about optim flags and etc.
  • IMO There should be no need to pass -DDEBUG_LOCKORDER via a flag, that kinda defeats the purpose of having configure options to begin with. How about just adding -DDEBUG_LOCKORDER to CPPFLAGS in the --enable-debug case?
@gavinandresen

This comment has been minimized.

Show comment
Hide comment
@gavinandresen

gavinandresen Jul 14, 2015

Contributor

RE: CPPFLAGS: hmm, then --enable-debug should add -DDEBUG to CPPFLAGS instead of CFLAGS/CXXFLAGS...

RE: adding -DDEBUG_LOCKORDER to --enable-debug: sounds good to me.

Contributor

gavinandresen commented Jul 14, 2015

RE: CPPFLAGS: hmm, then --enable-debug should add -DDEBUG to CPPFLAGS instead of CFLAGS/CXXFLAGS...

RE: adding -DDEBUG_LOCKORDER to --enable-debug: sounds good to me.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 15, 2015

Member

How about just adding -DDEBUG_LOCKORDER to CPPFLAGS in the --enable-debug case?

Sounds good to me.

Although maybe then it should be documented then that the flag doesn't just add debug information, but also enables specific debug functionality.

Or maybe that's already clear:

--enable-debug          use debug compiler flags and macros (default is no)
Member

laanwj commented Jul 15, 2015

How about just adding -DDEBUG_LOCKORDER to CPPFLAGS in the --enable-debug case?

Sounds good to me.

Although maybe then it should be documented then that the flag doesn't just add debug information, but also enables specific debug functionality.

Or maybe that's already clear:

--enable-debug          use debug compiler flags and macros (default is no)

@laanwj laanwj added the Build system label Jul 15, 2015

configure --enable-debug changes
Three changes to how configure --enable-debug behaves:

1. Preserve user-passed CXXFLAGS/CFLAGS
2. Compile with -DDEBUG_LOCKORDER
3. Add -DDEBUG -DDEBUG_LOCKORDER to CPPFLAGS (since they are preprocessor options)
@gavinandresen

This comment has been minimized.

Show comment
Hide comment
@gavinandresen

gavinandresen Jul 15, 2015

Contributor

Tweaked based on comments:

Three changes to how configure --enable-debug behaves:

  1. Preserve user-passed CXXFLAGS/CFLAGS
  2. Compile with -DDEBUG_LOCKORDER
  3. Add -DDEBUG -DDEBUG_LOCKORDER to CPPFLAGS (since they are preprocessor options)
Contributor

gavinandresen commented Jul 15, 2015

Tweaked based on comments:

Three changes to how configure --enable-debug behaves:

  1. Preserve user-passed CXXFLAGS/CFLAGS
  2. Compile with -DDEBUG_LOCKORDER
  3. Add -DDEBUG -DDEBUG_LOCKORDER to CPPFLAGS (since they are preprocessor options)
@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Jul 15, 2015

Member

Thanks, looks good.

Member

theuni commented Jul 15, 2015

Thanks, looks good.

@laanwj laanwj merged commit 83b48c8 into bitcoin:master Jul 17, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jul 17, 2015

Merge pull request #6434
83b48c8 configure --enable-debug changes (Gavin Andresen)

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jan 9, 2016

configure --enable-debug changes
Three changes to how configure --enable-debug behaves:

1. Preserve user-passed CXXFLAGS/CFLAGS
2. Compile with -DDEBUG_LOCKORDER
3. Add -DDEBUG -DDEBUG_LOCKORDER to CPPFLAGS (since they are preprocessor options)

Github-Pull: #6434
Rebased-From: 83b48c8

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jan 10, 2016

configure --enable-debug changes
Three changes to how configure --enable-debug behaves:

1. Preserve user-passed CXXFLAGS/CFLAGS
2. Compile with -DDEBUG_LOCKORDER
3. Add -DDEBUG -DDEBUG_LOCKORDER to CPPFLAGS (since they are preprocessor options)

Github-Pull: #6434
Rebased-From: 83b48c8

@str4d str4d referenced this pull request Feb 15, 2017

Merged

Bitcoin 0.12 misc PRs 1 #2099

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment