Actually use dependency caching in CI #310
Conversation
Codecov Report
@@ Coverage Diff @@
## master #310 +/- ##
=======================================
Coverage 79.64% 79.64%
=======================================
Files 163 163
Lines 8338 8338
=======================================
Hits 6641 6641
Misses 1343 1343
Partials 354 354 Continue to review full report at Codecov.
|
|
Does go/pkg/mod need to be persisted to workspace? Looks like that step is taking quite a while (1m) but shouldn't go/pkg/mod come from the cache? |
.circleci/config.yml
Outdated
| lint: | ||
| executor: golang | ||
| steps: | ||
| - attach_to_workspace | ||
| - run: | ||
| name: Lint | ||
| command: CMD="make lint" make -j4 for-all | ||
| command: CMD="make lint" make -j16 for-all |
There was a problem hiding this comment.
This is another thing I noticed. I don't think -j will increase parallelism here because the looping is being done in bash embedded in the make. I think we'd have to change how the make target works for the parallelism to work.
There was a problem hiding this comment.
for-all:
@$${CMD}
@set -e; for dir in $(ALL_MODULES); do \
(cd "$${dir}" && \
echo "running $${CMD} in $${dir}" && \
$${CMD} ); \
doneThere's no way for make to parallelize this afaict.
There was a problem hiding this comment.
It doesn't. Was working on a new target does actually runs everything in parallel. It's in now. I left for-all as is and named it for-all-make since it runs a make target.
There was a problem hiding this comment.
Got it to work but looks like golangci-lint doesn't like to run in parallel 🤦
Static check finished successfully
golangci-lint run
ERRO Parallel golangci-lint is running
../../Makefile.Common:95: recipe for target 'lint' failed
make[1]: *** [lint] Error 3
make[1]: Leaving directory '/home/circleci/project/exporter/honeycombexporter'
Makefile:67: recipe for target 'for-all-make-./exporter/honeycombexporter' failed
make: *** [for-all-make-./exporter/honeycombexporter] Error 2
make: *** Waiting for unfinished jobs....
ERRO Parallel golangci-lint is running
ERRO Parallel golangci-lint is running
../../Makefile.Common:95: recipe for target 'lint' failed
make[1]: *** [lint] Error 3
make[1]: Leaving directory '/home/circleci/project/exporter/splunkhecexporter'
ERRO Parallel golangci-lint is running
There was a problem hiding this comment.
Upgraded golangci-lint. Fixed now.
There was a problem hiding this comment.
Would for-all-target maybe be more clear what it's for and naming CMD in it TARGET? Just an idea.
There was a problem hiding this comment.
I thought of that but felt using same var was slightly nicer when browsing CLI history and just changing for-all to for-all-make. But it does indeed look better. Will change.
There was a problem hiding this comment.
One other option is to make for-all itself support parallelism using xargs:
.PHONY: for-all
PARALLEL?=1
for-all:
@echo $(ALL_MODULES) | xargs -I% -n1 -P$(PARALLEL) sh -ec "echo 'running $(CMD) in %' ; cd % && $(CMD)"The native make make one will probably be faster (less forking) but I'd be surprised if it's by much. Having the for-all one support parallel is nice for cases where the CMD isn't a Makefile target. Both probably have their uses. Don't have to add it if you don't need it but I probably will at some point.
There was a problem hiding this comment.
Actually maybe the native one won't be faster since it's still doing an invocation for each subdirectory 🤔
There was a problem hiding this comment.
Don't see much value in changing it. People might be using it in the local dev flow so don't want to cause any annoyances. make based parallel build targets works as expected and integrates with the intuitive -jX flag as well so I think we should keep that separate.
It doesn't have to come from the workspace but we have to restore the workspace anyway in these jobs (to get source code and installed binaries). We could not persist deps to workspace and restore cache instead every time but I don't think that would speed things up considerably as downloading deps cache also taking quite a while. |
|
Tested deps as part of workspace vs cache. Cache turned out to be faster (6 minutes vs 8 minutes). I still feel using workspace is probably the more "correct" way to do and it being slow is just a CircleCI implementation detail. They should improve workspace performance at least when running in the same workflow. Since the difference is significant enough, I'll use cache to restore dependencies in each job for now. |
Makefile
Outdated
| @@ -60,6 +60,13 @@ for-all: | |||
| $${CMD} ); \ | |||
| done | |||
|
|
|||
| .PHONY: allmodules $(ALL_MODULES) | |||
There was a problem hiding this comment.
No. Left behind from testing.
.circleci/config.yml
Outdated
| lint: | ||
| executor: golang | ||
| steps: | ||
| - attach_to_workspace | ||
| - run: | ||
| name: Lint | ||
| command: CMD="make lint" make -j4 for-all | ||
| command: CMD="make lint" make -j16 for-all |
There was a problem hiding this comment.
Would for-all-target maybe be more clear what it's for and naming CMD in it TARGET? Just an idea.
This commit enables dependency caching in CI. It uses root go.sum file to detect changes in dependencies but this is a good enough first step. We can concat go.sum files from all modules and hash that to detect changes in future if needed.
|
@owais lgtm, is this ready for merge? |
|
Yes, it's ready. Thanks for the review. |
This commit enables dependency caching in CI. It uses root go.sum file to detect changes in dependencies but this is a good enough first step. We can concat go.sum files from all modules and hash that to detect changes in future if needed.
* Refactor extensions to new config format * Refactored PProf * Refactor health-check extension * Refactor service application for extensions * Refactor zPages and update docker compose example * Fix staticcheck issue * Update README and other comments/docs * Remove tabs from yaml in extension/README.md * Remove closeFn field from service and related functions * PR Feedback 00 * Add doc.go to extensiontest package * Move getAvailablePort to testutils package * Add tests for testutils functions getting available endpoints
This commit enables dependency caching in CI. It uses root go.sum file
to detect changes in dependencies but this is a good enough first step.
We can concat go.sum files from all modules and hash that to detect
changes in future if needed.