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

ci: Switch from "RelWithDebInfo" to "Release" config for MSVC jobs #1547

Closed
wants to merge 1 commit into from

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Jun 22, 2024

It is reasonable to test a configuration that is used in the Bitcoin Core build.

For MSVC, the "Release" config (/O2 /Ob2) is preferable over "RelWithDebInfo" (/Zi /O2 /Ob1) for the following reasons:

  1. /Ob2 can be slightly better than /Ob1 for performance; see:
  1. /Zi, which produces a separate PDB file that contains all the symbolic debugging information, is incompatible with ccache; see:

It is reasonable to test a configuration that is used in the Bitcoin
Core build.

For MSVC, the "Release" config (/O2 /Ob2) is preferable over
"RelWithDebInfo" (/Zi /O2 /Ob1) for the following reasons:

1. /Ob2 can be slightly better than /Ob1 for performance;
see: https://learn.microsoft.com/en-us/cpp/build/reference/ob-inline-function-expansion

2. /Zi, which produces a separate PDB file that contains all the
symbolic debugging information, is incompatible with ccache; see:
https://learn.microsoft.com/en-us/cpp/build/reference/z7-zi-zi-debug-information-format
https://github.com/ccache/ccache/wiki/MS-Visual-Studio
@fanquake
Copy link
Member

It is #1543 (comment) to test a configuration that is used in the Bitcoin Core build.

Isn't our build going to be RelWithDebInfo once we switch to using the secp256k1 build system? Not sure it's worth it to switch this now, just to switch it back again very shortly after?

/Zi, which produces a separate PDB file that contains all the symbolic debugging information, is incompatible with ccache; see:

Why is that relevant here (I can't see any ccache usage)?

@hebasto
Copy link
Member Author

hebasto commented Jun 24, 2024

It is #1543 (comment) to test a configuration that is used in the Bitcoin Core build.

Isn't our build going to be RelWithDebInfo once we switch to using the secp256k1 build system? Not sure it's worth it to switch this now, just to switch it back again very shortly after?

I think that Bitcoin Core should use the "Release" config when building with MSVC for the reasons mentioned in the PR description.

/Zi, which produces a separate PDB file that contains all the symbolic debugging information, is incompatible with ccache; see:

Why is that relevant here (I can't see any ccache usage)?

We use ccache in the CI job, which saves us up to 15 minutes of build time.

@fanquake
Copy link
Member

I think that Bitcoin Core should use the "Release" config when building with MSVC for the reasons mentioned in the PR description.

Seems odd we'd make an exception in regard to build types for MSVC. If that flag really is that important, and the only difference, we can just pass it manually?

We use ccache in the CI job, which saves us up to 15 minutes of build time.

That isn't in this repo.

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

Successfully merging this pull request may close these issues.

3 participants