Skip to content

Remove use of dumpbin from post-build checks.#834

Merged
BillyONeal merged 38 commits intomicrosoft:mainfrom
BillyONeal:kill-dumpbin
Feb 8, 2023
Merged

Remove use of dumpbin from post-build checks.#834
BillyONeal merged 38 commits intomicrosoft:mainfrom
BillyONeal:kill-dumpbin

Conversation

@BillyONeal
Copy link
Member

@BillyONeal BillyONeal commented Dec 14, 2022

We already understand all the information for DLLs from the existing PE/COFF reader, so we don't need to ask dumpbin to tell us again. Advantages:

  • Toolsets don't need to tell us where a dumpbin is to run these checks.
  • It's faster because we parse the PE headers once instead of once per check.
  • It's faster because we aren't launching a subprocess per library per check.
  • It is more resilient in the face of localized systems. (In particular, appcontainer check was probably vulnerable to localization)

Drive by fix:

Other notes:

  • Static libs aren't checked for problems in dynamic triplets, even though dynamic triplets are allowed to produce static libs. For example, a static lib that wants static CRT in a triplet configuring dynamic CRT is not caught, even though we could do so. I did not fix this one in this change because I expect it to fail a lot of things and so want it to be its own smaller targeted change.

@Neumann-A
Copy link
Contributor

Probably needs testing with stuff generated using clang-cl/llvm-link?

@BillyONeal
Copy link
Member Author

Probably needs testing with stuff generated using clang-cl/llvm-link?

Those are not available in our labs.

@Neumann-A
Copy link
Contributor

Neumann-A commented Dec 15, 2022

Those are not available in our labs.

Doesn't VS nowadays bundle LLVM? I mean there is also a complete ClangCL toolchain if installed (replace für with for and you basically have the English translation):
image
So why isn't it available ?

But my LLVM triplet is probably not affected due to:

## Policy settings
set(VCPKG_POLICY_SKIP_ARCHITECTURE_CHECK enabled)
set(VCPKG_POLICY_SKIP_DUMPBIN_CHECKS enabled)

although I would really like to remove those two lines.

Will the name of set(VCPKG_POLICY_SKIP_DUMPBIN_CHECKS enabled) be changed?

@BillyONeal
Copy link
Member Author

Doesn't VS nowadays bundle LLVM?

They are very particular about what software goes into the lab that has access to code signing, if that makes sense. I could probably jump through some hoops to get that to happen but I don't think it's worth it for just this. I did manually check a few clang-cl examples and it worked fine.

my LLVM triplet is probably not affected due to: [...]

Skipping dumpbin checks makes sense to me but not sure why you need a skip architecture check? That's probably an interesting repro here...

Regardless I think you'll able to remove those lines after this and #814 land.

Will the name of set(VCPKG_POLICY_SKIP_DUMPBIN_CHECKS enabled) be changed?

I think that just becomes meaningless, since the problem that policy exists to work around will no longer exist.

@BillyONeal BillyONeal marked this pull request as ready for review December 15, 2022 13:01
@BillyONeal
Copy link
Member Author

Depends on #814

@BillyONeal BillyONeal changed the title Remove most uses of dumpbin from post-build checks. Remove use of dumpbin from post-build checks. Dec 15, 2022
@Neumann-A
Copy link
Contributor

Skipping dumpbin checks makes sense to me but not sure why you need a skip architecture check? That's probably an interesting repro here...

see microsoft/vcpkg#10398

@Neumann-A
Copy link
Contributor

I think that just becomes meaningless, since the problem that policy exists to work around will no longer exist.

The policy was added in microsoft/vcpkg#9901 due to dumpbin not running on some of the llvm generated files. I never investigated why this error happened but I know that link is also sometimes not able to consume *.obj and *.lib files generated with llvm.

Note: main llvm PR at the time was microsoft/vcpkg#4609. Current LLVM test pr is microsoft/vcpkg#25897 just ignore the merge conflicts (resolve to upstream); I am still waiting for the boost/msbuild fixes to land before i touch that PR again.

@BillyONeal
Copy link
Member Author

Skipping dumpbin checks makes sense to me but not sure why you need a skip architecture check? That's probably an interesting repro here...

see microsoft/vcpkg#10398

I looked at this and indeed only the first linker member is present. I pushed a fix.

@Neumann-A
Copy link
Contributor

Could you also test the implementation with a static library generated using clang-cl with -flto enabled.
I get LNK4048 running dumpbin on those after the symbol listing. I'll attach one of the smaller libs here.
charset.lib.txt

# Conflicts:
#	src/vcpkg/base/cofffilereader.cpp
@BillyONeal
Copy link
Member Author

Could you also test the implementation with a static library generated using clang-cl with -flto enabled. I get LNK4048 running dumpbin on those after the symbol listing. I'll attach one of the smaller libs here. charset.lib.txt

Do you have the matching release lib for this? (I'm adding it to an e2e test)

@BillyONeal
Copy link
Member Author

About to push a fix for this warning output. Before:
image

After:
image

@BillyONeal
Copy link
Member Author

Do you have the matching release lib for this? (I'm adding it to an e2e test)

Excuse me I mean the debug one. (The wrong warning output made me confused!)

@BillyONeal
Copy link
Member Author

@Neumann-A You got me to break out hex workshop I hope you're happy
image

@BillyONeal BillyONeal merged commit ab466a1 into microsoft:main Feb 8, 2023
@BillyONeal BillyONeal deleted the kill-dumpbin branch February 8, 2023 03:10
@dg0yt
Copy link
Contributor

dg0yt commented Mar 14, 2023

Should I change to a UCRT mingw toolchain now? Or is there a knob?

-- Performing post-build validation
warning: Detected outdated dynamic CRT in the following files:
    /home/dg0yt/Projekte/vcpkg/vcpkg/packages/zlib_x64-mingw-dynamic/debug/bin/libzlibd1.dll:msvcrt.dll
    /home/dg0yt/Projekte/vcpkg/vcpkg/packages/zlib_x64-mingw-dynamic/bin/libzlib1.dll:msvcrt.dll

@BillyONeal
Copy link
Member Author

Should I change to a UCRT mingw toolchain now? Or is there a knob?

set(ALLOW_OBSOLETE_MSVCRT ON)

@BillyONeal
Copy link
Member Author

Excuse me, I mean:

set(VCPKG_POLICY_ALLOW_OBSOLETE_MSVCRT ON)

@Neumann-A
Copy link
Contributor

You probably mean set(VCPKG_POLICY_ALLOW_OBSOLETE_MSVCRT enabled) ?

@BillyONeal
Copy link
Member Author

:sigh:

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants