scripts: add checks for minimum required OS versions #21871

pull fanquake wants to merge 4 commits into bitcoin:master from fanquake:new_tests_on_lief changing 6 files +61 −11
  1. fanquake commented at 12:44 pm on May 6, 2021: member

    macOS: We use a compile flag (-mmacosx-version-min=10.14) to set the minimum required version of macOS needed to run our binaries. This adds a sanity check that the version is being set as expected.

    Clangs Darwin driver should infer the SDK version used during compilation, and forward that through to the linker. Add a check that this has been done, and the expected SDK version is set. Should help prevent issues like #21771 in future.

    Windows: We use linker flags (-Wl,–major/minor-subsystem-version) to set the minimum required version of Windows needed to run our binaries. This adds a sanity check that the version is being set as expected.

    Gitian builds:

     0# macOS:
 18b6fcd61d75001c37b2af3fceb5ae09f5d2fe85e97d361f684214bd91c27954a  bitcoin-f015e1c2cac9-osx-unsigned.dmg
 23c1e412bc7f5a7a5d0f78e2cd84b7096831414e1304c1307211aa3e135d89bbf  bitcoin-f015e1c2cac9-osx-unsigned.tar.gz
 350b7b2804e8481f63c69c78e3e8a71c0d811bf2db8895dd6d3edae9c46a738ae  bitcoin-f015e1c2cac9-osx64.tar.gz
 4fe6b5c0a550096b76b6727efee30e85b60163a41c83f21868c849fdd9876b675  src/bitcoin-f015e1c2cac9.tar.gz
 58a20f21b20673dfc8c23e22b20ae0839bcaf65bf0e02f62381cdf5e7922936f0  bitcoin-core-osx-22-res.yml
 6
 7# Windows:
 8b01fcdc2a5673387050d6c6c4f96f1d350976a121155fde3f76c2af309111f9d  bitcoin-f015e1c2cac9-win-unsigned.tar.gz
 9b95bdcbef638804030671d2332d58011f8c4ed4c1db87d6ffd211515c32c9d02  bitcoin-f015e1c2cac9-win64-debug.zip
