Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UUID._init_from_int() size bugfix in uuid_.py #65

Merged
merged 3 commits into from
Mar 20, 2024
Merged

Conversation

lff5
Copy link
Contributor

@lff5 lff5 commented Nov 28, 2023

Hi

I think the pull request doesn't require much explaining: 16bit uuids and 32 bit uuids will always get length of 32 bits.

I am using this to DFU via BLE nRF52 microcontrollers.
We have been using the library for years, just found this when installed the libraries on new Raspberry Pi 4 (this change landed 7 months ago).

Where my actual problem happened was:
adafruit_ble.__init__.py contains _discover_remote() which calls
self._bleio_connection.discover_remote_services((uuid.bleio_uuid,))
Filtered discovery of remote services gives no results if UUID size is falsely 32 instead of 16 bit.

Btw thanks for amazing lib.

@lff5
Copy link
Contributor Author

lff5 commented Nov 28, 2023

CI failure does not seem to be related to my change.
AttributeError: module 'pkgutil' has no attribute 'ImpImporter'. Did you mean: 'zipimporter'?

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Makes sense! Thank you!

@tannewt
Copy link
Member

tannewt commented Nov 28, 2023

CI failure does not seem to be related to my change. AttributeError: module 'pkgutil' has no attribute 'ImpImporter'. Did you mean: 'zipimporter'?

I've asked on the Discord about it. Someone else may know.

@roman-yepishev
Copy link

This also fixes an exception when creating a Characteristic with 16 bit UUID:

    a = StructCharacteristic(struct_format='<H', uuid=StandardUUID(0x1234))
    print(a)
.../_bleio/uuid_.py", line 156, in pack_into
    raise IndexError("Buffer offset too small")
IndexError: Buffer offset too small

@dhalbert
Copy link
Contributor

dhalbert commented Mar 19, 2024

@jepler I am trying to fix the test failure here, which is the dreaded:
AttributeError: module 'pkgutil' has no attribute 'ImpImporter'. Did you mean: 'zipimporter'?

@makermelissa originally encountered this, thought it was a pre-commit problem, and was rebuffed here:
pre-commit/pre-commit#3041
I pinned a Python version to 3.11 to work around the problem temporarily:
adafruit/workflows-circuitpython-libs#32

You made a comment in the `pre-commit issue above:
pre-commit/pre-commit#3041 (comment)
implying it might be a pylint issue
but then it was locked.

There was also the remark that it was due to using virtualenv instead of venv, but I can't see that we are doing that.
Also there was fingerpointing between pip and setuptools, seemed to be setuptools. But we don't pin setuptools that I can see.

Do you remember this, or understand this issue more deeply? I am just going in circles, with detours to stackoverflow and various github issues in the Python world. I don't see anything we are requiring or pinning that might be causing this problem. I would like to remove the requirement to pin to Python 3.11.

Might be due to something in this repo, something in:
https://github.com/adafruit/workflows-circuitpython-libs
or something in
https://github.com/adafruit/actions-ci-circuitpython-libs
or pylint
but I cannot find whatever it is that is causing the issue.
Maybe when running in Ubuntu 22.04 in the runners we need to update pip or setuptools when using Python 3.12. But I am not clear about what to do or why the defaults are not working.

Also I'll ping @tekktrik in case he has an idea.

@tekktrik
Copy link
Member

Nothing comes to mind immediately, but I can also do some digging. I don't think I've encountered this specific failure, however, though. It does look like its setuptools though (or something using it) since Python 3.12 does in fact remove pkgutil.ImpImporter and this kind of code is now contained within a hasattr() block so it doesn't do this. It looks just like setuptools can't even get imported correctly. Does upgrading pip and/or setuptools help?

@tekktrik
Copy link
Member

I can look into this in the evening, I have suspect we can pin either upgrade setuptools or pip, or pin virutalenv to a version and it should get it to work.

@dhalbert
Copy link
Contributor

I can look into this in the evening, I have suspect we can pin either upgrade setuptools or pip, or pin virutalenv to a version and it should get it to work.

Thanks. It is not clear to me whether we're using the /usr/bin pip or the Python 3.12 one. And I don't see that virtualenv is mentioned at all anywhere. Our pylint is quite new. pipensure was mentioned somwhere as something that might help here, but I haven't seen any examples of using that in GitHub CI actions.

@tekktrik
Copy link
Member

virtualenv is a dependency of pre-commit itself, so it might be that it's using an outdated version of that for some reason (though setuptools still looks older than it is now). I'll do some digging tonight and see if I can unravel it.

@dhalbert
Copy link
Contributor

Maybe we need to fetch a virtualenv that's compatible with 3.12

@tekktrik
Copy link
Member

Using pipx to install pre-commit solves the issue. I can't seem to find how its pulling an older virtualenv and the only thing I can think of is that its somehow using the system one from apt which would be at 3.10, but that's a guess. I could hackaway by using an opened up version of pre-commit where I hack away at it, but not sure if the pipx solution is good enough.

@justmobilize
Copy link
Contributor

Build fixed in #67

Copy link
Contributor

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

After a very wide detour involving #67, we can now merge this fix. Thank you @lff5 !

@dhalbert dhalbert merged commit c3aec28 into adafruit:main Mar 20, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants