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

fanquake commented at 8:05 AM on March 21, 2020: member

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

objdump -x src/qt/bitcoin-qt.exe | grep "DLL Name:"
	DLL Name: ADVAPI32.dll
	DLL Name: dwmapi.dll
	DLL Name: GDI32.dll
	DLL Name: IMM32.dll
	DLL Name: IPHLPAPI.DLL
	DLL Name: KERNEL32.dll
	DLL Name: msvcrt.dll
	DLL Name: ole32.dll
	DLL Name: OLEAUT32.dll
	DLL Name: SHELL32.dll
	DLL Name: SHLWAPI.dll
	DLL Name: USER32.dll
	DLL Name: UxTheme.dll
	DLL Name: VERSION.dll
	DLL Name: WINMM.dll
	DLL Name: WS2_32.dll

fanquake added the label Windows on Mar 21, 2020

fanquake added the label Scripts and tools on Mar 21, 2020

fanquake added the label Needs gitian build on Mar 21, 2020

laanwj commented at 8:08 AM on March 21, 2020: member

ACK 20916eaea294fb62c2a5b4e466aa89c57cfcba21

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.

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?

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.

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

fanquake commented at 3:08 AM on March 22, 2020:

Sure.

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.

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.

DrahtBot commented at 11:58 PM on March 21, 2020: member

Gitian builds

File	commit 5bf45fe2a9642f8ae8f8a12bcbf8f8b4770421ad<br>(master)	commit 692a1e0ee26dd731702a06ea19c680e0736b1f6e<br>(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...`

DrahtBot removed the label Needs gitian build on Mar 21, 2020

scripts: add PE dylib checking to symbol-check.py 1a0993ae35

fanquake force-pushed on Mar 22, 2020

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.

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.

hebasto approved

hebasto commented at 10:10 AM on March 22, 2020: member

ACK 1a0993ae354c36d6f219e67f82ca8236530d6201, tested on Linux Mint 19.3:

$ make -C depends HOST=x86_64-w64-mingw32
$ CONFIG_SITE=$PWD/depends/x86_64-w64-mingw32/share/config.site ./configure
$ make
$ make -C src check-symbols
make: Entering directory '/home/hebasto/GitHub/bitcoin/src'
make[1]: Entering directory '/home/hebasto/GitHub/bitcoin'
make[1]: Leaving directory '/home/hebasto/GitHub/bitcoin'
Checking Windows dynamic libraries...
make: 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

fanquake requested review from laanwj on Mar 23, 2020

fanquake added the label Needs gitian build on Mar 23, 2020

DrahtBot commented at 5:59 PM on March 23, 2020: member

Gitian builds

File	commit 5504703a9f8388dff66d33bd077bcc4c82dff6c8<br>(master)	commit de8795729326ba844ad4823d270c81995ee20789<br>(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...`

DrahtBot removed the label Needs gitian build on Mar 23, 2020

dongcarl commented at 7:44 PM on March 23, 2020: member

Concept ACK 1a0993ae354c36d6f219e67f82ca8236530d6201 Skimmed code.

laanwj merged this on Mar 25, 2020

laanwj closed this on Mar 25, 2020

fanquake deleted the branch on Mar 25, 2020

DrahtBot locked this on Feb 15, 2022