Skip to content

Conversation

brydinh
Copy link
Contributor

@brydinh brydinh commented Apr 17, 2022

This PR addresses issue 24783. Converted lint-include-guards.sh to python.

@DrahtBot DrahtBot added the Tests label Apr 17, 2022
@laanwj laanwj mentioned this pull request Apr 18, 2022
25 tasks
@theStack
Copy link
Contributor

Concept ACK

Note that there is currently a linter error in the CI:

Traceback (most recent call last):
  File "test/lint/lint-include-guards.py", line 88, in <module>
    main()
  File "test/lint/lint-include-guards.py", line 71, in main
    header_file_contents_string = open(header_file, 'r').read()
  File "/usr/lib/python3.6/encodings/ascii.py", line 26, in decode
    return codecs.ascii_decode(input, self.errors)[0]
UnicodeDecodeError: 'ascii' codec can't decode byte 0xce in position 2003: ordinal not in range(128)
^---- failure generated from test/lint/lint-include-guards.py
Python's open(...) seems to be used to open text files without explicitly
specifying encoding="utf8":

test/lint/lint-include-guards.py:        header_file_contents_string = open(header_file, 'r').read()
^---- failure generated from test/lint/lint-python-utf8-encoding.sh
Success: no issues found in 236 source files


regex_pattern = f'#(ifndef|define|endif //) {header_id}'

header_file_contents_string = open(header_file, 'r').read()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to open files using with, it ensures that the file will be closed at the end of the operation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

@KevinMusgrave
Copy link
Contributor

KevinMusgrave commented Apr 20, 2022

I tested ea3e7ab by commenting out the include guard in bench/bench.h and got the following results:

Before (test/lint/lint-include-guards.sh):

src/bench/bench.h seems to be missing the expected include guard:
  #ifndef BITCOIN_BENCH_BENCH_H
  #define BITCOIN_BENCH_BENCH_H
  ...
  #endif // BITCOIN_BENCH_BENCH_H

After (test/lint/lint-include-guards.py): The test passes

If I delete (rather than comment out) the include guard, then it prints the correct message.

Specify encoding when reading header files, add docstring

Update test/lint/lint-include-guards.py  include guard count logic

Co-authored-by: Kevin Musgrave <tkm45@cornell.edu>

Update test/lint/lint-include-guards.py by removing whitespace
@brydinh brydinh force-pushed the 24783-port-lint-script-to-python branch from 3f9f09f to d5fdec5 Compare April 20, 2022 13:55
@KevinMusgrave
Copy link
Contributor

Tested ACK d5fdec5

Outputs and exit codes match. Example:

src/attributes.h seems to be missing the expected include guard:
  #ifndef BITCOIN_ATTRIBUTES_H
  #define BITCOIN_ATTRIBUTES_H
  ...
  #endif // BITCOIN_ATTRIBUTES_H

src/banman.h seems to be missing the expected include guard:
  #ifndef BITCOIN_BANMAN_H
  #define BITCOIN_BANMAN_H
  ...
  #endif // BITCOIN_BANMAN_H

src/bech32.h seems to be missing the expected include guard:
  #ifndef BITCOIN_BECH32_H
  #define BITCOIN_BECH32_H
  ...
  #endif // BITCOIN_BECH32_H

@laanwj
Copy link
Member

laanwj commented Apr 25, 2022

Code review ACK d5fdec5

@laanwj laanwj merged commit 8b68677 into bitcoin:master Apr 25, 2022
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 26, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Apr 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants