Skip to content

Enable docker build by only cloning the repository #604

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.

Sign up for GitHub

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

Open
Checker8763 wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Enable docker build by only cloning the repository #604

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1f86cd6
Update setup.py
Checker8763 May 17, 2025
fff5bc8
Update Dockerfile
Checker8763 May 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions docker/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,17 @@
FROM python
FROM python:3-slim AS builder

COPY websockify-*.tar.gz /
WORKDIR /build/websockify

RUN python3 -m pip install websockify-*.tar.gz
RUN rm -rf /websockify-* /root/.cache
COPY ./* /build/websockify

RUN pip3 install setuptools && \
python3 setup.py sdist --dist-dir docker/

# Actual final image
FROM python:3-slim
Copy link
Copy Markdown
Member

@CendioOssman CendioOssman May 26, 2025

Choose a reason for hiding this comment

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

What are we losing in this switch? We have a bunch of optional dependencies, like numpy, that users probably want.

Copy link
Copy Markdown
Author

@Checker8763 Checker8763 May 26, 2025

Choose a reason for hiding this comment

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

There is essentially nothing lost, only gained.
With the slim version the final image size reduced by only having the necessary runtime requirenments for python.
With specifying version 3 the builds are more explicit which helps anybody reading the Dockerfile, and making the build more predictable by actually being explicit to the dependend python version (Imagine python 4 is released and the dockerfile simply states python with an implicit latest).
By using the slim version there is less traffic ~1.2GB vs ~200MB and less stored.

Also by splitting the build into two stages the size is also reduced by only copying the necessary files to the final image, leaving any cache, temporary files etc. behind.

Copy link
Copy Markdown
Author

@Checker8763 Checker8763 May 26, 2025

Choose a reason for hiding this comment

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

Forgott that there is nothing lost, the full requirenments are installed.

Only the base Image i think Debian or Ubuntu, is not installed in a full fledged way but in a minimal with python runtime.

Copy link
Copy Markdown
Member

@CendioOssman CendioOssman Jun 3, 2025

Choose a reason for hiding this comment

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

Okay, it seems like the Python side are indeed the same.

Copy link
Copy Markdown
Author

@Checker8763 Checker8763 Jun 8, 2025

Choose a reason for hiding this comment

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

When I run the build in the "specific_package" branch in the git root with the command

docker build -t websockify -f docker/Dockerfile .

I get this error:

 > [builder 4/4] RUN pip3 install setuptools &&     python3 setup.py sdist --dist-dir docker/:
3.736 Collecting setuptools
3.786   Downloading setuptools-80.9.0-py3-none-any.whl.metadata (6.6 kB)
3.977 Downloading setuptools-80.9.0-py3-none-any.whl (1.2 MB)
4.359    ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 1.2/1.2 MB 2.6 MB/s eta 0:00:00
4.373 Installing collected packages: setuptools
5.149 Successfully installed setuptools-80.9.0
5.149 WARNING: Running pip as the 'root' user can result in broken permissions and conflicting behaviour with the system package manager, possibly rendering your system unusable. It is recommended to use a virtual environment instead: https://pip.pypa.io/warnings/venv. Use the --root-user-action option if you know what you are doing and want to suppress this warning.
5.540 running sdist
5.540 running egg_info
5.545 creating websockify.egg-info
5.551 writing websockify.egg-info/PKG-INFO
5.551 writing dependency_links to websockify.egg-info/dependency_links.txt
5.552 writing entry points to websockify.egg-info/entry_points.txt
5.552 writing requirements to websockify.egg-info/requires.txt
5.552 writing top-level names to websockify.egg-info/top_level.txt
5.552 writing manifest file 'websockify.egg-info/SOURCES.txt'
5.558 error: package directory 'websockify' does not exist
------
Dockerfile:7
--------------------
   6 |
   7 | >>> RUN pip3 install setuptools && \
   8 | >>>     python3 setup.py sdist --dist-dir docker/
   9 |
--------------------
ERROR: failed to solve: process "/bin/sh -c pip3 install setuptools &&     python3 setup.py sdist --dist-dir docker/" did not complete successfully: exit code: 1

Copy link
Copy Markdown
Author

@Checker8763 Checker8763 Jun 8, 2025

Choose a reason for hiding this comment

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

It seems to be tied to the environment, if I wrap the statements in a shell script and execute them with bash it works just fine, but when executed as a plain command in the docker file in a RUN statement it somehow breaks...

Copy link
Copy Markdown
Author

@Checker8763 Checker8763 Jun 8, 2025

Choose a reason for hiding this comment

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

I have debugged a bit further and found that find_packages() resolves to []

When I hardcode packages=[] it builds just fine.

Copy link
Copy Markdown
Author

@Checker8763 Checker8763 Jun 8, 2025

Choose a reason for hiding this comment

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

I have found the following section in the python docs:

~ https://docs.python.org/3.11/distutils/setupscript.html#listing-whole-packages

The keys to this dictionary are package names, 
and an empty package name stands for the root package. 
The values are directory names relative to your distribution root. 
In this case, when you say packages = ['foo'], 
you are promising that the file lib/foo/__init__.py exists.

Copy link
Copy Markdown
Author

@Checker8763 Checker8763 Jun 8, 2025

Choose a reason for hiding this comment

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

While reading in https://setuptools.pypa.io/en/latest/userguide/package_discovery.html

Another workaround I have found was to include package_dir = {"websockify": ""}, or package_dir = {"websockify": "."}, or package_dir = {"websockify": "/"},

This seems to be the least required amount of changes, to make the dockerfile work while leaving the statements in the dockerfile unchanged.

Copy link
Copy Markdown
Member

@CendioOssman CendioOssman Jun 12, 2025

Choose a reason for hiding this comment

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

I'm afraid I cannot reproduce the issue here. And it really should work. The documentation you refer to states:

Thus, when you say packages = ['foo'] in your setup script, you are promising that the Distutils will find a file foo/__init__.py (which might be spelled differently on your system, but you get the idea) relative to the directory where your setup script lives.

And since we say packages=['websockify'], we claim that there is a websockify directory next to setup.py. Which there is.

It seems like there is something more in your environment that is fouling thing.

COPY --from=builder /build/websockify/websockify-*.tar.gz /
RUN python3 -m pip install websockify-*.tar.gz && \
rm -rf /websockify-* /root/.cache
Comment thread
Checker8763 marked this conversation as resolved.

VOLUME /data

Expand Down
2 changes: 1 addition & 1 deletion setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
author="Joel Martin",
author_email="github@martintribe.org",

packages=['websockify'],
packages=find_packages(),
Copy link
Copy Markdown
Member

@CendioOssman CendioOssman May 26, 2025

Choose a reason for hiding this comment

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

Not sure this is a plus, given we have such a simple project. Explicitly listing them gives you some protection against including any random junk that people might have in their working copy.

Copy link
Copy Markdown
Author

@Checker8763 Checker8763 May 26, 2025

Choose a reason for hiding this comment

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

I am not sure why, but I only got it to work succesful this way.

When explicity stating the package, the build always fails with a message that the folder or package can not be found, even if they have the exact same name as the directory it is in...

I can do some tests again and give the error message maybe somone meight have more insight and makes the build without needing this change.

Copy link
Copy Markdown
Member

@CendioOssman CendioOssman Jun 3, 2025

Choose a reason for hiding this comment

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

Please do and share the error here.

Copy link
Copy Markdown
Author

@Checker8763 Checker8763 Jun 8, 2025

Choose a reason for hiding this comment

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

#604 (comment)

I posted into the wrong comment...

include_package_data=True,
install_requires=[
'numpy', 'requests',
Expand Down