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

Clean cache after smoke test #2207

Merged
merged 1 commit into from
Apr 2, 2025

Conversation

KobayashiAzusa
Copy link
Contributor

Description

Remove /tmp/node-compile-cache after smoke tests

Motivation and Context

The Node.js image, during the build process in the RUN layer, executed Node.js and generated the /tmp/node-compile-cache directory without deleting it. According to subsequent build steps, this directory was deleted in the next RUN, therefore it doesn't exist in the final image (though it's only marked as deleted in the current RUN layer - the original files still reside in the historical layers).

Testing Details

Example Output(if appropriate)

Types of changes

  • Documentation
  • Version change (Update, remove or add more Node.js versions)
  • Variant change (Update, remove or add more variants, or versions of variants)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Other (none of the above)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING.md document.
  • All new and existing tests passed.

@PeterDaveHello PeterDaveHello requested a review from Copilot March 30, 2025 09:26
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot wasn't able to review any files in this pull request.

Files not reviewed (3)
  • Dockerfile-alpine.template: Language not supported
  • Dockerfile-debian.template: Language not supported
  • Dockerfile-slim.template: Language not supported

Copy link
Member

@PeterDaveHello PeterDaveHello left a comment

Choose a reason for hiding this comment

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

I thought the cleanup was only for yarn? I tested node --version & npm --version locally, and also inserted one additional line into the Dockerfile to build the image as below:

&& echo "run \`ls -al /tmp/\`" 1>&2 && ls -al /tmp/ 1>&2 && echo 1>&2 \

None of the results look like we need this cleanup for node and npm smoke tests,

@KobayashiAzusa
Copy link
Contributor Author

I am not sure which one generated the cache, but it does exist in /tmp after smoke test in layer. This is also reported in #2199.

The patch removed /tmp/* , and cache files disappear.

@PeterDaveHello
Copy link
Member

PeterDaveHello commented Apr 2, 2025

@KobayashiAzusa you are right, I verified your thought, and I somehow inserted the test command at the wrong place, this is a good catch, thanks!

@PeterDaveHello PeterDaveHello self-requested a review April 2, 2025 20:17
@PeterDaveHello
Copy link
Member

Fun fact: there's no such issue with v18, v20, or even v21, but v22 and v23. That's why I couldn't reproduce the issue the first time. #2199 was very helpful in helping me notice that difference!

@PeterDaveHello
Copy link
Member

This won't hurt those versions that are not affected anyway.

@PeterDaveHello PeterDaveHello merged commit 48003d4 into nodejs:main Apr 2, 2025
1 check passed
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.

2 participants