Slight build improvements #675
Conversation
|
For this project, you should put the gain in the PR description |
| @@ -27,6 +28,3 @@ task test(type: NpmTask) { | |||
| inputs.file 'package-lock.json' | |||
| } | |||
|
|
|||
| clean { | |||
There was a problem hiding this comment.
I don't think this is correct to do that. The clean should give us the guarantee that we're starting from a clean slate.
Maybe the solution is to not call clean when we build in CI
There was a problem hiding this comment.
gradle clean removes build artifacts, not downloaded dependencies. Removing this line retains that behavior so this should be in line with gradle convention.
w.r.t performance, removing node_modules forces us to re-run npm install (install deps) after clean even if nothing in the packages.json/lock changed, which takes a non trivial time locally (30s+). This is different than java behavior where running clean removes build artifacts but not dependencies.
WDYT?
airbyte-webapp/build.gradle
Outdated
| @@ -14,6 +14,7 @@ npm_run_build { | |||
| inputs.file 'package.json' | |||
| inputs.file 'package-lock.json' | |||
|
|
|||
| outputs.cacheIf { true } | |||
There was a problem hiding this comment.
What trade off are we making here?
There was a problem hiding this comment.
if none of the inputs have changed since the last time we built, but the build artifacts are not present in this current build (which would happen if PR#1's build generated the react app, then PR#2 makes a totally unrelated change) then we can re-use the output from PR#1's build instead of rebuild. Since building the react app takes 40-50s (see example in this build scan). this saves a non-trivial amount of time.
There was a problem hiding this comment.
Although (and maybe this is the point you were initially making) since the caching is decoupled from the actual system building the code (npm) new inputs could be added without gradle realizing, and this would result in incorrect artifacts.
There was a problem hiding this comment.
Does it mean it is safe or unsafe?
There was a problem hiding this comment.
ultimately no because Gradle and NPM can go out of sync (one day we could add a new "source" directory that is no longer reflected accurately by the inputs specified in Gradle), for safety I'll remove it.
airbyte-webapp/build.gradle
Outdated
| @@ -14,6 +14,7 @@ npm_run_build { | |||
| inputs.file 'package.json' | |||
| inputs.file 'package-lock.json' | |||
|
|
|||
| outputs.cacheIf { true } | |||
There was a problem hiding this comment.
Does it mean it is safe or unsafe?
|
% the webapp output thingy |
What
Small build improvements:
workflow.ymlSome of these came up while looking into #621