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.

Sign up for GitHub

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

Jump to bottom

depends: add -g to DEBUG=1 flags #29527

Merged
merged 2 commits into from Apr 3, 2024
Merged

depends: add -g to DEBUG=1 flags #29527

merged 2 commits into from Apr 3, 2024

Conversation

fanquake
Copy link
Member

@fanquake fanquake commented Mar 1, 2024 •
edited

Add -g to the base DEBUG=1 flags in depends.
Avoids the need to specify it per-package.
More alignment with --enable-debug behaviour in configure.

We also want to align the optimization flags, currently -O1 vs -O0, however that can be it's own PR.

@fanquake fanquake requested a review from theuni March 1, 2024 16:36
@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 1, 2024 •
edited

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK theuni
Concept ACK TheCharlatan, hebasto

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@theuni
Copy link
Member

theuni commented Mar 1, 2024

Concept ACK.

As I've mentioned before, I don't have a strong preference for -O0 vs -O1 vs -Og, but -Og makes the most sense to me.

More alignment with --enable-debug behaviour in configure.
We also want to align the optimization flags, currently -O1 vs -O0, however that can be it's own PR.

This I strongly agree with though. Whatever we choose we should use everywhere.

@theuni
Copy link
Member

theuni commented Mar 1, 2024

If I recall, the original reason for keeping -g out of here is that it seriously blows up the size of the build, which (at the time) had very real consequences for caching with c-i. Unsure if it's as big a deal these days?

@maflcko
Copy link
Member

maflcko commented Mar 12, 2024

The CI depends cache is "unlimited" in size, and the CI ccache size can be modified, if needed.

@TheCharlatan
Copy link
Contributor

Concept ACK

@TheCharlatan
Copy link
Contributor

I get an error in the debug windows build:

make HOST=x86_64-w64-mingw32 DEBUG=1 -j 30
...
Qt Tools:
  QDoc ................................... yes

Note: Also available for Linux: linux-clang linux-icc

Note: Using static linking will disable the use of dynamically
loaded plugins. Make sure to import all needed static plugins,
or compile needed modules into the library.

ERROR: Qt requires a compliant STL library.
make: *** [funcs.mk:291: /home/drgrid/bitcoin/depends/x86_64-w64-mingw32/.qt_stamp_configured] Error 3

@hebasto
Copy link
Member

hebasto commented Mar 18, 2024

Concept ACK.

Add -g to the base DEBUG=1 flags in depends.
More alignment with --enable-debug behaviour in configure.

Why not -g3 then?

bitcoin/configure.ac

Line 353 in 015ac13

dnl Prefer -g3, fall back to -g if that is unavailable.

@fanquake
Copy link
Member Author

Why not -g3 then?

Because we aren't testing, and -g always works.

@fanquake fanquake mentioned this pull request Mar 26, 2024
Closed
@fanquake
Copy link
Member Author

fanquake commented Mar 26, 2024 •
edited

I get an error in the debug windows build:

Looks like that error is unrelated to the changes here, and instead the Windows Qt DEBUG=1 build is just broken on master.

fanquake added a commit to fanquake/bitcoin that referenced this pull request Mar 27, 2024
@fanquake
depends: fix mingw-w64 Qt DEBUG=1 build
b7e7e72 
The issue is that compilation is done with `x86_64-w64-mingw32-g++-posix`,
but then linking is done with `x86_64-w64-mingw32-g++`.

I'm guessing this has been broken since bitcoin#24131
(01d1845), but have not checked.

Fixes bitcoin#29734.
Unblocks bitcoin#29527 (now DEBUG=1 builds can be tested).
@fanquake fanquake mentioned this pull request Mar 27, 2024
Merged
@fanquake
Copy link
Member Author

Looks like that error is unrelated to the changes here, and instead the Windows Qt DEBUG=1 build is just broken on master.

Reabsed on a commit that fixes this.

fanquake added a commit that referenced this pull request Mar 27, 2024
@fanquake
Merge #29747: depends: fix mingw-w64 Qt DEBUG=1 build
7a12cbe 
b7e7e72 depends: fix mingw-w64 Qt DEBUG=1 build (fanquake)

Pull request description:

  The issue is that compilation is done with `x86_64-w64-mingw32-g++-posix`, but then linking is done with `x86_64-w64-mingw32-g++`.

  I'm guessing this has been broken since #24131 (01d1845), but have not checked.

  Fixes #29734.
  Unblocks #29527 (`DEBUG=1` builds can be tested).

ACKs for top commit:
  hebasto:
    ACK b7e7e72, tested on Ubuntu 22.04 with the [installed](#29734 (comment)) `g++-mingw-w64-x86-64` package.
  TheCharlatan:
    ACK b7e7e72

Tree-SHA512: 9e24e84046c0489c20971bb9c68d1a643c233837193c184f61bff79dfc8d7397a5c5526ac1a205ad423920f2589559cd01cb104ceb7f89515bb6421510d82ca9
@fanquake fanquake force-pushed the depends_g_debug_flags branch 2 times, most recently from 7eed316 to aa78b9a Compare April 1, 2024 15:54
theuni
theuni approved these changes Apr 2, 2024
View reviewed changes
Copy link
Member

@theuni theuni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 84fbf9b

@fanquake fanquake merged commit 0d509ba into bitcoin:master Apr 3, 2024
15 of 16 checks passed
@fanquake fanquake deleted the depends_g_debug_flags branch April 3, 2024 09:43
@fanquake
Copy link
Member Author

fanquake commented Apr 3, 2024

Going to followup with an optimization flags alingment RFC.

fanquake added a commit to fanquake/bitcoin that referenced this pull request Apr 16, 2024
@fanquake
depends: fix mingw-w64 Qt DEBUG=1 build
a6862c5 
The issue is that compilation is done with `x86_64-w64-mingw32-g++-posix`,
but then linking is done with `x86_64-w64-mingw32-g++`.

I'm guessing this has been broken since bitcoin#24131
(01d1845), but have not checked.

Fixes bitcoin#29734.
Unblocks bitcoin#29527 (now DEBUG=1 builds can be tested).

Github-Pull: bitcoin#29747
Rebased-From: b7e7e72
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Apr 24, 2024
@fanquake @luke-jr
depends: fix mingw-w64 Qt DEBUG=1 build
f20a87a 
The issue is that compilation is done with `x86_64-w64-mingw32-g++-posix`,
but then linking is done with `x86_64-w64-mingw32-g++`.

I'm guessing this has been broken since bitcoin#24131
(01d1845), but have not checked.

Fixes bitcoin#29734.
Unblocks bitcoin#29527 (now DEBUG=1 builds can be tested).

Github-Pull: bitcoin#29747
Rebased-From: b7e7e72
glozow pushed a commit to glozow/bitcoin that referenced this pull request May 13, 2024
@fanquake @glozow
depends: fix mingw-w64 Qt DEBUG=1 build
72f843c 
The issue is that compilation is done with `x86_64-w64-mingw32-g++-posix`,
but then linking is done with `x86_64-w64-mingw32-g++`.

I'm guessing this has been broken since bitcoin#24131
(01d1845), but have not checked.

Fixes bitcoin#29734.
Unblocks bitcoin#29527 (now DEBUG=1 builds can be tested).

Github-Pull: bitcoin#29747
Rebased-From: b7e7e72
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theuni theuni theuni approved these changes

@TheCharlatan TheCharlatan Awaiting requested review from TheCharlatan

@hebasto hebasto Awaiting requested review from hebasto

Assignees
No one assigned
Labels
Build system CI failed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@fanquake @DrahtBot @theuni @maflcko @TheCharlatan @hebasto