lint/contrib/doc updates

jonatack commented at 2:20 am on May 11, 2024: contributor

DrahtBot commented at 2:20 am on May 11, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30291 (test: write functional test results to csv by tdb3)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Consensus on May 11, 2024

fanquake commented at 2:51 am on May 11, 2024: member

Fix the following linter errors observed

There are currently no linter errors if you run the linter using the container, which installs the same versions as the CI. This is the recommended way to run the linters, as it avoids differences that may occur due to different versions of dependencies installed on your local system, and the need for changes like this.

since recent releases of lief

As LIEF mostly exists here for the type stubs, I don’t think there’s a reason to diverge it from the version we use in Guix (0.13.2), if things are working fine.

jonatack commented at 3:13 am on May 11, 2024: contributor

This patch allows the individual linters to continue to be usefully called without Docker or Rust, per the following section of test/lint/README.md:

0#### Running the tests
1
2Individual tests can be run by directly calling the test script, e.g.:
3
4    test/lint/lint-files.py

I’ve been running them that way for 5+ years and would prefer to be able to continue doing so. If we now no longer want to support calling them directly or individually, then I’ll update the README to discourage/warn.

This patch (if correct) also avoids some duplicate work by the person who updates lief later on, who will most likely not see this if it is binned.

jonatack marked this as ready for review on May 11, 2024

fanquake commented at 5:59 am on May 11, 2024: member

This patch also avoids some duplicate work by the person who updates lief later on, who will most likely not see this if it is binned.

If someone if updating LIEF in Guix, they need to check the changes, and update the scripts for the new version, if needed.

I’m guessing your changes here will actually just break the Guix build, given it’s still using 0.13.2?

fanquake removed the label Consensus on May 11, 2024

DrahtBot added the label Consensus on May 11, 2024

fanquake renamed this:
~~script: lief updates~~
contrib: lief updates
on May 11, 2024

fanquake removed the label Consensus on May 11, 2024

DrahtBot added the label Scripts and tools on May 11, 2024

fanquake commented at 9:35 am on May 11, 2024: member

