-
Notifications
You must be signed in to change notification settings - Fork 31
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
Add caching of node_modules and eslintcache to Github Actions #4
Merged
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
433bda5
Add more caching to build actions
mattrunyon 50e5c8f
Add missing space to actions file. Conveniently reruns check to test …
mattrunyon 90d1c21
Try removing lerna hoist, node_modules cache, and parallel tests
mattrunyon 4929ee6
Readd hoist, bootstrap cache. Set eslint to use content instead of me…
mattrunyon 290d54f
Hopefully fix for linter cache file globs
mattrunyon c16951d
Add maxWorkers=2 for unit tests
mattrunyon 58abf59
Trying runInBand for tests
mattrunyon 1c150bf
Remove runInBand for final test
mattrunyon 58f9088
Swap build/test so packages aonly need to be built once
mattrunyon 2c39c9f
Update caching rule globs and set maxWorkers=2 for tests
mattrunyon 9e0b4e3
Hash all package.json for skipping npm ci
mattrunyon File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not very knowledgeable about this repo or NPM, but shouldn't
npm ci
be a fast no-op if node_modules is already populated?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't take too long (it actually only installs the dependencies of the root project), but the post install script which uses lerna to bootstrap and link the packages (and install their much larger list of dependencies) is slow (60-120s).
I know we can speed up lerna bootstrap by removing the hoist flag at the cost of space from redundant dependencies being installed in multiple packages. It will still take 45-60s though I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, using the presence of a cache hit to skip a full step is a bit hacky, and in general pretty dangerous. Is there any sort of lerna cache we could take advantage of? Is there a more appropriate task that we should be running, and we shouldn't need to do post install / lerna stuff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it's a bit hacky. I don't know if it's necessarily that dangerous though since it's matching only if the hash of all package-locks is unchanged. The actual files can still change and be used correctly since lerna symlinks node_modules within the project (e.g. client-ui depends on grid.
node_modules/@deephaven/grid
is symlinked topackages/grid
). If package-lock hasn't changed then that indicates the exact dependencies are the same as they were before.Using yarn might fix part of the speed issue, but I was having other problems with yarn locally with the license files being symlinks (it couldn't find the file).
The vast majority of our PRs (90%+, probably higher) don't touch package-lock files. If we can speed up lerna + npm then this wouldn't be necessary, but npm is painfully slow when doing a no-op install.
Also I think
npm ci
actually deletes node_modules to start. Which it should only be deleting the small set of dependencies from the root. That on its own takes ~10 seconds for me locally when I remove the postinstall script.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@devinrsmith why is using the cache hit to skip a step hacky? That's exactly what this action is for: https://github.com/actions/cache#skipping-steps-based-on-cache-hit
@mattrunyon There's no more sym linked license files so you should be good from there to try yarn again. I think
npm ci
should be clearing all the node_modules first, to make sure it has a false pass when a package is removed or something.