Skip to content

Create a docker container for a single-line setup and rendering#2638

Merged
Girgias merged 8 commits into
php:masterfrom
afilina:docker-compose-up
Aug 7, 2023
Merged

Create a docker container for a single-line setup and rendering#2638
Girgias merged 8 commits into
php:masterfrom
afilina:docker-compose-up

Conversation

@afilina
Copy link
Copy Markdown
Contributor

@afilina afilina commented Aug 2, 2023

Use case: a casual contributor who wants to preview their changes before opening a PR.

  • There is currently no way to provide custom args to phd/render.php, because then we'd be back to complicated instructions.
  • I tried not to make any assumptions about the host system, so I don't use make.
  • This image is tied to an older commit of phd, since a (very) recent commit causes a crash on assert($this->cchunk["classsynopsis"]["legacy"] === true);. I'll just assume that it's a regression, since it fails on 2 machines and 2 PHP versions. I don't want to go down that rabbit hole today.

Fixes #2637

@afilina afilina requested review from Girgias and cmb69 August 2, 2023 20:18
@afilina
Copy link
Copy Markdown
Contributor Author

afilina commented Aug 2, 2023

@cmb69 Might be an alternative to #877

Comment thread .docker/Dockerfile
Copy link
Copy Markdown
Member

@TimWolla TimWolla 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 absolutely reasonable for a starter.

Comment thread .docker/Dockerfile Outdated
Comment thread .docker/Dockerfile Outdated
@Lewiscowles1986
Copy link
Copy Markdown
Contributor

php/phd#77 seems to be what broke phpd with something explicitly asserting a true which is not true.

@mallardduck
Copy link
Copy Markdown
Contributor

Once #2641 is merged it will fix docs-en builds.

Comment thread .docker/Dockerfile Outdated
Comment thread .docker/Dockerfile Outdated
Comment thread .docker/Dockerfile Outdated
Comment thread README
Comment thread README Outdated
Comment thread README Outdated
Comment thread .docker/Dockerfile Outdated
@Lewiscowles1986
Copy link
Copy Markdown
Contributor

Lewiscowles1986 commented Aug 3, 2023

All builds for me

I used the following edits to test depth 1 and not having a commit.

FROM php:8.2-cli

RUN apt-get update && \
    apt-get install -y git

WORKDIR /var/www

ARG PHPD_SELECTOR="--branch=master"
ARG DOC_BASE_SELECTOR="--branch=master"

RUN git clone --depth 1 https://github.com/php/phd.git ${PHPD_SELECTOR} &&  \
    git clone --depth 1 https://github.com/php/doc-base.git ${DOC_BASE_SELECTOR}

RUN echo 'memory_limit = 512M' >> /usr/local/etc/php/conf.d/local.ini

ARG PHD_RENDER_ARGS=" -f xhtml"
CMD php doc-base/configure.php && \
    php phd/render.php -d doc-base/.manual.xml -P PHP -o /var/www/en/output/ ${PHD_RENDER_ARGS}


The suggested edits have the whole thing build in under a minute (50 seconds), which is all clone depth.

You should be free to iterate in future.

The ARGS are optional, but if someone wants to fill this out with chm support or PDF, why not right?

afilina added 7 commits August 3, 2023 13:17
Use case: a casual contributor who wants to preview their changes before opening a PR.

There is currently no way to provide custom args to phd/render.php, because then we'd be back to complicated instructions.

I tried not to make any assumptions about the host system, so I don't use make.

This image is tied to an older commit of phd, since a (very) recent commit causes a crash on `assert($this->cchunk["classsynopsis"]["legacy"] === true);`. I'll just assume that it's a regression, since it fails on 2 machines and 2 PHP versions. I don't want to go down that rabbit hole today.
Comment thread .docker/Dockerfile
Copy link
Copy Markdown
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

I know nothing about Docker, but this seems sensible if people who know better are OK with the docker file.
Although I might move this more to doc-base. Would it be possible to have it take an argument to be able to render translations? (just a lang thing that defaults to en)

@afilina
Copy link
Copy Markdown
Contributor Author

afilina commented Aug 4, 2023

Although I might move this more to doc-base. Would it be possible to have it take an argument to be able to render translations? (just a lang thing that defaults to en)

That means that people would need to also clone doc-base in a sensible location, bringing us back to multiple steps and complex instructions (how to organize folders, where to run the command, etc.) I'm hoping to start small here, get a few people try it for feedback, then move it to dockerhub or something so that it wouldn't need to be in any repo. A bit like phpco works, solving the exact same setup problem for another project.

@Girgias
Copy link
Copy Markdown
Member

Girgias commented Aug 4, 2023

Although I might move this more to doc-base. Would it be possible to have it take an argument to be able to render translations? (just a lang thing that defaults to en)

That means that people would need to also clone doc-base in a sensible location, bringing us back to multiple steps and complex instructions (how to organize folders, where to run the command, etc.) I'm hoping to start small here, get a few people try it for feedback, then move it to dockerhub or something so that it wouldn't need to be in any repo. A bit like phpco works, solving the exact same setup problem for another project.

Right... I was just thinking have people "just" clone doc-base and let the docker compose do the cloning for them. But then one can't actually edit the docs.

Comment thread .docker/Dockerfile
Co-authored-by: Lewis Cowles <lewis+github@cowles.uk>
Copy link
Copy Markdown
Member

@ndossche ndossche left a comment

Choose a reason for hiding this comment

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

Looks legit to me

@Girgias Girgias merged commit 52a614e into php:master Aug 7, 2023
@Lewiscowles1986
Copy link
Copy Markdown
Contributor

🙌 very nice

@afilina afilina deleted the docker-compose-up branch August 8, 2023 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dockerize for easier local setup of the docs

6 participants