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

Support for socket AF_BLUETOOTH in python standard lib #445

Merged
merged 8 commits into from
Dec 31, 2019

Conversation

sergioisidoro
Copy link
Contributor

@sergioisidoro sergioisidoro commented Dec 21, 2019

⚠️ Disclaimer: I'm new around here. ⚠️

As mentioned in #444 , python 3 has native support for bluetooth sockets, with socket family AF_BLUETOOTH

However, it only has support for it if built with bluetooth.h in the dependencies

Before:

└─▪docker run -it python:3.7 python 
Unable to find image 'python:3.7' locally
3.7: Pulling from library/python
16ea0e8c8879: Pull complete 
50024b0106d5: Pull complete 
ff95660c6937: Pull complete 
9c7d0e5c0bc2: Pull complete 
29c4fb388fdf: Pull complete 
8659dae93050: Pull complete 
c8208a94eb32: Pull complete 
465b26a31c40: Pull complete 
84af071ddf6f: Pull complete 
Digest: sha256:4ce3a6ba2c2103d50ac59d18d46a484ad1b489b89609f548ef78f02d43eaecf9
Status: Downloaded newer image for python:3.7
Python 3.7.6 (default, Dec 20 2019, 22:36:14) 
[GCC 8.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from socket import AF_BLUETOOTH
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: cannot import name 'AF_BLUETOOTH' from 'socket'

After:

└─▪docker run -it sergio/py37 python
Python 3.7.6 (default, Dec 21 2019, 23:33:58) 
[GCC 8.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from socket import AF_BLUETOOTH
>>> AF_BLUETOOTH
<AddressFamily.AF_BLUETOOTH: 31>
>>> 

This might be a bit too much to add to the slim images, but since this is a native python support, I was wondering if it would make sense to include in these images.

Some mentions of the issue:

I have not ran update.sh. I have ran update.sh but the pip version commit hash is broken. Had to hard-code it to generate the Dockerfiles

This change does not apply to 2.x, but since 2.x is deprecated in 9 days anyway

@sergioisidoro
Copy link
Contributor Author

Update: On Ubuntu 18 python comes built with bluetooth support, so this change makes the behaviour similar to, for example, doing a apt install python

Copy link
Member

@tianon tianon left a comment

Choose a reason for hiding this comment

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

A few minor comments -- generally we try to keep the list of installed packages sorted so we can more easily notice duplicates (and it makes it easier to read). 👍

I'm +1 on the change overall, but we do need to clarify whether this is something that Python 2 actually supports or whether it's only for Python 3. 👍

@@ -38,6 +38,7 @@ RUN set -ex \
wget \
xz-utils \
zlib1g-dev \
libbluetooth-dev \
Copy link
Member

Choose a reason for hiding this comment

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

This list should be in sorted order. 👍

@@ -13,6 +13,7 @@ RUN apt-get update && apt-get install -y --no-install-recommends \
libssl-dev \
tk-dev \
uuid-dev \
libbluetooth-dev \
Copy link
Member

Choose a reason for hiding this comment

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

This list should be in sorted order. 👍

@@ -13,6 +13,7 @@ ENV PYTHONIOENCODING UTF-8
RUN apt-get update && apt-get install -y --no-install-recommends \
ca-certificates \
netbase \
libbluetooth-dev \
Copy link
Member

Choose a reason for hiding this comment

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

This should be down in the temporary list (not this persistent list), like in Dockerfile-slim.template -- also, is it confirmed that Python 2 does support this as well or is it Python 3 only?

Copy link
Member

Choose a reason for hiding this comment

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

Doh, just saw your note ("This change does not apply to 2.x, but since 2.x is deprecated in 9 days anyway") -- so this should be removed from all -caveman templates which should remove it from all 2.x versions. 👍

@@ -56,6 +56,7 @@ RUN set -ex \
util-linux-dev \
xz-dev \
zlib-dev \
bluez-dev \
Copy link
Member

Choose a reason for hiding this comment

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

This list should be in sorted order. 👍

@yosifkit
Copy link
Member

Since #442 was merged this now needs a rebase. We can help if you like.

@sergioisidoro
Copy link
Contributor Author

sergioisidoro commented Dec 24, 2019

Ok, if I understood correctly, I'll:

  • Rebase
  • Move the dep to temporarily dependencies
  • Lint so that the deps are ordered alphabetically
  • Double check that this does not apply to 2.x and if so, remove this from caveman templates
    Got it!

@@ -20,6 +20,7 @@ ENV PYTHON_VERSION %%PLACEHOLDER%%

RUN set -ex \
\
&& apt-get update && apt-get install -y --no-install-recommends libbluetooth-dev \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please note that I did not make anything regarding the savedAptMark and apt-mark showmanual.

I have very little clue about its meaning, but it seemed to be in the slim image.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for not being more clear -- this one was fine before (with libbluetooth-dev up above libssl-dev's comment line above), ala:

diff --git a/Dockerfile-debian.template b/Dockerfile-debian.template
index 4d56b25..8a14f49 100644
--- a/Dockerfile-debian.template
+++ b/Dockerfile-debian.template
@@ -9,6 +9,7 @@ ENV LANG C.UTF-8
 
 # extra dependencies (over what buildpack-deps already includes)
 RUN apt-get update && apt-get install -y --no-install-recommends \
+		libbluetooth-dev \
 # Python 3.4 on Stretch+ needs to use an older version of "libssl-dev" (these lines both get removed for every other combination)
 		libssl-dev \
 		tk-dev \

Happy to take over and make this change directly to your branch if you'd rather. 👍

Thanks for updating this! 🤘

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, my fault. Now that I re-read the comment it was clear that is was only referring to the slim image, not all Deb. 😅

I can also change it, since it's 5 min, but feel free to push to branch if you want :)
I'll stop force pushing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

I force pushed one more time to keep the update.sh as the last commit.
I don't know what is the merge strategy, but maybe squash and merge would be beneficial here.

Also, does that libssl sill make sense?
3.4 is not being rendered, so libssl never makes it to any of the images.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, nice catch! Maybe we should clean that out separately from this though. 👍

@sergioisidoro
Copy link
Contributor Author

Double checked that AF_BLUETOOTH is never mentioned in the python 2.x docs for the sockets family, so I removed the deps from the caveman templates

@tianon tianon merged commit bd779fa into docker-library:master Dec 31, 2019
docker-library-bot added a commit to docker-library-bot/official-images that referenced this pull request Dec 31, 2019
Changes:

- docker-library/python@2ab7979: Remove dead comment
- docker-library/python@de34fa5: Remove dead code (thanks @sergioisidoro!)
- docker-library/python@bd779fa: Merge pull request docker-library/python#445 from sergioisidoro/socket-af-bluetooth
- docker-library/python@d2a2b4f: Run update.sh
- docker-library/python@7d934af: Revert debian change
- docker-library/python@2eaf8c2: Fix mishap
- docker-library/python@a02c43c: Move libbluetooth-dev out of runtime deps
- docker-library/python@7044dac: Sort deps
- docker-library/python@10a3736: Remove caveman deps (not needed)
- docker-library/python@a23b410: Use libbluetooth-dev in debian images
- docker-library/python@e92918d: Support for socket AF_BLUETOOTH
nghiant2710 added a commit to nghiant2710/python-build that referenced this pull request Jul 6, 2020
Fixes balena-io-library/base-images#641.
Python should be compiled with libbluetooth, otherwise it cannot access
the AF_BLUETOOTH socket. This feature is in the official images (see docker-library/python#445) so we should support it too.

Change-type: patch
Signed-off-by: Trong Nghia Nguyen <nghiant2710@gmail.com>
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.

4 participants