scripts: add PE dylib checking to symbol-check.py #18395

pull fanquake wants to merge 1 commits into bitcoin:master from fanquake:pe_dll_checking changing 4 files +53 −4
  1. fanquake commented at 8:05 am on March 21, 2020: member

    Uses objdump -x and looks for DLL Name: lines. i.e:

     0objdump -x src/qt/bitcoin-qt.exe | grep "DLL Name:"
     1	DLL Name: ADVAPI32.dll
     2	DLL Name: dwmapi.dll
     3	DLL Name: GDI32.dll
     4	DLL Name: IMM32.dll
     5	DLL Name: IPHLPAPI.DLL
     6	DLL Name: KERNEL32.dll
     7	DLL Name: msvcrt.dll
     8	DLL Name: ole32.dll
     9	DLL Name: OLEAUT32.dll
    10	DLL Name: SHELL32.dll
    11	DLL Name: SHLWAPI.dll
    12	DLL Name: USER32.dll
    13	DLL Name: UxTheme.dll
    14	DLL Name: VERSION.dll
    15	DLL Name: WINMM.dll
    16	DLL Name: WS2_32.dll
    
  2. fanquake added the label Windows on Mar 21, 2020
  3. fanquake added the label Scripts and tools on Mar 21, 2020
  4. fanquake added the label Needs gitian build on Mar 21, 2020
  5. laanwj commented at 8:08 am on March 21, 2020: member
    ACK 20916eaea294fb62c2a5b4e466aa89c57cfcba21
  6. practicalswift commented at 9:10 am on March 21, 2020: contributor

    Concept ACK

    Capitalization is not significiant in this context, right? If so I suggest .lower():ing all DLL names to make the script less fragile.

  7. hebasto commented at 1:00 pm on March 21, 2020: member

    Concept ACK 20916eaea294fb62c2a5b4e466aa89c57cfcba21

    Does contrib/devtools/README.md - symbol-check.py need update to mention Windows OS?

  8. in contrib/devtools/symbol-check.py:251 in 20916eaea2 outdated
    246+        raise IOError('Error opening file')
    247+    libraries = []
    248+    for line in stdout.splitlines():
    249+        if 'DLL Name:' in line:
    250+            tokens = line.split()
    251+            libraries.append(tokens[2])
    


    theStack commented at 1:19 pm on March 21, 2020:

    nit: wouldn’t rely on the whitespace in the left-hand side description string here, but rather use ‘: ’ as seperator and then access with index 1.

    0            tokens = line.split(': ')
    1            libraries.append(tokens[1])
    

    fanquake commented at 3:08 am on March 22, 2020:
    Sure.
  9. theStack commented at 1:28 pm on March 21, 2020: member
    Concept ACK (left one nit code review comment) I support practicalswift’s suggestion to not check for the DLL names in case-sensitive manner.
  10. laanwj commented at 5:05 pm on March 21, 2020: member
    The capitalization is fixed and determined by our build process, I don’t think it’s a problem to check case sensitively.
  11. DrahtBot commented at 11:58 pm on March 21, 2020: member

    Gitian builds

    File commit 5bf45fe2a9642f8ae8f8a12bcbf8f8b4770421ad(master) commit 692a1e0ee26dd731702a06ea19c680e0736b1f6e(master and this pull)
    bitcoin-0.19.99-aarch64-linux-gnu-debug.tar.gz b22d53a17559ca60... 1ea6368c252241a5...
    bitcoin-0.19.99-aarch64-linux-gnu.tar.gz 0c9a5d8774b2af8d... a9b6d36967701e29...
    bitcoin-0.19.99-arm-linux-gnueabihf-debug.tar.gz ec367ad80fb3d38c... 5e07748c13984af5...
    bitcoin-0.19.99-arm-linux-gnueabihf.tar.gz 422b2df32e15c86e... df11e3f4d907db2b...
    bitcoin-0.19.99-osx-unsigned.dmg 400859ceb32937ef... 1fb3bcfbb56ed86a...
    bitcoin-0.19.99-osx64.tar.gz 592d01b95db0fcc2... 3ff50de042c6167c...
    bitcoin-0.19.99-riscv64-linux-gnu-debug.tar.gz 3f3b7a8a49f6ef74... f0b5ffe09e9476e5...
    bitcoin-0.19.99-riscv64-linux-gnu.tar.gz e2f2f0829f15274b... 7e2e232c48f99556...
    bitcoin-0.19.99-win64-debug.zip ea387f7be17ebdac... 76779b82d29281e2...
    bitcoin-0.19.99-win64-setup-unsigned.exe 9a47ac4a9a082577... 1a7029421a1073d8...
    bitcoin-0.19.99-win64.zip 2b0fa9d48800ca97... 57eded5a7674647a...
    bitcoin-0.19.99-x86_64-linux-gnu-debug.tar.gz 7ac3305ca3ad57d9... 2825cdc6edf5114c...
    bitcoin-0.19.99-x86_64-linux-gnu.tar.gz fb048392882b021d... 193045afff904b00...
    bitcoin-0.19.99.tar.gz 0d103a4489a2b12f... 4139bffbda0465cb...
    bitcoin-core-linux-0.20-res.yml 0e8473886335bfb1... 13fe42618c791d13...
    bitcoin-core-osx-0.20-res.yml e179fc2662f698f7... e6c4cfda6cd07c77...
    bitcoin-core-win-0.20-res.yml 6930350677d39138... fb0361de1aee8018...
    linux-build.log f09f65b35309bf6e... cb451717f60b1b2e...
    osx-build.log 5fdfe8b124613a2c... ada4cd4794c7c8b8...
    win-build.log 756f9f6d2f891482... b371a4426f10304a...
    bitcoin-core-linux-0.20-res.yml.diff 12503d976df5c218...
    bitcoin-core-osx-0.20-res.yml.diff c9783e919a0f8163...
    bitcoin-core-win-0.20-res.yml.diff a947703cbb62a122...
    linux-build.log.diff ef6a8389af3ad6fd...
    osx-build.log.diff d064cf04f2f925ff...
    win-build.log.diff d7dadffda3fe583c...
  12. DrahtBot removed the label Needs gitian build on Mar 21, 2020
  13. scripts: add PE dylib checking to symbol-check.py 1a0993ae35
  14. fanquake force-pushed on Mar 22, 2020
  15. fanquake commented at 3:09 am on March 22, 2020: member

    Does contrib/devtools/README.md - symbol-check.py need update to mention Windows OS?

    Done.

    Capitalization is not significiant in this context, right? If so I suggest .lower():ing all DLL names to make the script less fragile.

    I support practicalswift’s suggestion to not check for the DLL names in case-sensitive manner. @practicalswift @theStack Can you clarify what the benefits of lower-casing would be?

    From what I can tell that would just introduce the ability for changes in the build to be unnecessarily hidden. i.e we could end up linked against a.dll and A.dll DLLs and that would go unnoticed.

    I care far less about this script being fragile than I care about knowing what is happening in our gitian build. If for some reason a bunch of DLLs get renamed it’s probably something we want to know about.

  16. theStack commented at 9:28 am on March 22, 2020: member

    The capitalization is fixed and determined by our build process, I don’t think it’s a problem to check case sensitively.

    Though I never went through the process of building Bitcoin Core for Windows, I was assuming that it is far from clear if the capitalization is really always fixed (as it doesn’t need to be for the resulting binary to work), but possibly rather dependent on the concrete build environment (OS/Toolchain, maybe even used file system?). My admittedly hasty rationale was that this script could outside of gitian builds may also be useful for people to validate inofficial binaries.

    I care far less about this script being fragile than I care about knowing what is happening in our gitian build. If for some reason a bunch of DLLs get renamed it’s probably something we want to know about.

    Yes, for gitian builds, that absolutely makes sense, better err on the side of caution.

  17. hebasto approved
  18. hebasto commented at 10:10 am on March 22, 2020: member

    ACK 1a0993ae354c36d6f219e67f82ca8236530d6201, tested on Linux Mint 19.3:

    0$ make -C depends HOST=x86_64-w64-mingw32
    1$ CONFIG_SITE=$PWD/depends/x86_64-w64-mingw32/share/config.site ./configure
    2$ make
    3$ make -C src check-symbols
    4make: Entering directory '/home/hebasto/GitHub/bitcoin/src'
    5make[1]: Entering directory '/home/hebasto/GitHub/bitcoin'
    6make[1]: Leaving directory '/home/hebasto/GitHub/bitcoin'
    7Checking Windows dynamic libraries...
    8make: Leaving directory '/home/hebasto/GitHub/bitcoin/src'
    

    Verified that DLLs were retrieved from binaries by /usr/bin/x86_64-w64-mingw32-objdump which belongs to the binutils-mingw-w64-x86-64 package. This package is installed on gitian as a dependency of gcc-mingw-w64-x86-64 which, in turn, is a dependency of https://github.com/bitcoin/bitcoin/blob/5504703a9f8388dff66d33bd077bcc4c82dff6c8/contrib/gitian-descriptors/gitian-win.yml#L20

  19. fanquake requested review from laanwj on Mar 23, 2020
  20. fanquake added the label Needs gitian build on Mar 23, 2020
  21. DrahtBot commented at 5:59 pm on March 23, 2020: member

    Gitian builds

    File commit 5504703a9f8388dff66d33bd077bcc4c82dff6c8(master) commit de8795729326ba844ad4823d270c81995ee20789(master and this pull)
    bitcoin-0.19.99-aarch64-linux-gnu-debug.tar.gz d906d729a9e0aa8a... 1e2aeb40e669362f...
    bitcoin-0.19.99-aarch64-linux-gnu.tar.gz 9d061a629d1f6493... ae3cfb7224f54b1b...
    bitcoin-0.19.99-arm-linux-gnueabihf-debug.tar.gz e653316207dc9d5f... bedba523ac7f5328...
    bitcoin-0.19.99-arm-linux-gnueabihf.tar.gz c1252fce8a9e4dcc... 38bdaa4f4e392c16...
    bitcoin-0.19.99-osx-unsigned.dmg b27dc85a5839aa01... 5cd1391592c7cee9...
    bitcoin-0.19.99-osx64.tar.gz 0b6d6ab4550e9e80... d3e5e2ca01589c1d...
    bitcoin-0.19.99-riscv64-linux-gnu-debug.tar.gz 0b581664aa7e71c0... 639bde276926bbf9...
    bitcoin-0.19.99-riscv64-linux-gnu.tar.gz 9055fdec76ec30e9... db83997f9f8d0178...
    bitcoin-0.19.99-win64-debug.zip 87b0ccd2625315bf... 29090c208924bc06...
    bitcoin-0.19.99-win64-setup-unsigned.exe 7c43dd66ee9381df... 3ed062ef6985d8d6...
    bitcoin-0.19.99-win64.zip 5d07503079a05cea... 22cb03b6b013a935...
    bitcoin-0.19.99-x86_64-linux-gnu-debug.tar.gz b5c4b3d860a8041e... 7dfc53f6123665a6...
    bitcoin-0.19.99-x86_64-linux-gnu.tar.gz b2a95cafa3d16933... dc7c7300940ae36f...
    bitcoin-0.19.99.tar.gz c74b1b1ad84e1065... 44ae3a9f4fa7fcdb...
    bitcoin-core-linux-0.20-res.yml 48a03fc68c71a926... e1c52598381a936d...
    bitcoin-core-osx-0.20-res.yml f1439a184460cb40... 6804d5c2e91ecf87...
    bitcoin-core-win-0.20-res.yml 01a86daf223f2965... 06bfc249894e85a6...
    linux-build.log ca02e3c29b81c8ad... b0d90b9e22294b78...
    osx-build.log f9d5833739965581... c82fb4c95bf44e80...
    win-build.log 9ac4e16e1774f0a4... 5c13a1d37567a4c0...
    bitcoin-core-linux-0.20-res.yml.diff 3d64abfd7d31f169...
    bitcoin-core-osx-0.20-res.yml.diff 46d85dc102b6c59e...
    bitcoin-core-win-0.20-res.yml.diff df30b3d0c4ec3ba3...
    linux-build.log.diff 79eeeaed404875c7...
    osx-build.log.diff 72e1a69cc22780e5...
    win-build.log.diff 5ca352c28a3d0311...
  22. DrahtBot removed the label Needs gitian build on Mar 23, 2020
  23. dongcarl commented at 7:44 pm on March 23, 2020: member
    Concept ACK 1a0993ae354c36d6f219e67f82ca8236530d6201 Skimmed code.
  24. laanwj merged this on Mar 25, 2020
  25. laanwj closed this on Mar 25, 2020

  26. fanquake deleted the branch on Mar 25, 2020
  27. DrahtBot locked this on Feb 15, 2022

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-19 00:12 UTC

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