lint/contrib/doc updates #30084

jonatack · 2024-05-11T02:20:00Z

No description provided.

DrahtBot · 2024-05-11T02:20:03Z

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.

fanquake · 2024-05-11T02:51:08Z

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 · 2024-05-11T03:13:56Z

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:

#### Running the tests

Individual tests can be run by directly calling the test script, e.g.:

    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.

fanquake · 2024-05-11T05:59:01Z

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 · 2024-05-11T09:35:33Z

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

time BASE_CACHE="/base_cache" SOURCES_PATH="/sources" SDK_PATH="/SDKs" ./contrib/guix/guix-build
< snip >
make[1]: Leaving directory '/distsrc-base/distsrc-950b3d4587f0-x86_64-linux-gnu'
Traceback (most recent call last):
  File "/distsrc-base/distsrc-950b3d4587f0-x86_64-linux-gnu/./contrib/devtools/security-check.py", line 230, in <module>
    lief.Binary.FORMATS.ELF: {
AttributeError: type object 'lief._lief.Binary' has no attribute 'FORMATS'
F
======================================================================
FAIL: test_ELF (__main__.TestSecurityChecks)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/distsrc-base/distsrc-950b3d4587f0-x86_64-linux-gnu/./contrib/devtools/test-security-check.py", line 60, in test_ELF
    self.assertEqual(call_security_check(cc, source, executable, ['-Wl,-zexecstack','-fno-stack-protector','-Wl,-znorelro','-no-pie','-fno-PIE', '-Wl,-z,separate-code']),
AssertionError: Tuples differ: (1, '') != (1, 'test1: failed PIE NX RELRO Canary CONTROL_FLOW')

First differing element 1:
''
'test1: failed PIE NX RELRO Canary CONTROL_FLOW'

- (1, '')
+ (1, 'test1: failed PIE NX RELRO Canary CONTROL_FLOW')

----------------------------------------------------------------------
Ran 1 test in 0.086s

FAILED (failures=1)
make: *** [Makefile:1355: test-security-check] Error 1

jonatack · 2024-05-11T21:49:45Z

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 · 2024-05-13T04:57:50Z

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.

DrahtBot · 2024-05-13T13:13:42Z

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

File	commit `b940619` (master)	commit `9b60771` (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...`

jonatack · 2024-05-17T22:26:00Z

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).

…ipts 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]

to appease codespell, and improve its grammar

tdb3 · 2024-05-18T07:27:57Z

@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.

maflcko · 2024-05-18T08:12:37Z

test/lint/README.md

@@ -50,6 +50,10 @@ Individual tests can be run by directly calling the test script, e.g.:
 test/lint/lint-files.py
 ```

+However, doing this with dependency versions that differ from the [CI


nit: Shouldn't this go into the section that ends with "...could be outdated" above?

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.

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.

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.

maflcko · 2024-05-18T08:16:00Z

contrib/devtools/security-check.py

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


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.

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

jonatack · 2024-05-19T17:49:34Z

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.

DrahtBot added the Consensus label May 11, 2024

jonatack marked this pull request as ready for review May 11, 2024 03:56

fanquake removed the Consensus label May 11, 2024

DrahtBot added the Consensus label May 11, 2024

fanquake changed the title ~~script: lief updates~~ contrib: lief updates May 11, 2024

fanquake removed the Consensus label May 11, 2024

DrahtBot added the Scripts and tools label May 11, 2024

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

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

jonatack mentioned this pull request May 15, 2024

lint: fixed typo in test_runner causing linter warning #30106

Closed

kevkevinpal mentioned this pull request May 15, 2024

refactor: simplify FormatSubVersion using strprintf/Join #30098

Merged

jonatack changed the title ~~contrib: lief updates~~ lint/contrib/doc updates May 17, 2024

jonatack force-pushed the 2024-05-lief-update branch from 950b3d4 to 4a5078b Compare May 17, 2024 22:20

jonatack force-pushed the 2024-05-lief-update branch from 4a5078b to ad6885a Compare May 17, 2024 22:40

jonatack added 4 commits May 17, 2024 16:47

doc: warn about running linters with different dependency versions

15f511b

ci: update dependency versions

5229faa

test, doc: fix insufficient free space error message

114a522

to appease codespell, and improve its grammar

jonatack force-pushed the 2024-05-lief-update branch from ad6885a to 114a522 Compare May 17, 2024 22:49

maflcko reviewed May 18, 2024

View reviewed changes

tdb3 mentioned this pull request May 19, 2024

doc: add guidance for RPC to developer notes #30142

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lint/contrib/doc updates #30084

lint/contrib/doc updates #30084

jonatack commented May 11, 2024 •

edited

DrahtBot commented May 11, 2024 •

edited

fanquake commented May 11, 2024

jonatack commented May 11, 2024 •

edited

fanquake commented May 11, 2024

fanquake commented May 11, 2024

jonatack commented May 11, 2024

fanquake commented May 13, 2024

DrahtBot commented May 13, 2024

jonatack commented May 17, 2024

tdb3 commented May 18, 2024 •

edited

maflcko May 18, 2024

jonatack May 18, 2024 •

edited

maflcko May 18, 2024

maflcko May 20, 2024

maflcko May 18, 2024

fanquake May 22, 2024

jonatack commented May 19, 2024

lint/contrib/doc updates #30084

Are you sure you want to change the base?

lint/contrib/doc updates #30084

Conversation

jonatack commented May 11, 2024 • edited

DrahtBot commented May 11, 2024 • edited

Code Coverage

Reviews

fanquake commented May 11, 2024

jonatack commented May 11, 2024 • edited

fanquake commented May 11, 2024

fanquake commented May 11, 2024

jonatack commented May 11, 2024

fanquake commented May 13, 2024

DrahtBot commented May 13, 2024

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

jonatack commented May 17, 2024

tdb3 commented May 18, 2024 • edited

maflcko May 18, 2024

Choose a reason for hiding this comment

jonatack May 18, 2024 • edited

Choose a reason for hiding this comment

maflcko May 18, 2024

Choose a reason for hiding this comment

maflcko May 20, 2024

Choose a reason for hiding this comment

maflcko May 18, 2024

Choose a reason for hiding this comment

fanquake May 22, 2024

Choose a reason for hiding this comment

jonatack commented May 19, 2024

jonatack commented May 11, 2024 •

edited

DrahtBot commented May 11, 2024 •

edited

jonatack commented May 11, 2024 •

edited

tdb3 commented May 18, 2024 •

edited

jonatack May 18, 2024 •

edited