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(@angular-devkit/build-angular): ensure NG_PERSISTENT_BUILD_CACHE always creates a cache in the specified cache directory #21274

Merged
merged 1 commit into from
Jul 6, 2021

Conversation

terencehonles
Copy link

This change fixes NG_PERSISTENT_BUILD_CACHE sometimes creating cache entries that live outside of the cache directory by using a hex encoding rather than a base64 encoding. This error is caused because the base64 alphabet includes /. According to the webpack documentation 1 the default cacheLocation is: path.resolve(cache.cacheDirectory, cache.name) which means cache names with a leading / would remove the cacheDirectory altogether.

…` always creates a cache in the specified cache directory

This change fixes `NG_PERSISTENT_BUILD_CACHE` sometimes creating cache
entries that live outside of the cache directory by using a hex encoding
rather than a base64 encoding. This error is caused because the base64
alphabet includes `/`. According to the webpack documentation [1] the
default `cacheLocation` is:

  path.resolve(cache.cacheDirectory, cache.name)

Which means cache names with a leading `/` would remove the
`cacheDirectory` altogether.

[1]: https://webpack.js.org/configuration/other-options/#cachecachelocation
@google-cla google-cla bot added the cla: yes label Jul 3, 2021
@terencehonles
Copy link
Author

@alan-agius4 I'm not sure if base64 is preferred and a prefix should just be added to the cache name. Kicking off the CI to see what tests are on it.

Copy link
Collaborator

@alan-agius4 alan-agius4 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 we could also be encodeURIComponent

@alan-agius4 alan-agius4 added action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Jul 3, 2021
@terencehonles
Copy link
Author

True, but it sounds like there's not a strong reason to use base64 and it looks like it doesn't break any tests so it probably makes sense to just leave it as hex encoding. Node 16 looks like it has base64url, but I'm not sure it's worth picking the encoding based on the node version.

@clydin clydin merged commit 9abf44d into angular:12.1.x Jul 6, 2021
@terencehonles terencehonles deleted the fix-cache-name-error branch July 6, 2021 16:48
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Aug 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants