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

pull hebasto wants to merge 1 commits into bitcoin-core:master from hebasto:240622-ci-msvc changing 1 files +6 −6
  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:
    1. /Zi, which produces a separate PDB file that contains all the symbolic debugging information, is incompatible with ccache; see:
  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

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-09 07:15 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me