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

1. 
   hebasto commented at 4:06 pm on June 22, 2024: member

   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

   3. /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
2. ci: Switch from "RelWithDebInfo" to "Release" config for MSVC jobs

   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

   d2fecc993a
3. hebasto force-pushed on Jun 22, 2024
4. 
   fanquake commented at 9:28 am on June 24, 2024: 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)?

5. 
   hebasto commented at 9:53 am on June 24, 2024: 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?

   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.

6. 
   fanquake commented at 9:55 am on June 24, 2024: 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.

7. hebasto closed this on Jun 24, 2024

   ---

8. real-or-random added the label ci on Jun 24, 2024
9. real-or-random added the label build on Jun 24, 2024

Contributors
hebasto fanquake

Labels
ci build