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

Calculate Docker cache hash precisely from Dockerfile's dependencies #59253

Merged
merged 1 commit into from
Mar 20, 2019

Conversation

kennytm
Copy link
Member

@kennytm kennytm commented Mar 17, 2019

#58549 changed the Docker cache calculation to include every file under src/ci/docker, so that when files under dist-x86_64-linux is changed, its dependent image dist-i686-linux will also be rebuilt.

However, this ultraconservative solution caused the dist-i686-linux to be rebuilt every time an irrelevant Dockerfile (e.g. the PowerPC ones) is changed, which increases the building time beyond 3 hours and forcing a spurious but expected failure.

This commit instead parses the Dockerfile itself and look for the actual dependencies. The scripts needs to be copied into the Docker image, which must be done with the COPY command, so we just need to find all lines with a COPY command and add the source file into the hash calculator.

Note: this script only handles single-lined COPY command in the form COPY src1 src2 src3 dst, since these are the only variant used inside this repository.

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 17, 2019
@kennytm
Copy link
Member Author

kennytm commented Mar 17, 2019

r? @pietroalbini or @Mark-Simulacrum

@pietroalbini
Copy link
Member

Seems good to me.

@bors r+

@bors
Copy link
Contributor

bors commented Mar 17, 2019

📌 Commit 3cb77ac751a12680f20c4a9c9a7400106ffbe60e has been approved by pietroalbini

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 17, 2019
@pietroalbini
Copy link
Member

No, wait, it behaves weirdly when tested locally, let me investigate.

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 17, 2019
Copy link
Member

@pietroalbini pietroalbini left a comment

Choose a reason for hiding this comment

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

Other than the two (minor) comments you forgot to include the Dockerfile itself in the hash.

src/ci/docker/run.sh Outdated Show resolved Hide resolved
src/ci/docker/run.sh Show resolved Hide resolved
@kennytm kennytm force-pushed the precise-docker-cache-hash branch from 3cb77ac to d14f923 Compare March 17, 2019 14:00
@bors

This comment has been minimized.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 17, 2019
`src/ci/docker`, so that when files under `dist-x86_64-linux` is changed,
its dependent image `dist-i686-linux` will also be rebuilt.

However, this ultraconservative solution caused the `dist-i686-linux` to
be rebuilt every time an irrelevant Dockerfile (e.g. the PowerPC ones) is
changed, which increases the building time beyond 3 hours and forcing
a spurious but expected failure.

This commit instead parses the Dockerfile itself and look for the actual
dependencies. The scripts needs to be copied into the Docker image, which
must be done with the COPY command, so we just need to find all lines with
a COPY command and add the source file into the hash calculator.

Note: this script only handles single-lined COPY command in the form
`COPY src1 src2 src3 dst`, since these are the only variant used inside
this repository.
@kennytm kennytm force-pushed the precise-docker-cache-hash branch from d14f923 to 07aee1d Compare March 17, 2019 15:19
@bors

This comment has been minimized.

@kennytm
Copy link
Member Author

kennytm commented Mar 17, 2019

@bors r=pietroalbini

@bors

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Mar 17, 2019

📌 Commit 07aee1d has been approved by pietroalbini

kennytm added a commit to kennytm/rust that referenced this pull request Mar 19, 2019
…r=pietroalbini

Calculate Docker cache hash precisely from Dockerfile's dependencies

rust-lang#58549 changed the Docker cache calculation to include every file under `src/ci/docker`, so that when files under `dist-x86_64-linux` is changed, its dependent image `dist-i686-linux` will also be rebuilt.

However, this ultraconservative solution caused the `dist-i686-linux` to be rebuilt every time an irrelevant Dockerfile (e.g. the PowerPC ones) is changed, which increases the building time beyond 3 hours and forcing a spurious but expected failure.

This commit instead parses the Dockerfile itself and look for the actual dependencies. The scripts needs to be copied into the Docker image, which must be done with the COPY command, so we just need to find all lines with a COPY command and add the source file into the hash calculator.

Note: this script only handles single-lined COPY command in the form `COPY src1 src2 src3 dst`, since these are the only variant used inside this repository.
bors added a commit that referenced this pull request Mar 20, 2019
Rollup of 5 pull requests (all of which changes `src/ci/docker`)

Successful merges:

 - #58986 ([CI] Update binutils for powerpc64 and powerpc64le)
 - #59038 (Track embedded-book in the toolstate)
 - #59055 (CI: Set job names.)
 - #59253 (Calculate Docker cache hash precisely from Dockerfile's dependencies)
 - #59257 (Update CI configuration for building Redox libraries)

Failed merges:

r? @ghost
@bors bors merged commit 07aee1d into rust-lang:master Mar 20, 2019
@kennytm kennytm deleted the precise-docker-cache-hash branch April 12, 2019 04:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants