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

fix: Ensure PCNTL extension is always installed in Docker #7782

Merged

Conversation

Wirone
Copy link
Member

@Wirone Wirone commented Jan 26, 2024

We need signals support both in local development and in official releases.

Related to #7783, but does not completely resolve it.


Explanation:

  • we need pcntl extension both for official releases and local development, so it's extracted to the lowest base layer
  • Composer is required for local development and for preparing Docker releases, so it's extracted to base-dev layer
  • vendor layer is only an intermediate layer used to COPY --from to copy pre-installed vendor packages to the Docker release
  • dist is a layer with official Docker releases, built on top of base (with pcntl), with vendors copied from vendor
  • dev is a top layer for local development, built od top of base-dev (with pcntl and Composer)

@Wirone Wirone added the topic/docker Docker distribution, development runtime label Jan 26, 2024
@Wirone Wirone self-assigned this Jan 26, 2024
@Wirone Wirone changed the title build: Move installation of PCNTL to the base layer fix: Move installation of PCNTL to the base layer Jan 26, 2024
@Wirone
Copy link
Member Author

Wirone commented Jan 26, 2024

FYI: we currently have problem with ctrl+C signal anyway (does not stop the command even when run natively on the host), it's not related to Docker and must be fixed independently. Will create issue for that and will look at it.

Edit: it seems like killing the processes makes my terminal working bad and signals did not work for native execution, but when I've checked in fresh terminal (PHAR distributions, native call from the source code, call inside Docker container) and ctrl+C is handled properly. So it seems there's only problem when running via docker run.

@coveralls
Copy link

coveralls commented Jan 26, 2024

Coverage Status

coverage: 94.753%. remained the same
when pulling b8826ae on Wirone:codito/docker-with-signals-support
into aafe887 on PHP-CS-Fixer:master.

@Wirone Wirone marked this pull request as draft January 26, 2024 13:17
@Wirone Wirone force-pushed the codito/docker-with-signals-support branch from da4e039 to 7bef436 Compare January 26, 2024 13:22
@Wirone Wirone marked this pull request as ready for review January 26, 2024 13:59
@Wirone Wirone changed the title fix: Move installation of PCNTL to the base layer fix: Ensure PCNTL extension is always installed in Docker Jan 26, 2024
@Wirone Wirone requested a review from julienfalque January 29, 2024 11:37
Copy link
Member

@julienfalque julienfalque left a comment

Choose a reason for hiding this comment

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

LGTM, although I didn't actually test the changes.

We need signals support both in local development and in official releases.
@Wirone Wirone force-pushed the codito/docker-with-signals-support branch from 7bef436 to b8826ae Compare February 3, 2024 10:55
@Wirone Wirone enabled auto-merge (squash) February 3, 2024 10:58
@Wirone Wirone disabled auto-merge February 3, 2024 10:59
@Wirone Wirone enabled auto-merge (squash) February 3, 2024 11:00
@Wirone Wirone merged commit 1fc90da into PHP-CS-Fixer:master Feb 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic/docker Docker distribution, development runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants