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

Design doc: allow to install packages using apt #8060

Merged
merged 4 commits into from May 4, 2021

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Mar 30, 2021

Related: #7566

@stsewd stsewd requested a review from a team March 30, 2021 22:58
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

This looks like a good approach 👍

.. note::

We can rename the key to be ``build.apt_packages`` or ``build.extra_packages``
so users aren't confused with Python packages or maybe having that option under the ``build`` key is enough.
Copy link
Member

Choose a reason for hiding this comment

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

Yea, I think we want this to be explicit. I'd say apt_packages, since that's the implementation detail required. Many packages have different names in eg. redhat repos.

Copy link
Member

Choose a reason for hiding this comment

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

We already talk about this at and came up with system_packages: #7566 (comment) . I'd vote to use it.

Copy link
Member Author

Choose a reason for hiding this comment

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

system_packages is already used in the python options https://docs.readthedocs.io/en/stable/config-file/v2.html#python-system-packages

Copy link
Member

Choose a reason for hiding this comment

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

Is that a problem? I think it's fine to have python.system_packages and build.system_packages.

Copy link
Member

Choose a reason for hiding this comment

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

Reading your code, https://github.com/readthedocs/readthedocs.org/pull/8065/files#diff-99dfd740f4f146e66e1c3709bebdc3db357a8c6defbf3e592434be4308f5d697R1156, I realized that another possible name could be build.system_dependencies

Copy link
Contributor

Choose a reason for hiding this comment

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

We only support apt right now but if we ever move to support custom docker containers, those custom containers may not support apt. I still vote for system_packages but I don't care enough to really fight it.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, but having ambiguous system_packages that aren't mapped to a specific namespace feels even more broken, doesn't it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have a strong opinion on this, but mostly because I hope we could allow to execute any commands in the future instead of relying on this option p:

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, but having ambiguous system_packages that aren't mapped to a specific namespace feels even more broken, doesn't it?

Maybe system_dependencies is more precise? Apt feels very much like an implementation detail.

Maybe Santos is right that long term we should get toward custom commands and maybe custom containers which seems closer to what the CI companies are doing.

Copy link
Member

Choose a reason for hiding this comment

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

Apt isn't an implementation detail. It's very user facing, because it's the namespace that the key represents. You can't just put "docker" or "git" in that field and have it work (as examples of packages that in the past have had weird ubuntu package names).

I agree though, that custom commands is the correct solution. Baby steps tho :)

.. _travis-build: https://github.com/travis-ci/travis-build
.. _travis-worker: https://github.com/travis-ci/worker

- A similar solution could be implemented using `AWS Lambda`_.
Copy link
Member

Choose a reason for hiding this comment

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

Lambda still has a lot of restrictions that wouldn't make it suitable for our use (eg. time & memory). I imagine it will get there in a year or two tho.


- We can allow to run the containers as root doing something similar to what Travis does:
They have one tool to convert the config file to a shell script (travis-build_),
and another that spins a docker container, executes that shell script and streams the logs back (travis-worker_).
Copy link
Member

Choose a reason for hiding this comment

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

I'd love to have much better UX around our build logs. Definitely something to keep in mind in the future.


Builds are run in a Docker container,
but the app controlling that container lives in the same server.
Allowing to execute arbitrary commands with super user privileges may introduce some security issues.
Copy link
Member

Choose a reason for hiding this comment

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

This is good to mention to provide context & understand the current solution 👍

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

LGTM! I'd prefer to use build.system_packages for the config which is cleaner to me.

.. note::

We can rename the key to be ``build.apt_packages`` or ``build.extra_packages``
so users aren't confused with Python packages or maybe having that option under the ``build`` key is enough.
Copy link
Member

Choose a reason for hiding this comment

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

We already talk about this at and came up with system_packages: #7566 (comment) . I'd vote to use it.

Comment on lines +76 to +80
Possible problems
-----------------

- Some users may require to pass some additional flags or install from a ppa.
- Some packages may require some additional setup after installation.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think these are problems, but limitations from the decisions we made.

docs/development/design/system-packages.rst Outdated Show resolved Hide resolved
Co-authored-by: Manuel Kaufmann <humitos@gmail.com>
Co-authored-by: Manuel Kaufmann <humitos@gmail.com>
@stsewd stsewd merged commit c7afd1a into master May 4, 2021
@stsewd stsewd deleted the system-packages-design-doc branch May 4, 2021 14:41
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.

None yet

5 participants