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

Add output of system_dependencies install to build process #10229

Closed
Jacob-Stevens-Haas opened this issue Apr 7, 2023 · 5 comments
Closed

Add output of system_dependencies install to build process #10229

Jacob-Stevens-Haas opened this issue Apr 7, 2023 · 5 comments
Labels
Needed: documentation Documentation is required

Comments

@Jacob-Stevens-Haas
Copy link

Jacob-Stevens-Haas commented Apr 7, 2023

Details

The RTD documentation lists the steps of the build process that roughly correspond to the steps on a build's status page (e.g.). However, the system_dependencies step doesn't have an entry in the build logs.

Expected Result

Our build uses a sphinx extension that requires pandoc, itself a non-python dependency. Since we didn't include build.apt_packages in our config (didn't know about it), I would've expected the build to fail, as it does on a developer machine that does not have the pandoc apt package or windows install.

Actual Result

It built successfully. Obviously, wondering why things are working is better than wondering why they're broken, but even better would be to see the output of the system_dependencies step on our build status page. Apparently pandoc was added to rtd way back in 2014. But as another developer noted (and was confused about) in #4987, there's nothing in the RTD documentation (i could find) about pandoc being available, just that ancient lore.

Comments

In general, seeing dependency versions in build output feels like a good thing, unless there's a security issue to sharing that. It would have at least solved my and the other person's issue. Any thoughts?

Jacob-Stevens-Haas added a commit to dynamicslab/pysindy that referenced this issue Apr 7, 2023
Old sphinx version spec is very old.  Both RTD and CI interpret sphinx>=2.0.0 as
5.0.3, so it seems reasonable that requiring this for developers would only
serve to prevent doc build mistakes.

Also, added pandoc as a dev requirement, because it is a requirement for
building docs.  It has an external binary requirement (pandoc) which can't be
accomodated in a requirements file.  So I added info to the readme.rst

Bump python version to 3.8 in RTD since 3.7 is approaching EOL

FWIW, was confused about pandoc so added an issue to RTFD:
readthedocs/readthedocs.org#10229
Jacob-Stevens-Haas added a commit to dynamicslab/pysindy that referenced this issue Apr 7, 2023
Old sphinx version spec is very old.  Both RTD and CI interpret sphinx>=2.0.0 as
5.0.3, so it seems reasonable that requiring this for developers would only
serve to prevent doc build mistakes.

Also, added pandoc as a dev requirement, because it is a requirement for
building docs.  It has an external binary requirement (pandoc) which can't be
accomodated in a requirements file.  So I added info to the readme.rst

Bump python version to 3.8 in RTD since 3.7 is approaching EOL

FWIW, was confused about pandoc so added an issue to RTFD:
readthedocs/readthedocs.org#10229
@stsewd
Copy link
Member

stsewd commented Apr 10, 2023

Hi, this dependency is installed by default in the docker image https://github.com/readthedocs/readthedocs-docker-images/blob/03b076eafeceaa84de34701d9b0b87fc5ae39936/Dockerfile#L48, that's why it doesn't appear as an step in the build process.

But +1 on linking to our dockerfile maybe.

@stsewd stsewd added the Needed: documentation Documentation is required label Apr 10, 2023
@humitos
Copy link
Member

humitos commented Apr 11, 2023

@Jacob-Stevens-Haas

would be to see the output of the system_dependencies step on our build status page

This is shown in the build details page. These dependencies are the ones installed by the user when defining build.apt_packages.

The ones that are not shown are those installed by default in the Docker image used to build the documentation.

@stsewd

But +1 on linking to our dockerfile maybe.

Users shouldn't be aware of our Dockerfile at all. This is internal and should not be used/exposed, IMO. If anything, we could expose "all the packages and versions installed on each of this images" as a reference in our documentation.

@Jacob-Stevens-Haas
Copy link
Author

As a user, I'd have a minor preference for seeing package installs in the build status page if possible, because (a) that's where any troubleshooting starts and (b) versions will always stay up to date. But any add'l documentation would be welcome.

Users shouldn't be aware of our Dockerfile at all.

I don't have much investment in the decision and far from a security expert, but are you suggesting to take that repo private?

@humitos
Copy link
Member

humitos commented Apr 12, 2023

@Jacob-Stevens-Haas

As a user, I'd have a minor preference for seeing package installs in the build status page if possible, because (a) that's where any troubleshooting starts and (b) versions will always stay up to date

The packages are already installed in the Docker image, so they are not installed on each build.

I don't have much investment in the decision and far from a security expert, but are you suggesting to take that repo private?

Nope. I'm saying we should not point people to that repository nor tell them to explore how our Dockerfile works.

@humitos
Copy link
Member

humitos commented Jul 24, 2023

Users wanting to know exactly what are the packages installed on Read the Docs images at build time can execute the following command: dpkg -l | grep -E "^ii". This can be done via build.jobs.

@humitos humitos closed this as completed Jul 24, 2023
jpcurbelo pushed a commit to jpcurbelo/pysindy_fork that referenced this issue Apr 30, 2024
Old sphinx version spec is very old.  Both RTD and CI interpret sphinx>=2.0.0 as
5.0.3, so it seems reasonable that requiring this for developers would only
serve to prevent doc build mistakes.

Also, added pandoc as a dev requirement, because it is a requirement for
building docs.  It has an external binary requirement (pandoc) which can't be
accomodated in a requirements file.  So I added info to the readme.rst

Bump python version to 3.8 in RTD since 3.7 is approaching EOL

FWIW, was confused about pandoc so added an issue to RTFD:
readthedocs/readthedocs.org#10229
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needed: documentation Documentation is required
Projects
None yet
Development

No branches or pull requests

3 participants