Open vfazio opened 1 month ago
brgl
commented
1 month ago @brgl let me know if you'd prefer to stick with relative vs absolute imports and we can address them in this series.
Within the gpiod module, I'd prefer to stick with relative imports. I think they are relative for most parts, the ones that are absolute were most likely introduced by accident.
vfazio
commented
1 month ago Thanks.
I should have a branch for you to look at shortly. I'd appreciate a cursory review prior to me submitting it to the mailing list to make sure it's largely structured the way you prefer. Submitting subsequent revisions via git send-mail is always a PITA
brgl
commented
1 month ago Have you tried using b4? It's really a game changer for mailing-list oriented development. It allows to track revisions, changes etc. I recommend using it for interacting with the lkml.
vfazio
commented
1 month ago Nope, i'll have to check that out. There are very few projects where i still have to use send-mail, but having a tool that makes it easier when i do would be great, so thanks for the recommendation.
@brgl i noticed that bindings/python/gpiod/internal.py is still marked as under the GPL license and not LGPL, should this be fixed?
vfazio
commented
1 month ago Here's a branch https://github.com/vfazio/libgpiod/tree/vfazio-mypy
Can you let me know if there need to be more splits or combines?
brgl
commented
1 month ago @brgl i noticed that
bindings/python/gpiod/internal.pyis still marked as under the GPL license and not LGPL, should this be fixed?
Ah good catch. And seems like bindings/python/tests/ should all be GPL-2.0 while some are still LGPL.
brgl
commented
1 month ago Here's a branch https://github.com/vfazio/libgpiod/tree/vfazio-mypy
Can you let me know if there need to be more splits or combines?
No, it looks good to me. I really don't mind more simpler patches.
vfazio
commented
1 month ago @brgl I'm mostly happy with where i have the branch at this point. I'll push new refs in a second.
I have not run unit tests on this so would appreciate some help testing it, but as this is mostly internal typing changes, i don't expect problems.
The one thing i want to get your thoughts on before I submit is this:
One of the recommendations for newer code is to leverage f-strings where possible. I can either:
So, strings go from:
- raise ValueError("unknown line name: {}".format(line))
+ raise ValueError(f"unknown line name: {line}")For 3, the biggest challenge is the __str__ and __repr__ methods on dataclasses:
def __str__(self) -> str:
- return '<LineInfo offset={} name="{}" used={} consumer="{}" direction={} active_low={} bias={} drive={} edge_detection={} event_clock={} debounced={} debounce_period={}>'.format(
- self.offset,
- self.name,
- self.used,
- self.consumer,
- self.direction,
- self.active_low,
- self.bias,
- self.drive,
- self.edge_detection,
- self.event_clock,
- self.debounced,
- self.debounce_period,
- )
+ return f'<LineInfo offset={self.offset} name="{self.name}" used={self.used} consumer="{self.consumer}" direction={self.direction} active_low={self.active_low} bias={self.bias} drive={self.drive} edge_detecti
on={self.edge_detection} event_clock={self.event_clock} debounced={self.debounced} debounce_period={self.debounce_period}>'These lines are overly long and black/ruff will not split them.
There are two options that i can see for these:
If I'm "picky", 1 looks pretty dumb because the string looks like so:
print(f"<LineSettings {self.direction=!s} {self.edge_detection=!s} {self.bias=!s}>")
<LineSettings self.direction=Direction.AS_IS self.edge_detection=Edge.NONE self.bias=Bias.AS_IS>method 2 is slicker:
print(f"<LineSettings {' '.join(f'{f.name}={getattr(self, f.name)}' for f in fields(self))}>")
<LineSettings direction=Direction.AS_IS edge_detection=Edge.NONE bias=Bias.AS_IS drive=Drive.PUSH_PULL active_low=False debounce_period=0:00:00 event_clock=Clock.MONOTONIC output_value=Value.INACTIVE>However, neither option give us the easy ability to rename attributes, which we do in InfoEvent where event_type is printed as type:
return "<InfoEvent type={} timestamp_ns={} line_info={}>".format(
self.event_type, self.timestamp_ns, self.line_info
)If you don't care that some attributes get renamed in the str/repr, my vote is for 2. If you don't care for this change at all, i can either selectively apply it or ignore it all together.
vfazio
commented
1 month ago pushed new refs. I need to reword some commits, but i'm in a bit of a hurry so will have to look into it tomorrow.
brgl
commented
1 month ago @brgl I'm mostly happy with where i have the branch at this point. I'll push new refs in a second.
I have not run unit tests on this so would appreciate some help testing it, but as this is mostly internal typing changes, i don't expect problems.
The one thing i want to get your thoughts on before I submit is this:
One of the recommendations for newer code is to leverage f-strings where possible. I can either:
- suppress this check within the bindings
- selectively apply it on easy targets
- apply it within all of gpiod/
Let's apply them wherever it makes sense.
So, strings go from:
- raise ValueError("unknown line name: {}".format(line)) + raise ValueError(f"unknown line name: {line}")For 3, the biggest challenge is the
__str__and__repr__methods on dataclasses:def __str__(self) -> str: - return '<LineInfo offset={} name="{}" used={} consumer="{}" direction={} active_low={} bias={} drive={} edge_detection={} event_clock={} debounced={} debounce_period={}>'.format( - self.offset, - self.name, - self.used, - self.consumer, - self.direction, - self.active_low, - self.bias, - self.drive, - self.edge_detection, - self.event_clock, - self.debounced, - self.debounce_period, - ) + return f'<LineInfo offset={self.offset} name="{self.name}" used={self.used} consumer="{self.consumer}" direction={self.direction} active_low={self.active_low} bias={self.bias} drive={self.drive} edge_detecti on={self.edge_detection} event_clock={self.event_clock} debounced={self.debounced} debounce_period={self.debounce_period}>'These lines are overly long and black/ruff will not split them.
There are two options that i can see for these:
- replace them using {self.=} syntax
- iterate over the fields of the dataclass.
If I'm "picky", 1 looks pretty dumb because the string looks like so:
print(f"<LineSettings {self.direction=!s} {self.edge_detection=!s} {self.bias=!s}>") <LineSettings self.direction=Direction.AS_IS self.edge_detection=Edge.NONE self.bias=Bias.AS_IS>method 2 is slicker:
print(f"<LineSettings {' '.join(f'{f.name}={getattr(self, f.name)}' for f in fields(self))}>") <LineSettings direction=Direction.AS_IS edge_detection=Edge.NONE bias=Bias.AS_IS drive=Drive.PUSH_PULL active_low=False debounce_period=0:00:00 event_clock=Clock.MONOTONIC output_value=Value.INACTIVE>However, neither option give us the easy ability to rename attributes, which we do in InfoEvent where
event_typeis printed astype:return "<InfoEvent type={} timestamp_ns={} line_info={}>".format( self.event_type, self.timestamp_ns, self.line_info )If you don't care that some attributes get renamed in the str/repr, my vote is for 2. If you don't care for this change at all, i can either selectively apply it or ignore it all together.
Honestly in this particular case I'd leave it as is. How do other projects typically deal with it? This can't be the only one that has long repr strings. Can we suppress the warning just for specific cases?
vfazio
commented
1 month ago Honestly in this particular case I'd leave it as is. How do other projects typically deal with it? This can't be the only one that has long repr strings. Can we suppress the warning just for specific cases? Ill have to dig around and find out. I have a feeling most libraries dont bother sanitizing these values
In the mean time, ive selectively applied these and ignored these lines via a marker
https://github.com/vfazio/libgpiod/commit/2f2381d8260eb42e1b437289cfe1030dd02cc5d6
vfazio
commented
1 month ago vfazio
commented
1 month ago My guess is they use implicit string concatenation via (). I can test this out when i get to the office
vfazio
commented
1 month ago Hmm i see we have unit tests that are unfortunately actually assuming the repr can be eval'd.
I don't really agree with this choice and we can discuss it in a different issue. From the docs https://docs.python.org/3/library/functions.html#repr
For many types, this function makes an attempt to return a string that would yield an object with the same value when passed to eval(); otherwise, the representation is a string enclosed in angle brackets that contains the name of the type of the object together with additional information often including the name and address of the object
This may be fine with built-in types, but it doesn't take into consideration the actual import declarations. For this to work for complex types, the code has to directly import classes like Value, Direction, etc. They cannot be aliased (import ... as gpiodValue) which may be necessary to avoid import naming conflicts/shadowing.
I honestly have never seen a unittest eval a repr to make sure its instantiable and to compare it. It seems like an odd thing to require and may only work in a testing scenario, not out in the wild (do we really want this contractual obligation?) . It also seems like a maintenance burden since we cant just rely on the default implementations from dataclass.
It does seem absolutely legitimate to compare two instances of an object with the same values to check equality.
brgl
commented
1 month ago Hmm i see we have unit tests that are unfortunately actually assuming the repr can be eval'd.
I don't really agree with this choice and we can discuss it in a different issue. From the docs https://docs.python.org/3/library/functions.html#repr
For many types, this function makes an attempt to return a string that would yield an object with the same value when passed to eval(); otherwise, the representation is a string enclosed in angle brackets that contains the name of the type of the object together with additional information often including the name and address of the objectThis may be fine with built-in types, but it doesn't take into consideration the actual import declarations. For this to work for complex types, the code has to directly import classes like
Value,Direction, etc. They cannot be aliased (import ... as gpiodValue) which may be necessary to avoid import naming conflicts/shadowing.I honestly have never seen a unittest eval a repr to make sure its instantiable and to compare it. It seems like an odd thing to require and may only work in a testing scenario, not out in the wild (do we really want this contractual obligation?) . It also seems like a maintenance burden since we cant just rely on the default implementations from
dataclass.It does seem absolutely legitimate to compare two instances of an object with the same values to check equality.
Haven't I already made this an obligation by having it released? In any case: how much of an actual problem is this for refactoring? I'll trust your judgement on it, I don't know any better.
vfazio
commented
1 month ago In any case: how much of an actual problem is this for refactoring?
This is ultimately a non-issue for the patch series i'll be sending since I'm not planning to modify these. It's just something i noticed as i was looking through the code.
Haven't I already made this an obligation by having it released?
My series will abide by the current obligation. It was just a surprise to see unit tests utilizing the repr. Some of the classes in the bindings don't even implement __repr__, so there are non-conformant classes (InfoEvent, LineInfo) that aren't being highlighted by the unit tests. I consider these out of scope for this patch series however, which is why this probably warrants a separate issue.
As for future obligation, it seems reasonable to bump the minor version of the bindings to communicate an API breaking point if we want to consider the repr part of the API. If we're just adding missing or broken repr strings, there's probably no reason to bump the major/minor version.
I probably wouldn't consider it part of the API, as i think it's for debugging/development purposes and not supposed to be a guaranteed way for external libraries to interface with each other, especially since the results of repr can be inconsistent across libraries and whether eval works or not is highly subject to the state of the imports at the time of the call.
I'll trust your judgement on it, I don't know any better.
:smiling_imp: that's dangerous. While I have some exposure to the python ecosystem, I'm nowhere near an expert. I'm happy to go spelunking to see how other projects do things however.
And, to be clear, I'm not saying what's being done is wrong, it just seems like a hassle to maintain but maybe it's painless once it's all been figured out. My expectations could :100: be wrong here. I'm always willing to learn something new and abide by community expectations.
vfazio
commented
1 month ago Ran into some issues while making sure the typing interface was working as intended. Just to make sure I wasn't missing something, I typed all of the examples and unit tests and found some gaps so I'm making some revisions.
I'll likely tack on typing these directories to the series as well, but i may be less judicious in breaking out individual commits.
vfazio
commented
1 month ago @brgl Quick question. I'm running the unit tests over my branch now to make sure i'm not adding any behavior changes.
I see that if a line name is in the LineRequest.reconfigure_lines call but not in the request, it raises an error:
def _check_line_name(self, line):
if isinstance(line, str):
if line not in self._name_map:
raise ValueError("unknown line name: {}".format(line))
return True
return FalseHowever, the logic for integers (pure offsets) does not raise an error, it's just ignored.
The code comment says:
Args:
config
Dictionary mapping offsets or names (or tuples thereof) to
LineSettings. If no entry exists, or a None is passed as the
settings, then the configuration for that line is not changed.
Any settings for non-requested lines are ignored.So is the handling for string based values wrong and we should just silently ignore it or should we raise an error any time reconfigure is called for a line that wasn't requested? Or am I misreading how this works?
brgl
commented
1 month ago I think my goal was to silently ignore names that exist but were not configured but raise an error for names that don't exist. Right now it seems we error out on every name that's not in the mapping.
vfazio
commented
1 month ago I'll keep current behavior and won't change it. We can fix this in a separate patch if the behavior is not as we expect.
vfazio
commented
1 month ago Ok, that was a journey... I've decided to postpone making the public interfaces more generic for a separate series.
I've pushed new refs to my branch if you want to preview them.
All code passes mypy strict checks and ruff linting/formatting
(venv) vfazio@vfazio4 /mnt/development/libgpiod/bindings/python $ git rev-parse HEAD
dd2b4f32e54ef0795436f7303a67c322a8367fa1
(venv) vfazio@vfazio4 /mnt/development/libgpiod/bindings/python $ mypy --strict gpiod tests
Success: no issues found in 30 source files
(venv) vfazio@vfazio4 /mnt/development/libgpiod/bindings/python $ ruff format
30 files left unchanged
(venv) vfazio@vfazio4 /mnt/development/libgpiod/bindings/python $ ruff check
All checks passed!Tests continue to pass, though there are two pre-existing failures:
vfazio4 /mnt/development/libgpiod/bindings/python # make clean
Making clean in gpiod
make[1]: Entering directory '/mnt/development/libgpiod/bindings/python/gpiod'
Making clean in ext
make[2]: Entering directory '/mnt/development/libgpiod/bindings/python/gpiod/ext'
rm -rf .libs _libs
rm -f *.lo
make[2]: Leaving directory '/mnt/development/libgpiod/bindings/python/gpiod/ext'
make[2]: Entering directory '/mnt/development/libgpiod/bindings/python/gpiod'
rm -rf .libs _libs
rm -f *.lo
make[2]: Leaving directory '/mnt/development/libgpiod/bindings/python/gpiod'
make[1]: Leaving directory '/mnt/development/libgpiod/bindings/python/gpiod'
Making clean in tests
make[1]: Entering directory '/mnt/development/libgpiod/bindings/python/tests'
Making clean in gpiosim
make[2]: Entering directory '/mnt/development/libgpiod/bindings/python/tests/gpiosim'
rm -rf .libs _libs
rm -f *.lo
make[2]: Leaving directory '/mnt/development/libgpiod/bindings/python/tests/gpiosim'
Making clean in procname
make[2]: Entering directory '/mnt/development/libgpiod/bindings/python/tests/procname'
rm -rf .libs _libs
rm -f *.lo
make[2]: Leaving directory '/mnt/development/libgpiod/bindings/python/tests/procname'
make[2]: Entering directory '/mnt/development/libgpiod/bindings/python/tests'
rm -rf .libs _libs
rm -f *.lo
make[2]: Leaving directory '/mnt/development/libgpiod/bindings/python/tests'
make[1]: Leaving directory '/mnt/development/libgpiod/bindings/python/tests'
make[1]: Entering directory '/mnt/development/libgpiod/bindings/python'
rm -rf .libs _libs
rm -rf dist
rm -f gpiod/_ext.*.so
rm -f tests/*/_ext.*.so
rm -f *.lo
make[1]: Leaving directory '/mnt/development/libgpiod/bindings/python'
vfazio4 /mnt/development/libgpiod/bindings/python :( # exit
exit
(venv) vfazio@vfazio4 /mnt/development/libgpiod/bindings/python $ make python-tests
TOP_SRCDIR=/mnt/development/libgpiod \
TOP_BUILDDIR=/mnt/development/libgpiod \
/mnt/development/libgpiod/bindings/python/venv/bin/python build_tests.py
building 'tests.gpiosim._ext' extension
creating /tmp/libgpiod-rth5hf_2/tests
creating /tmp/libgpiod-rth5hf_2/tests/gpiosim
gcc -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -DOPENSSL_NO_SSL3 -fPIC -D_GNU_SOURCE=1 -I/mnt/development/libgpiod/include -I/mnt/development/libgpiod/tests/gpiosim -I/mnt/development/libgpiod/bindings/python/venv/include -I/home/vfazio/.pyenv/versions/3.9.8/include/python3.9 -c tests/gpiosim/ext.c -o /tmp/libgpiod-rth5hf_2/tests/gpiosim/ext.o -Wall -Wextra
gcc -shared -L/home/vfazio/.pyenv/versions/3.9.8/lib -Wl,-rpath,/home/vfazio/.pyenv/versions/3.9.8/lib -L/home/vfazio/.pyenv/versions/3.9.8/lib -Wl,-rpath,/home/vfazio/.pyenv/versions/3.9.8/lib /tmp/libgpiod-rth5hf_2/tests/gpiosim/ext.o -L/mnt/development/libgpiod/lib/.libs -L/mnt/development/libgpiod/tests/gpiosim/.libs -L/home/vfazio/.pyenv/versions/3.9.8/lib -lgpiosim -o /tmp/libgpiod-rth5hf_2/tests/gpiosim/_ext.cpython-39-x86_64-linux-gnu.so
building 'tests.procname._ext' extension
creating /tmp/libgpiod-rth5hf_2/tests/procname
gcc -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -DOPENSSL_NO_SSL3 -fPIC -D_GNU_SOURCE=1 -I/mnt/development/libgpiod/bindings/python/venv/include -I/home/vfazio/.pyenv/versions/3.9.8/include/python3.9 -c tests/procname/ext.c -o /tmp/libgpiod-rth5hf_2/tests/procname/ext.o -Wall -Wextra
gcc -shared -L/home/vfazio/.pyenv/versions/3.9.8/lib -Wl,-rpath,/home/vfazio/.pyenv/versions/3.9.8/lib -L/home/vfazio/.pyenv/versions/3.9.8/lib -Wl,-rpath,/home/vfazio/.pyenv/versions/3.9.8/lib /tmp/libgpiod-rth5hf_2/tests/procname/ext.o -L/home/vfazio/.pyenv/versions/3.9.8/lib -o /tmp/libgpiod-rth5hf_2/tests/procname/_ext.cpython-39-x86_64-linux-gnu.so
building 'gpiod._ext' extension
creating /tmp/libgpiod-rth5hf_2/gpiod
creating /tmp/libgpiod-rth5hf_2/gpiod/ext
gcc -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -DOPENSSL_NO_SSL3 -fPIC -D_GNU_SOURCE=1 -I/mnt/development/libgpiod/include -I/mnt/development/libgpiod/bindings/python/venv/include -I/home/vfazio/.pyenv/versions/3.9.8/include/python3.9 -c gpiod/ext/chip.c -o /tmp/libgpiod-rth5hf_2/gpiod/ext/chip.o -Wall -Wextra
gcc -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -DOPENSSL_NO_SSL3 -fPIC -D_GNU_SOURCE=1 -I/mnt/development/libgpiod/include -I/mnt/development/libgpiod/bindings/python/venv/include -I/home/vfazio/.pyenv/versions/3.9.8/include/python3.9 -c gpiod/ext/common.c -o /tmp/libgpiod-rth5hf_2/gpiod/ext/common.o -Wall -Wextra
gcc -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -DOPENSSL_NO_SSL3 -fPIC -D_GNU_SOURCE=1 -I/mnt/development/libgpiod/include -I/mnt/development/libgpiod/bindings/python/venv/include -I/home/vfazio/.pyenv/versions/3.9.8/include/python3.9 -c gpiod/ext/line-config.c -o /tmp/libgpiod-rth5hf_2/gpiod/ext/line-config.o -Wall -Wextra
gcc -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -DOPENSSL_NO_SSL3 -fPIC -D_GNU_SOURCE=1 -I/mnt/development/libgpiod/include -I/mnt/development/libgpiod/bindings/python/venv/include -I/home/vfazio/.pyenv/versions/3.9.8/include/python3.9 -c gpiod/ext/line-settings.c -o /tmp/libgpiod-rth5hf_2/gpiod/ext/line-settings.o -Wall -Wextra
gcc -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -DOPENSSL_NO_SSL3 -fPIC -D_GNU_SOURCE=1 -I/mnt/development/libgpiod/include -I/mnt/development/libgpiod/bindings/python/venv/include -I/home/vfazio/.pyenv/versions/3.9.8/include/python3.9 -c gpiod/ext/module.c -o /tmp/libgpiod-rth5hf_2/gpiod/ext/module.o -Wall -Wextra
gcc -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -DOPENSSL_NO_SSL3 -fPIC -D_GNU_SOURCE=1 -I/mnt/development/libgpiod/include -I/mnt/development/libgpiod/bindings/python/venv/include -I/home/vfazio/.pyenv/versions/3.9.8/include/python3.9 -c gpiod/ext/request.c -o /tmp/libgpiod-rth5hf_2/gpiod/ext/request.o -Wall -Wextra
gcc -shared -L/home/vfazio/.pyenv/versions/3.9.8/lib -Wl,-rpath,/home/vfazio/.pyenv/versions/3.9.8/lib -L/home/vfazio/.pyenv/versions/3.9.8/lib -Wl,-rpath,/home/vfazio/.pyenv/versions/3.9.8/lib /tmp/libgpiod-rth5hf_2/gpiod/ext/chip.o /tmp/libgpiod-rth5hf_2/gpiod/ext/common.o /tmp/libgpiod-rth5hf_2/gpiod/ext/line-config.o /tmp/libgpiod-rth5hf_2/gpiod/ext/line-settings.o /tmp/libgpiod-rth5hf_2/gpiod/ext/module.o /tmp/libgpiod-rth5hf_2/gpiod/ext/request.o -L/mnt/development/libgpiod/lib/.libs -L/home/vfazio/.pyenv/versions/3.9.8/lib -lgpiod -o /tmp/libgpiod-rth5hf_2/gpiod/_ext.cpython-39-x86_64-linux-gnu.so
copying /tmp/libgpiod-rth5hf_2/tests/gpiosim/_ext.cpython-39-x86_64-linux-gnu.so -> tests/gpiosim
copying /tmp/libgpiod-rth5hf_2/tests/procname/_ext.cpython-39-x86_64-linux-gnu.so -> tests/procname
copying /tmp/libgpiod-rth5hf_2/gpiod/_ext.cpython-39-x86_64-linux-gnu.so -> gpiod
(venv) vfazio@vfazio4 /mnt/development/libgpiod/bindings/python $ sudo su
vfazio4 /mnt/development/libgpiod/bindings/python # make python-tests-run
PYTHONPATH=/mnt/development/libgpiod/bindings/python \
LD_LIBRARY_PATH=/mnt/development/libgpiod/lib/.libs/:\
/mnt/development/libgpiod/tests/gpiosim/.libs/ \
/mnt/development/libgpiod/bindings/python/venv/bin/python -B -m tests
..................................................................................................................FF...........................
======================================================================
FAIL: test_reconfigure_missing_offsets (tests.tests_line_request.ReconfigureRequestedLines)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/mnt/development/libgpiod/bindings/python/tests/tests_line_request.py", line 583, in test_reconfigure_missing_offsets
self.assertTrue(info.active_low)
AssertionError: False is not true
======================================================================
FAIL: test_reconfigure_with_default (tests.tests_line_request.ReconfigureRequestedLines)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/mnt/development/libgpiod/bindings/python/tests/tests_line_request.py", line 568, in test_reconfigure_with_default
self.assertTrue(info.active_low)
AssertionError: False is not true
----------------------------------------------------------------------
Ran 143 tests in 0.978s
FAILED (failures=2)
make: *** [Makefile:686: python-tests-run] Error 1These failures occur on master as well, so do not seem related to my changes.
(venv) vfazio@vfazio4 /mnt/development/libgpiod/bindings/python $ git pull
Updating c840e17..5715f61
Fast-forward
bindings/python/examples/async_watch_line_value.py | 50 +++++++++++++++++++++++++++++++++++++-------------
1 file changed, 37 insertions(+), 13 deletions(-)
(venv) vfazio@vfazio4 /mnt/development/libgpiod/bindings/python $ make python-tests
TOP_SRCDIR=/mnt/development/libgpiod \
TOP_BUILDDIR=/mnt/development/libgpiod \
/mnt/development/libgpiod/bindings/python/venv/bin/python build_tests.py
building 'tests.gpiosim._ext' extension
creating /tmp/libgpiod-9zf4qwy8/tests
creating /tmp/libgpiod-9zf4qwy8/tests/gpiosim
gcc -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -DOPENSSL_NO_SSL3 -fPIC -D_GNU_SOURCE=1 -I/mnt/development/libgpiod/include -I/mnt/development/libgpiod/tests/gpiosim -I/mnt/development/libgpiod/bindings/python/venv/include -I/home/vfazio/.pyenv/versions/3.9.8/include/python3.9 -c tests/gpiosim/ext.c -o /tmp/libgpiod-9zf4qwy8/tests/gpiosim/ext.o -Wall -Wextra
gcc -shared -L/home/vfazio/.pyenv/versions/3.9.8/lib -Wl,-rpath,/home/vfazio/.pyenv/versions/3.9.8/lib -L/home/vfazio/.pyenv/versions/3.9.8/lib -Wl,-rpath,/home/vfazio/.pyenv/versions/3.9.8/lib /tmp/libgpiod-9zf4qwy8/tests/gpiosim/ext.o -L/mnt/development/libgpiod/lib/.libs -L/mnt/development/libgpiod/tests/gpiosim/.libs -L/home/vfazio/.pyenv/versions/3.9.8/lib -lgpiosim -o /tmp/libgpiod-9zf4qwy8/tests/gpiosim/_ext.cpython-39-x86_64-linux-gnu.so
building 'tests.procname._ext' extension
creating /tmp/libgpiod-9zf4qwy8/tests/procname
gcc -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -DOPENSSL_NO_SSL3 -fPIC -D_GNU_SOURCE=1 -I/mnt/development/libgpiod/bindings/python/venv/include -I/home/vfazio/.pyenv/versions/3.9.8/include/python3.9 -c tests/procname/ext.c -o /tmp/libgpiod-9zf4qwy8/tests/procname/ext.o -Wall -Wextra
gcc -shared -L/home/vfazio/.pyenv/versions/3.9.8/lib -Wl,-rpath,/home/vfazio/.pyenv/versions/3.9.8/lib -L/home/vfazio/.pyenv/versions/3.9.8/lib -Wl,-rpath,/home/vfazio/.pyenv/versions/3.9.8/lib /tmp/libgpiod-9zf4qwy8/tests/procname/ext.o -L/home/vfazio/.pyenv/versions/3.9.8/lib -o /tmp/libgpiod-9zf4qwy8/tests/procname/_ext.cpython-39-x86_64-linux-gnu.so
building 'gpiod._ext' extension
creating /tmp/libgpiod-9zf4qwy8/gpiod
creating /tmp/libgpiod-9zf4qwy8/gpiod/ext
gcc -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -DOPENSSL_NO_SSL3 -fPIC -D_GNU_SOURCE=1 -I/mnt/development/libgpiod/include -I/mnt/development/libgpiod/bindings/python/venv/include -I/home/vfazio/.pyenv/versions/3.9.8/include/python3.9 -c gpiod/ext/chip.c -o /tmp/libgpiod-9zf4qwy8/gpiod/ext/chip.o -Wall -Wextra
gcc -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -DOPENSSL_NO_SSL3 -fPIC -D_GNU_SOURCE=1 -I/mnt/development/libgpiod/include -I/mnt/development/libgpiod/bindings/python/venv/include -I/home/vfazio/.pyenv/versions/3.9.8/include/python3.9 -c gpiod/ext/common.c -o /tmp/libgpiod-9zf4qwy8/gpiod/ext/common.o -Wall -Wextra
gcc -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -DOPENSSL_NO_SSL3 -fPIC -D_GNU_SOURCE=1 -I/mnt/development/libgpiod/include -I/mnt/development/libgpiod/bindings/python/venv/include -I/home/vfazio/.pyenv/versions/3.9.8/include/python3.9 -c gpiod/ext/line-config.c -o /tmp/libgpiod-9zf4qwy8/gpiod/ext/line-config.o -Wall -Wextra
gcc -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -DOPENSSL_NO_SSL3 -fPIC -D_GNU_SOURCE=1 -I/mnt/development/libgpiod/include -I/mnt/development/libgpiod/bindings/python/venv/include -I/home/vfazio/.pyenv/versions/3.9.8/include/python3.9 -c gpiod/ext/line-settings.c -o /tmp/libgpiod-9zf4qwy8/gpiod/ext/line-settings.o -Wall -Wextra
gcc -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -DOPENSSL_NO_SSL3 -fPIC -D_GNU_SOURCE=1 -I/mnt/development/libgpiod/include -I/mnt/development/libgpiod/bindings/python/venv/include -I/home/vfazio/.pyenv/versions/3.9.8/include/python3.9 -c gpiod/ext/module.c -o /tmp/libgpiod-9zf4qwy8/gpiod/ext/module.o -Wall -Wextra
gcc -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -DOPENSSL_NO_SSL3 -fPIC -D_GNU_SOURCE=1 -I/mnt/development/libgpiod/include -I/mnt/development/libgpiod/bindings/python/venv/include -I/home/vfazio/.pyenv/versions/3.9.8/include/python3.9 -c gpiod/ext/request.c -o /tmp/libgpiod-9zf4qwy8/gpiod/ext/request.o -Wall -Wextra
gcc -shared -L/home/vfazio/.pyenv/versions/3.9.8/lib -Wl,-rpath,/home/vfazio/.pyenv/versions/3.9.8/lib -L/home/vfazio/.pyenv/versions/3.9.8/lib -Wl,-rpath,/home/vfazio/.pyenv/versions/3.9.8/lib /tmp/libgpiod-9zf4qwy8/gpiod/ext/chip.o /tmp/libgpiod-9zf4qwy8/gpiod/ext/common.o /tmp/libgpiod-9zf4qwy8/gpiod/ext/line-config.o /tmp/libgpiod-9zf4qwy8/gpiod/ext/line-settings.o /tmp/libgpiod-9zf4qwy8/gpiod/ext/module.o /tmp/libgpiod-9zf4qwy8/gpiod/ext/request.o -L/mnt/development/libgpiod/lib/.libs -L/home/vfazio/.pyenv/versions/3.9.8/lib -lgpiod -o /tmp/libgpiod-9zf4qwy8/gpiod/_ext.cpython-39-x86_64-linux-gnu.so
copying /tmp/libgpiod-9zf4qwy8/tests/gpiosim/_ext.cpython-39-x86_64-linux-gnu.so -> tests/gpiosim
copying /tmp/libgpiod-9zf4qwy8/tests/procname/_ext.cpython-39-x86_64-linux-gnu.so -> tests/procname
copying /tmp/libgpiod-9zf4qwy8/gpiod/_ext.cpython-39-x86_64-linux-gnu.so -> gpiod
(venv) vfazio@vfazio4 /mnt/development/libgpiod/bindings/python $ sudo su
vfazio4 /mnt/development/libgpiod/bindings/python # make python-tests-run
PYTHONPATH=/mnt/development/libgpiod/bindings/python \
LD_LIBRARY_PATH=/mnt/development/libgpiod/lib/.libs/:\
/mnt/development/libgpiod/tests/gpiosim/.libs/ \
/mnt/development/libgpiod/bindings/python/venv/bin/python -B -m tests
..................................................................................................................FF..........................
======================================================================
FAIL: test_reconfigure_missing_offsets (tests.tests_line_request.ReconfigureRequestedLines)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/mnt/development/libgpiod/bindings/python/tests/tests_line_request.py", line 583, in test_reconfigure_missing_offsets
self.assertTrue(info.active_low)
AssertionError: False is not true
======================================================================
FAIL: test_reconfigure_with_default (tests.tests_line_request.ReconfigureRequestedLines)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/mnt/development/libgpiod/bindings/python/tests/tests_line_request.py", line 568, in test_reconfigure_with_default
self.assertTrue(info.active_low)
AssertionError: False is not true
----------------------------------------------------------------------
Ran 142 tests in 0.879s
FAILED (failures=2)
make: *** [Makefile:686: python-tests-run] Error 1The "pre-existing" issues needs clarifying: I'm not seeing any and the tests pass fine for me on master with linux v6.11. What kernel are you on? Anything else in the environment that could cause this?
vfazio
commented
1 month ago The "pre-existing" issues needs clarifying: I'm not seeing any and the tests pass fine for me on master with linux v6.11. What kernel are you on? Anything else in the environment that could cause this?
It was my work machine. I think its on Ubuntu 22.04. Ill have to remote in or wait until Monday to see what the actual kernel version is.
However, if they pass for you, then its obviously indicative of an issue with my setup.
vfazio
commented
1 month ago @brgl I spent some time debugging the unittest failures this this morning and here's what i'm seeing:
The two tests:
def test_reconfigure_with_default(self):
info = self.chip.get_line_info(2)
self.assertEqual(info.direction, Direction.OUTPUT)
self.assertTrue(info.active_low)
self.assertEqual(info.drive, Drive.OPEN_DRAIN)
self.req.reconfigure_lines(
{
0: gpiod.LineSettings(direction=Direction.INPUT),
2: None,
("baz", "foo"): gpiod.LineSettings(direction=Direction.INPUT),
}
)
info = self.chip.get_line_info(0)
self.assertEqual(info.direction, Direction.INPUT)
info = self.chip.get_line_info(2)
self.assertEqual(info.direction, Direction.OUTPUT)
self.assertTrue(info.active_low) # this fails
self.assertEqual(info.drive, Drive.OPEN_DRAIN)
def test_reconfigure_missing_offsets(self):
info = self.chip.get_line_info(2)
self.assertEqual(info.direction, Direction.OUTPUT)
self.assertTrue(info.active_low)
self.assertEqual(info.drive, Drive.OPEN_DRAIN)
self.req.reconfigure_lines(
{(6, 0): gpiod.LineSettings(direction=Direction.INPUT)}
)
info = self.chip.get_line_info(0)
self.assertEqual(info.direction, Direction.INPUT)
info = self.chip.get_line_info(2)
self.assertEqual(info.direction, Direction.OUTPUT)
self.assertTrue(info.active_low) # this fails
self.assertEqual(info.drive, Drive.OPEN_DRAIN)In both cases, the failing line is a line that was a part of the chip.request_lines call, but is either not included in the request.reconfigure_lines call, or is included but with a None LineSettings object.
Because they're not in the reconfigure_lines call or not set with specific values, the lines get reverted to the default LineSettings value which was introduced with https://github.com/brgl/libgpiod/commit/fd57153ab2ba0d05f359f21eb72765ac1ede8fb5
So, either the behavior documented in the unittest is correct, and the expected functionality is now broken, or this is behaving as expected and the unit tests need to be updated.
People approaching the API from Python may not expect this behavior, especially since the documentation says:
def reconfigure_lines(
self, config: dict[tuple[Union[int, str]], LineSettings]
) -> None:
"""
Reconfigure requested lines.
Args:
config
Dictionary mapping offsets or names (or tuples thereof) to
LineSettings. If no entry exists, or a None is passed as the
settings, then the configuration for that line is not changed.
Any settings for non-requested lines are ignored.If I think about this structurally:
LineSettings() when a pin is missing from subB, maybe it should default to the LineSettings value from the request that originally generated subA.If you cannot reconfigure subB without affecting subA - subB when subB != subA, using bulk request objects seems not ideal given there is no granularity. Users may be better off managing a map of N requests for more fine-grained control to prevent accidentally reconfiguring pins.
brgl
commented
1 month ago Ah, isn't this the issue introduced in the kernel by commit b44039638741 ("gpiolib: cdev: Ignore reconfiguration without direction")? You need a recent kernel and current master of libgpiod I think.
vfazio
commented
1 month ago The only minimal kernel requirement documented are MIN_KERNEL_VERSION="5.17.4" and for python, apparently it's 5.19 required_kernel_version = LooseVersion("5.19.0")
The patch you mentioned is in 6.10 and looks to have been backported to 6.9. I don't think any distribution has shipped these in a stable release.
So, from the sound of it, we have binding behavior that's dependent on the deployed kernel without any shim within the bindings. People using these bindings will be covering a wide swath of kernel versions, especially if support for python 3.9 is advertised. I do not know if other bindings are affected (I don't know if you're publishing Rust crates).
Without clear advertisements or quirks in kernel behavior via a flags mechanism, i'm not sure we can do much better than a strict kernel version number check.
So, some nice to haves:
I don't think we want to have a hard runtime check that prevents the library from loading on older kernels, but having a list of patches and implications for the bindings seems kind of important.
vfazio
commented
1 month ago @brgl
Thanks again for the back and forth discussion on this issue, I appreciate your input!
I'm finalizing the patch list and will be drawing up a cover letter for the series soon.
One last thing before I do, I wanted to touch base on gpiod/__init__.py
Currently, there are a number of "unused" imports. My guess is that this is so someone can from gpiod import * (which is largely an anti-pattern nowadays) or from gpiod import <object> and have access to most of the publicly facing classes.
Right now, the objects in gpiod.line are hidden behind line.X or require explicit import via from gpiod.line import X, Y
>>> from gpiod import *
>>> print("\n".join(sorted(globals().keys())))
Chip
ChipClosedError
ChipInfo
EdgeEvent
InfoEvent
LineRequest
LineSettings
Optional
RequestReleasedError
Union
__annotations__
__builtins__
__doc__
__loader__
__name__
__package__
__spec__
api_version
chip
chip_info
edge_event
exception
info_event
internal
is_gpiochip_device
line
line_info
line_request
line_settings
request_lines
versionWould it be preferable to have these objects (Bias, Drive, etc) directly importable from the base gpiod module (from gpiod import Value) or would you like to keep them behind line.X? I ask because other objects are directly referencable from the core import.
Something like:
>>> from gpiod import *
>>> print("\n".join(sorted(globals().keys())))
Bias
Chip
ChipClosedError
ChipInfo
Clock
Direction
Drive
Edge
EdgeEvent
InfoEvent
LineInfo
LineRequest
LineSettings
RequestReleasedError
Value
__annotations__
__builtins__
__doc__
__loader__
__name__
__package__
__spec__
__version__
api_version
chip
chip_info
edge_event
exception
info_event
is_gpiochip_device
line
line_info
line_request
line_settings
request_linesSo, some nice to haves:
- the bindings have a shim to make things work across various kernel versions
Oh God, no. I don't want the ifdiffery this inevitably unleashes.
- tests are skipped on known non-working kernel versions
Same. The problem here is not in libgpiod. The kernel patch should have been backported to many more branches I think. The patch that's fixed by it dates back from 2020. @warthog618 is there any reason why this didn't happen? Did the patch not automatically apply to stable branches and so was dropped by Greg and Sasha?
- documentation about the behavior if it varies across kernel versions
I don't think it will be needed if we finalize the backporting of the offending fix.
brgl
commented
1 month ago One last thing before I do, I wanted to touch base on
gpiod/__init__.pyCurrently, there are a number of "unused" imports. My guess is that this is so someone can
from gpiod import *(which is largely an anti-pattern nowadays) orfrom gpiod import <object>and have access to most of the publicly facing classes.
Ha! You need to educate me on it being an anti-pattern. I admit I did it so that from gpiod import * can work.
Right now, the objects in
gpiod.lineare hidden behindline.Xor require explicit import viafrom gpiod.line import X, Y
And that is an omission from my side.
Would it be preferable to have these objects (
Bias,Drive, etc) directly importable from the base gpiod module (from gpiod import Value) or would you like to keep them behindline.X? I ask because other objects are directly referencable from the core import.Something like:
>>> from gpiod import * >>> print("\n".join(sorted(globals().keys()))) Bias Chip ChipClosedError ChipInfo Clock Direction Drive Edge EdgeEvent InfoEvent LineInfo LineRequest LineSettings RequestReleasedError Value __annotations__ __builtins__ __doc__ __loader__ __name__ __package__ __spec__ __version__ api_version chip chip_info edge_event exception info_event is_gpiochip_device line line_info line_request line_settings request_lines
Do you mean putting them under gpiod.Drive etc.? Can we do from .gpiod.line import * in __init__.py instead?
vfazio
commented
1 month ago
- the bindings have a shim to make things work across various kernel versions
Oh God, no. I don't want the ifdiffery this inevitably unleashes.
I 100% understand, but figured i'd throw it out there.
I don't think it will be needed if we finalize the backporting of the offending fix.
My search was naive, but i only saw it in 6.10 and 6.9 when i grepped for the upstream commit hash.
Do you mean putting them under gpiod.Drive etc.? Can we do from .gpiod.line import * in init.py instead?
I would import them from .line in __init__.py. I haven't finalized the structure yet, so don't judge the following too harshly:
from . import (
_ext,
chip,
chip_info,
edge_event,
exception,
info_event,
line,
line_info,
line_request,
line_settings,
)
from .chip import Chip
from .chip_info import ChipInfo
from .edge_event import EdgeEvent
from .exception import ChipClosedError, RequestReleasedError
from .info_event import InfoEvent
from .line import Bias, Clock, Direction, Drive, Edge, Value
from .line_info import LineInfo
from .line_request import LineRequest
from .line_settings import LineSettings
from .version import __version__
api_version = _ext.api_version
# Include submodules
__all__ = [
"chip",
"chip_info",
"edge_event",
"exception",
"info_event",
"line",
"line_info",
"line_request",
"line_settings",
"version",
]
# Re-export public types from this module
__all__ += chip.__all__
__all__ += chip_info.__all__
__all__ += edge_event.__all__
__all__ += exception.__all__
__all__ += info_event.__all__
__all__ += line.__all__
__all__ += line_info.__all__
__all__ += line_request.__all__
__all__ += line_settings.__all__
# Export module functions and attributes
__all__ += [
"api_version",
"is_gpiochip_device",
"request_lines",
"__version__",
]Right now, the objects in gpiod.line are hidden behind line.X or require explicit import via from gpiod.line import X, Y
And that is an omission from my side.
Since it sounds like it was skipped by mistake, I will work out something to allow these to be imported from gpiod a la from gpiod import Value
warthog618
commented
1 month ago Same. The problem here is not in libgpiod. The kernel patch should have been backported to many more branches I think. The patch that's fixed by it dates back from 2020. @warthog618 is there any reason why this didn't happen? Did the patch not automatically apply to stable branches and so was dropped by Greg and Sasha?
Similar patches have been backported to all stable branches as far as 5.10, so I assume it conflicts with some other non-backported changes and so could not be automatically applied. e.g. applying it to 6.6 fails. This is a new one for me - what is the usual process here?
brgl
commented
1 month ago Same. The problem here is not in libgpiod. The kernel patch should have been backported to many more branches I think. The patch that's fixed by it dates back from 2020. @warthog618 is there any reason why this didn't happen? Did the patch not automatically apply to stable branches and so was dropped by Greg and Sasha?
Similar patches have been backported to all stable branches as far as 5.10, so I assume it conflicts with some other non-backported changes and so could not be automatically applied. e.g. applying it to 6.6 fails. This is a new one for me - what is the usual process here?
Ah then maybe the issue is with distros not having picked it up? @vfazio can you point me to the sources for the kernel you're using?
warthog618
commented
1 month ago Ah then maybe the issue is with distros not having picked it up? @vfazio can you point me to the sources for the kernel you're using?
No, it has only been applied to 6.9 as it conflicts with the next earliest stable (6.6) and so any before that. I assume we need to submit an amended patch to stable if it does not apply automatically?
brgl
commented
1 month ago Ah then maybe the issue is with distros not having picked it up? @vfazio can you point me to the sources for the kernel you're using?
No, it has only been applied to 6.9 as it conflicts with the next earliest stable (6.6) and so any before that. I assume we need to submit an amended patch to stable if it does not apply automatically?
You said Similar patches have been backported to all stable branches as far as 5.10 so I assumed you meant that the similar patches were the backported ones? But maybe I misunderstood? Anyway, yes, if that's not the case then that should be manually applied to older branches.
vfazio
commented
1 month ago Just to respond to the mention, Ubuntu 22.04 releases come from https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/jammy/
I'm on this revision https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/jammy/tree/?h=Ubuntu-hwe-6.8-6.8.0-40.40_22.04.3
Distros typically control the version of the kernel that's deployed via their package manager. So even if a patch landed in 6.8.13 or 6.6.53, it's up to the distro to bump to that minor version or port patches to their target kernel.
RPi is on https://github.com/raspberrypi/linux, i think their default branch is 6.6 right now but they also are selective in their backports (though they typically take minor point releases pretty quickly from my experience).
The WSL2 kernel is i think even further behind, so people doing development in a WSL container may be at an even further disadvantage
I hand compile my own RPi kernel, so i have some control over that for my deployed situation, however when it comes to running unit tests on the code, i have less control as these are locked down by organization.
warthog618
commented
1 month ago You said
Similar patches have been backported to all stable branches as far as 5.10so I assumed you meant that the similar patches were the backported ones? But maybe I misunderstood? Anyway, yes, if that's not the case then that should be manually applied to older branches.
Yeah, sorry, the point I was trying to make was that I had expected it to be backported as similar patches had been. But it seems linereq_set_config() has had a bit of work done relatively recently so that particular patch only applies to recent kernels. I'll take a look at updating it to suit the older branches.
warthog618
commented
1 month ago Wrt the distros, they generally do pull the updates to the stable branches into their kernels reasonably promptly - at least that has been my experience with the RPi kernels.
For testing, the sticking point I find is the gpio-sim module that is not typically provided by distos, so I end up doing a kernel build anyway.
vfazio
commented
1 month ago This series is currently at 22 patches and i'm probably going to draw the line here.
git log --oneline HEAD...origin/master | wc -l
22
da7cf34 bindings: python: configure and document dev dependencies
b050458 bindings: python: tests: use f-strings
e915de5 bindings: python: tests: annotate internal members
523da39 bindings: python: tests: make EventType private to prevent export
0c6fcc8 bindings: python: tests: ignore purposeful type errors
f442abb bindings: python: tests: add missing type annotations
a516aa0 bindings: python: tests: fix imports and exports
5e2b30c bindings: python: tests: add type stubs for external modules
c16df8b bindings: python: tests: fix duplicate test name
03d0255 bindings: python: selectively use f-strings
646cbe1 bindings: python: convert lines to offsets in LineRequest
9f850d0 bindings: python: fix LineRequest union-attr type errors
496ab61 bindings: python: fix Chip union-attr type errors
8c79b1c bindings: python: annotate internal members of Chip
2c512a8 bindings: python: annotate internal members of LineRequest
9222ccf bindings: python: raise exception type, not exception instance
111bfc8 bindings: python: add missing method type annotations
b4a5840 bindings: python: fix annotation of variable length tuples
ae18980 bindings: python: cast return value of LineRequest.get_values
6127405 bindings: python: make internal a private submodule
6b9b2bc bindings: python: fix imports and exports
241626c bindings: python: add type stubs for _extI need to draft a reasonable cover letter I suppose.
The goal is to not introduce any functional changes; code coverage stays the same and unit tests continue to pass.
There are changes to the public interfaces, but they:
I was hoping to have this out earlier this week but fires keep popping up at work
vfazio
commented
1 month ago Have you tried using b4? It's really a game changer for mailing-list oriented development. It allows to track revisions, changes etc. I recommend using it for interacting with the lkml.
@brgl I feel a bit dumb, but when using b4, how are you adding the [libgpiod] prefix to the patches? When i use --set-prefix or --add-prefix it formats it as [PATCH [prefix] x/y] and not [libgpiod][PATCH x/y]
brgl
commented
1 month ago Have you tried using b4? It's really a game changer for mailing-list oriented development. It allows to track revisions, changes etc. I recommend using it for interacting with the lkml.
@brgl I feel a bit dumb, but when using b4, how are you adding the [libgpiod] prefix to the patches? When i use --set-prefix or --add-prefix it formats it as [PATCH x/y] and not [libgpiod][PATCH x/y]
Yeah, I haven't found a way to do it either. Just go with [libgpiod][PATCH] That's alright.
vfazio
commented
1 month ago Have you tried using b4? It's really a game changer for mailing-list oriented development. It allows to track revisions, changes etc. I recommend using it for interacting with the lkml.
@brgl I feel a bit dumb, but when using b4, how are you adding the [libgpiod] prefix to the patches? When i use --set-prefix or --add-prefix it formats it as [PATCH [prefix] x/y] and not [libgpiod][PATCH x/y]
vfazio
commented
1 month ago brgl
commented
1 month ago odd, i submitted the patches and i received my CCs both at work and in gmail, but do not see patches 00-04 on the list
All the patches are in lore alright[1]. Looks good, thanks!
[1] https://lore.kernel.org/linux-gpio/20240927-vfazio-mypy-v1-0-91a7c2e20884@xes-inc.com/
vfazio
commented
1 month ago @brgl thanks again for all of the feedback.
It will take me some time to respin the series. I will be away from work for 2 weeks later this month, so I will try to get something posted prior to my departure.
brgl
commented
1 month ago @vfazio no worries, as I said, the python releases are largely independent from the main libgpiod releases so I will publish v2.2 and then we can apply your series once it's ready.
vfazio
commented
3 weeks ago Just an update, I won't get this series re-posted before I leave so it will be after Nov 1. With the holidays coming up, I'll try to have this resubmitted no later than by FOSDEM. Worst case scenario, I'll do it while I'm at the conference.
brgl
commented
3 weeks ago No worries, we have lived without these changes for a long time, we'll survive another 3 months. :)
vfazio
commented
2 days ago I will probably be making some smaller patch submissions between now and this respin. Some of which may be dependencies for this series.
vfazio
commented
1 day ago I think i have this mostly reworked, i just have to add meaningful commit messages.
There are a number of "issues" with the python bindings when it comes to mypy compliance and ruff lint/format checks. Output below.
There are also a couple of files that are using absolute imports where the prevailing style in the library is relative imports.
Functionally i don't think it matters and I cannot find strong suggestions one way or the other. I've seen popular libraries use both mechanisms (httpx/httpcore use relative, fastapi uses absolute) so I think it's up to the maintainer to make a style choice.
@brgl let me know if you'd prefer to stick with relative vs absolute imports and we can address them in this series.