I’m guessing your changes here will actually just break the Guix build, given it’s still using 0.13.2?

 0time BASE_CACHE="/base_cache" SOURCES_PATH="/sources" SDK_PATH="/SDKs" ./contrib/guix/guix-build
 1< snip >
 2make[1]: Leaving directory '/distsrc-base/distsrc-950b3d4587f0-x86_64-linux-gnu'
 3Traceback (most recent call last):
 4  File "/distsrc-base/distsrc-950b3d4587f0-x86_64-linux-gnu/./contrib/devtools/security-check.py", line 230, in <module>
 5    lief.Binary.FORMATS.ELF: {
 6AttributeError: type object 'lief._lief.Binary' has no attribute 'FORMATS'
 7F
 8======================================================================
 9FAIL: test_ELF (__main__.TestSecurityChecks)
10----------------------------------------------------------------------
11Traceback (most recent call last):
12  File "/distsrc-base/distsrc-950b3d4587f0-x86_64-linux-gnu/./contrib/devtools/test-security-check.py", line 60, in test_ELF
13    self.assertEqual(call_security_check(cc, source, executable, ['-Wl,-zexecstack','-fno-stack-protector','-Wl,-znorelro','-no-pie','-fno-PIE', '-Wl,-z,separate-code']),
14AssertionError: Tuples differ: (1, '') != (1, 'test1: failed PIE NX RELRO Canary CONTROL_FLOW')
15
16First differing element 1:
17''
18'test1: failed PIE NX RELRO Canary CONTROL_FLOW'
19
20- (1, '')
21+ (1, 'test1: failed PIE NX RELRO Canary CONTROL_FLOW')
22
23----------------------------------------------------------------------
24Ran 1 test in 0.086s
25
26FAILED (failures=1)
27make: *** [Makefile:1355: test-security-check] Error 1

jonatack commented at 9:49 pm on May 11, 2024: contributor

Guix build, given it’s still using 0.13.2?

Thanks for pointing out this interdependency. Looking at #27813 and related preceding changes, would you be concept ACK on updating the guix manifest to a recent lief (0.14 or maybe imminent 0.15), or prefer to hold off for now?

fanquake commented at 4:57 am on May 13, 2024: member

would you be concept ACK on updating the guix manifest to a recent lief (0.14 or maybe imminent 0.15), or prefer to hold off for now?

I don’t really think we need to update or change anything in our release infrastructure right now, particularly if the only reason is so that linters can be run against versions of dependencies that we don’t use.

If anything, I think the docs could be updated in some way to say that running the linters outside the container, or with versions that differ from the CI is generally unsupported.

maflcko added the label DrahtBot Guix build requested on May 13, 2024

DrahtBot commented at 1:13 pm on May 13, 2024: contributor

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]

File	commit b94061902e52132489fe296c12396798700f1f35(master)	commit 9b607717039651c2c53d6c046da6e5bf01bc0d60(master and this pull)
SHA256SUMS.part	`a88d5be69a36dff1...`
*-aarch64-linux-gnu-debug.tar.gz	`f29e0eab457072da...`
*-aarch64-linux-gnu.tar.gz	`801fd4527c4ce2fd...`
*-arm-linux-gnueabihf-debug.tar.gz	`f2dad0b55bbea4ec...`
*-arm-linux-gnueabihf.tar.gz	`fbe1995d42c9f132...`
*-arm64-apple-darwin-unsigned.tar.gz	`f8d58b438d72c010...`
*-arm64-apple-darwin-unsigned.zip	`f2f3bc0716e05669...`
*-arm64-apple-darwin.tar.gz	`de55ee5223ce9ee7...`
*-powerpc64-linux-gnu-debug.tar.gz	`35798b92db6fe90e...`
*-powerpc64-linux-gnu.tar.gz	`517219bb850e0a9d...`
*-riscv64-linux-gnu-debug.tar.gz	`169aa1a25e60ad4f...`
*-riscv64-linux-gnu.tar.gz	`0caa779c623fdaaa...`
*-x86_64-apple-darwin-unsigned.tar.gz	`163e7aa096fff3fc...`
*-x86_64-apple-darwin-unsigned.zip	`b64a1c76d03be649...`
*-x86_64-apple-darwin.tar.gz	`98a43a113f919f78...`
*-x86_64-linux-gnu-debug.tar.gz	`3a7c3e554f36ae57...`
*-x86_64-linux-gnu.tar.gz	`e79caf55d2ccbdd8...`
*.tar.gz	`78861e054f9eb242...`	`a4dfd707ff256dd5...`
guix_build.log	`238639b3a831699f...`	`8115f2713312b0d9...`
guix_build.log.diff		`e73bf610cdc64f8b...`

DrahtBot removed the label DrahtBot Guix build requested on May 13, 2024

jonatack renamed this:
~~contrib: lief updates~~
lint/contrib/doc updates
on May 17, 2024

jonatack force-pushed on May 17, 2024

jonatack commented at 10:26 pm on May 17, 2024: contributor

If anything, I think the docs could be updated in some way to say that running the linters outside the container, or with versions that differ from the CI is generally unsupported.

Thanks for the feedback, done. Open to further suggestions (like whether to update the other dependencies).

jonatack force-pushed on May 17, 2024

contrib: continue if binary is None in {security,symbol}-check.py scripts

as the code in that iteration should not proceed in the absence of a binary.

This also fixes the following errors reported by recent releases of the python linter:

contrib/devtools/symbol-check.py:298: error: Item "None" of "Binary | None" has no attribute "format"  [union-attr]
contrib/devtools/security-check.py:253: error: Item "None" of "Binary | None" has no attribute "format"  [union-attr]
contrib/devtools/security-check.py:254: error: Item "None" of "Binary | None" has no attribute "abstract"  [union-attr]
contrib/devtools/security-check.py:255: error: Item "None" of "Binary | None" has no attribute "concrete"  [union-attr]

a7240e1a68

doc: warn about running linters with different dependency versions 15f511b6c6

ci: update dependency versions 5229faad27

test, doc: fix insufficient free space error message

to appease codespell, and improve its grammar

114a522d98

jonatack force-pushed on May 17, 2024

tdb3 commented at 7:27 am on May 18, 2024: contributor

@jonatack @kevkevinpal, what are your thoughts on reopening PR #30106 and dropping the test runner insuffient spelling error from 30084?

Seems like this PR (30084) is scoped to more than just the spelling error and fixing the error could help prevent near term lint issues for open PRs.

Maybe I’m missing something and this was already addressed?

edit: This is a just a warning from the linter.

in test/lint/README.md:53 in 114a522d98

49@@ -50,6 +50,10 @@ Individual tests can be run by directly calling the test script, e.g.:
50 test/lint/lint-files.py
51 ```
52 
53+However, doing this with dependency versions that differ from the [CI

maflcko commented at 8:12 am on May 18, 2024:

nit: Shouldn’t this go into the section that ends with “…could be outdated” above?

jonatack commented at 5:40 pm on May 18, 2024:

The sentence you mention refers to Linux distributions: Please be aware that on Linux distributions all dependencies are usually available as packages, but could be outdated.

The sentence in this patch refers to calling the files directly, which can be done with other platforms (and the issues I’m seeing are not that the packages are outdated, but that the current releases are ahead of the CI versions). So it is proposed here in the section where it’s relevant. That said, if you have a suggested version that combines both mentions, I’m happy to look at it.

maflcko commented at 6:46 pm on May 18, 2024:

The dependency version is picked at install time, and the previous section is about installing the dependencies, so it seems more suitable.

But just a nit, either is fine.

maflcko commented at 11:12 am on May 20, 2024:

I think the key point of both sentences is that all dependency versions must match exactly. Installing via distro packages or via pip are just two ways. Both can fail in the same way (outdated or too-recent dependency versions), so communicating them in the same section makes sense, but again, just a nit.

in contrib/devtools/security-check.py:254 in 114a522d98

249@@ -250,6 +250,8 @@ def check_MACHO_branch_protection(binary) -> bool:
250     for filename in sys.argv[1:]:
251         try:
252             binary = lief.parse(filename)
253+            if binary is None:
254+                continue

maflcko commented at 8:16 am on May 18, 2024:

Instead of claiming that this was suggested to be changed by a linter, it would be better to have a real motivation for the change, as well as a reason why the change is correct.

Looking at this, I don’t see why continuing is the right behavior change here.

fanquake commented at 8:54 am on May 22, 2024:

I agree, this change doesn’t look like an improvement.

jonatack commented at 5:49 pm on May 19, 2024: contributor

error could help prevent near term lint issues for open PRs. @tdb3 Spelling warnings reported by ./test/lint/lint-spelling.py are printed as warnings but don’t cause the lint CI to fail on open PRs and aren’t particularly high-priority to fix.

fanquake commented at 8:46 am on May 29, 2024: member

What is the status of this? I think it’d be easiest to just drop the contrib changes. Also no PR description now?

in test/functional/test_runner.py:651 in 114a522d98

648+                    logging.debug("Exiting early after test failure")
649                     break
650 
651                 if "[Errno 28] No space left on device" in stdout:
652-                    sys.exit(f"Early exiting after test failure due to insuffient free space in {tmpdir}\n"
653+                    sys.exit(f"Exiting early after test failure due to insufficient free space in {tmpdir}\n"

maflcko commented at 1:53 pm on May 31, 2024:

When fixing typos, it could make sense to cherry-pick the real ones (non-subtree, non-release notes) from the closed https://github.com/bitcoin/bitcoin/pull/30188/files. Otherwise, people will continue to open pull requests to fix them.

DrahtBot added the label Needs rebase on Jun 17, 2024

DrahtBot commented at 7:02 pm on June 17, 2024: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

fanquake marked this as a draft on Jun 25, 2024

fanquake commented at 3:45 pm on June 25, 2024: member

Moved to draft for now, given it’s not clear what the status of this is.

maflcko commented at 10:03 pm on June 26, 2024: member

Closing for now, due to inactivity. In any case, the major build system changes should be separate from lint/doc typo fixes, so if you want to pick this up again, I’d recommend to create two separate pull requests.

maflcko closed this on Jun 26, 2024

lint/contrib/doc updates #30084

Code Coverage

Reviews

Conflicts

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]