10350bf180252d24a3d40f05e22398fec7bb00e06d812204eb5a421100a8e10638  bitcoin-f015e1c2cac9-win64-setup-unsigned.exe
112730ddabe246d99913c9a779e97edcadb2d55309933d46f1dffd0d23ecf9aae5  bitcoin-f015e1c2cac9-win64.zip
12fe6b5c0a550096b76b6727efee30e85b60163a41c83f21868c849fdd9876b675  src/bitcoin-f015e1c2cac9.tar.gz
13aa60d7a753e8cb2d4323cfbbf4d964ad3645e74c918cccd66862888f8646d80f  bitcoin-core-win-22-res.yml
  2. fanquake added the label Scripts and tools on May 6, 2021
  3. hebasto commented at 12:55 pm on May 6, 2021: member
    Concep ACK. Was working on similar preventing #21771 right now :)
  4. fanquake force-pushed on May 7, 2021
  5. fanquake commented at 5:33 am on May 7, 2021: member
    Updated to fix a typo, and address the CI failure.
  6. hebasto commented at 2:10 pm on May 8, 2021: member

    @fanquake

    I would add a check for the version of the macOS SDK using during compilation (which should help prevent issues like #21771), however there seems to be a bug in LIEF when accessing binary.build_version.sdk (although not when sdk is printed as part of build_version). I have opened an issue upstream: lief-project/LIEF#577.

    Are we going to wait for LIEF 0.12 with a fix, or macOS SDK version check could be added in a separated PR?

  7. fanquake commented at 8:39 am on May 9, 2021: member

    Are we going to wait for LIEF 0.12 with a fix, or macOS SDK version check could be added in a separated PR?

    I’ve just added one here. Apparently LIEF 0.12 wont be released until September / October.

  8. hebasto commented at 11:55 am on May 9, 2021: member

    https://cirrus-ci.com/task/5407569322704896?logs=lint#L855:

    0contrib/devtools/symbol-check.py:232: error: Item "None" of "Optional[Match[str]]" has no attribute "groups"
1Found 1 error in 1 file (checked 200 source files)
2^---- failure generated from test/lint/lint-python.sh
  9. hebasto commented at 2:15 pm on May 9, 2021: member

    How about to check the solely MIN_OS error firing:

     0--- a/contrib/devtools/test-symbol-check.py
 1+++ b/contrib/devtools/test-symbol-check.py
 2@@ -142,6 +142,19 @@ class TestSymbolChecks(unittest.TestCase):
 3         self.assertEqual(call_symbol_check(cc, source, executable, ['-mmacosx-version-min=10.14', '-Wl,-platform_version', '-Wl,macos', '-Wl,10.14', '-Wl,10.15.6']),
 4                 (0, ''))
 5 
 6+        source = 'test5.c'
 7+        executable = 'test5'
 8+        with open(source, 'w', encoding="utf8") as f:
 9+            f.write('''
10+                int main()
11+                {
12+                    return 0;
13+                }
14+        ''')
15+
16+        self.assertEqual(call_symbol_check(cc, source, executable, ['-mmacosx-version-min=10.14', '-Wl,-platform_version', '-Wl,macos', '-Wl,10.15.6', '-Wl,10.15.6']),
17+                (1, executable + ': failed MIN_OS'))
18+
19     def test_PE(self):
20         source = 'test1.c'
21         executable = 'test1.exe'

    ?

  10. in contrib/devtools/symbol-check.py:268 in ae6a06dcac outdated 
    265+    ('SDK', check_MACHO_sdk),
266 ],
267 'PE' : [
268-    ('DYNAMIC_LIBRARIES', check_PE_libraries)
269+    ('DYNAMIC_LIBRARIES', check_PE_libraries),
270+    ('SUBSYTEM_VERSION', check_PE_subsystem_version),
    hebasto commented at 2:21 pm on May 9, 2021:

    typo:

    0    ('SUBSYSTEM_VERSION', check_PE_subsystem_version),
    fanquake commented at 6:48 am on May 10, 2021:
    Fixed in next pushed.
  11. hebasto commented at 2:23 pm on May 9, 2021: member
    Maybe add a test for the SUBSYSTEM_VERSION error to the test_PE function of the test-symbol-check.py?
  12. laanwj commented at 12:22 pm on May 10, 2021: member 
    0contrib/devtools/symbol-check.py:232: error: Item "None" of "Optional[Match[str]]" has no attribute "groups"
1Found 1 error in 1 file (checked 200 source files)
  13. fanquake force-pushed on May 12, 2021
  14. fanquake commented at 2:35 am on May 12, 2021: member

    contrib/devtools/symbol-check.py:232: error: Item “None” of “Optional[Match[str]]” has no attribute “groups”

    Addressed.

    How about to check the solely MIN_OS error firing:

    There is some overlap between the MIN_OS and SDK test. I think we have enough coverage in this regard.

    Maybe add a test for the SUBSYSTEM_VERSION error to the test_PE function of the test-symbol-check.py?

    Added a test for this.

  15. hebasto commented at 9:20 pm on May 12, 2021: member

    Assuming that test-symbol-check.py could and should be ran for cross builds, I see this PR closely related to #20980.

    At least #20980 allows to make test-security-check for macOS cross builds on Linux.

    After rebasing this PR on top of #20980 getting an error:

     0$ make test-security-check
 1.
 2----------------------------------------------------------------------
 3Ran 1 test in 0.484s
 4
 5OK
 6F
 7======================================================================
 8FAIL: test_MACHO (__main__.TestSymbolChecks)
 9----------------------------------------------------------------------
10Traceback (most recent call last):
11  File "./contrib/devtools/test-symbol-check.py", line 102, in test_MACHO
12    self.assertEqual(call_symbol_check(cc, source, executable, ['-lexpat']),
13AssertionError: Tuples differ: (1, '[28 chars]LLOWED_LIBRARIES!\ntest1: failed DYNAMIC_LIBRARIES') != (1, '[28 chars]LLOWED_LIBRARIES!\ntest1: failed DYNAMIC_LIBRARIES MIN_OS SDK')
14
15First differing element 1:
16'libe[23 chars]ALLOWED_LIBRARIES!\ntest1: failed DYNAMIC_LIBRARIES'
17'libe[23 chars]ALLOWED_LIBRARIES!\ntest1: failed DYNAMIC_LIBRARIES MIN_OS SDK'
18
19  (1,
20   'libexpat.1.dylib is not in ALLOWED_LIBRARIES!\n'
21-  'test1: failed DYNAMIC_LIBRARIES')
22+  'test1: failed DYNAMIC_LIBRARIES MIN_OS SDK')
23?                                  +++++++++++
24
25
26----------------------------------------------------------------------
27Ran 1 test in 0.093s
28
29FAILED (failures=1)
30make: *** [Makefile:1444: test-security-check] Error 1
  16. hebasto commented at 8:32 am on May 23, 2021: member

    @fanquake

    Are we going to wait for LIEF 0.12 with a fix, or macOS SDK version check could be added in a separated PR?

    I’ve just added one here. Apparently LIEF 0.12 wont be released until September / October.

    The fix is included in LIEF 0.11.5. From the changelog:

    0:MachO:
1  * Fix error on property :attr:`lief.MachO.BuildVersion.sdk` (see :issue:`533`)
  17. fanquake force-pushed on May 24, 2021
  18. fanquake commented at 3:30 am on May 24, 2021: member

    At least #20980 allows to make test-security-check for macOS cross builds on Linux. After rebasing this PR on top of #20980 getting an error:

    Ok. What is the actual error though?

    The fix is included in LIEF 0.11.5.

    I’ve included an update to 0.11.5 in this PR, and simplified the SDK check.

  19. hebasto commented at 6:04 am on May 24, 2021: member

    At least #20980 allows to make test-security-check for macOS cross builds on Linux. After rebasing this PR on top of #20980 getting an error:

    Ok. What is the actual error though?

    Well, not an error, but changed behavior: make test-security-check fails (this PR on top of the master + #20980), while it passed before (this PR on top of the master).

  20. fanquake commented at 6:28 am on May 24, 2021: member

    Well, not an error, but changed behavior:

    I understand. I’m asking is what is the underlying cause of the failure. I assume it’s the compiler/linker not accepting some of the newly added flags.

  21. hebasto commented at 6:32 am on May 24, 2021: member

    Well, not an error, but changed behavior:

    I understand. I’m asking is what is the underlying cause of the failure. I assume it’s the compiler/linker not accepting some of the newly added flags.

    From #bitcoin-builds channel on 2021-03-13:

    21:23:32 <dongcarl> Oh huh… I’m guessing the test-*-check.py scripts don’t expect the CC to be set with an SDK?

  22. scripts: LIEF 0.11.5 8732f7b6c9
  23. scripts: check minimum required macOS vesion is set 
    We use a compile flag (-mmacosx-version-min) to set the minimum required
version of macOS needed to run our binaries. This adds a sanity check
that the version is being set as expected.
    29615aef52
  24. scripts: check minimum required Windows version is set 
    We use linker flags (-Wl,--major/minor-subsystem-version) to set the
minimum required version of Windows needed to run our binaries. This
adds a sanity check that the version is being set as expected.
    c972345bac
  25. fanquake force-pushed on Jun 10, 2021
  26. fanquake commented at 3:22 am on June 10, 2021: member
    I’ve removed the macOS SDK check from test-security-check. I don’t think it adds much value, and given we don’t control the SDK version being used in the CI, we can’t necessarily test for/using the same SDK version we expect in our binaries.
  27. scripts: check macOS SDK version is set 
    Clangs Darwin driver should infer the SDK version used during compilation, and
forward that through to the linker. Add a check that this has been done, and the
expected SDK version is set.

Should help prevent issues like #21771 in future.
    aa80b5759d
  28. fanquake force-pushed on Jun 10, 2021
  29. hebasto approved
  30. hebasto commented at 8:39 pm on June 17, 2021: member

    ACK aa80b5759dfa613780a99801641519dd78bb3eca, tested by breaking tests:

    • macOS changes – on macOS Big Sur 11.4 (20F71)
    • Windows changes – on Linux Mint 20.1 (x86_64) with providing CC like
    0$ CC=x86_64-w64-mingw32-gcc contrib/devtools/test-symbol-check.py TestSymbolChecks.test_PE
  31. practicalswift commented at 8:51 pm on June 17, 2021: contributor
    Concept ACK
  32. fanquake merged this on Jun 18, 2021
  33. fanquake closed this on Jun 18, 2021
  34. fanquake deleted the branch on Jun 18, 2021
  35. sidhujag referenced this in commit 9e71e81dd6 on Jun 18, 2021
  36. gwillen referenced this in commit dcb699cad8 on Jun 1, 2022
  37. DrahtBot locked this on Aug 18, 2022


fanquake hebasto laanwj practicalswift

Labels
Scripts and tools

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-10-06 16:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me