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.

Sign up for GitHub

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

Slight build improvements #675

Merged
merged 12 commits into from
Oct 22, 2020
Merged

Slight build improvements #675

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
e6ba788
turn on caching
sherifnada Oct 21, 2020
7e9875f
cache docker layers
sherifnada Oct 21, 2020
0b1d0f3
use gradle daemon
sherifnada Oct 21, 2020
e14fe8e
make a small docker file
sherifnada Oct 21, 2020
5b60541
spotty
sherifnada Oct 21, 2020
2f817b7
random gen
sherifnada Oct 21, 2020
bde57e3
rename
sherifnada Oct 21, 2020
b8c24ee
docekr build
sherifnada Oct 21, 2020
21b537f
readd gradle
sherifnada Oct 21, 2020
441bf46
import changes from experiments branch
sherifnada Oct 21, 2020
9c5dfa8
Merge branch 'master' of github.com:airbytehq/airbyte into sherif/spe…
sherifnada Oct 21, 2020
c3d8be7
Update build.gradle
sherifnada Oct 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions .github/workflows/gradle.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@ on:

jobs:
build:
env:
GRADLE_PATH: /tmp/gradle
runs-on: ubuntu-latest
steps:
- name: Checkout Airbyte
Expand All @@ -17,7 +15,7 @@ jobs:
- name: Cache java deps
uses: actions/cache@v2
with:
path: ${{ env.GRADLE_PATH }}
path: ~/.gradle
key: gradle-${{ hashFiles('**/*.gradle') }}
restore-keys: |
gradle-
Expand Down Expand Up @@ -45,17 +43,17 @@ jobs:
STRIPE_INTEGRATION_TEST_CREDS: ${{ secrets.STRIPE_INTEGRATION_TEST_CREDS }}

- name: Build
run: ./gradlew build --no-daemon -g ${{ env.GRADLE_PATH }}
run: ./gradlew --no-daemon build

- name: Ensure no file change
run: git status --porcelain && test -z "$(git status --porcelain)"

- name: Run Integration Tests
run: ./gradlew integrationTest --no-daemon -g ${{ env.GRADLE_PATH }}
run: ./gradlew --no-daemon integrationTest

- name: Build Core Docker Images
if: success() && github.ref == 'refs/heads/master'
run: ./gradlew composeBuild --no-daemon -g ${{ env.GRADLE_PATH }}
run: ./gradlew --no-daemon composeBuild
env:
GIT_REVISION: ${{ github.sha }}

Expand Down
4 changes: 1 addition & 3 deletions airbyte-webapp/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ npm_run_build {
inputs.file 'package.json'
inputs.file 'package-lock.json'

outputs.cacheIf { true }
Copy link
Contributor

Choose a reason for hiding this comment

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

What trade off are we making here?

Copy link
Contributor Author

@sherifnada sherifnada Oct 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it mean it is safe or unsafe?

Copy link
Contributor Author

@sherifnada sherifnada Oct 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

outputs.dir project.buildDir
}
assemble.dependsOn npm_run_build
Expand All @@ -27,6 +28,3 @@ task test(type: NpmTask) {
inputs.file 'package-lock.json'
}

clean {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

OK then, I am sold!

delete "${projectDir}/node_modules/"
}
1 change: 0 additions & 1 deletion airbyte-workers/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ dependencies {

implementation project(':airbyte-config:models')
implementation project(':airbyte-db')
implementation project(':airbyte-integrations')
implementation project(':airbyte-json-validation')
implementation project(':airbyte-protocol:models')
implementation project(':airbyte-singer')
Expand Down
2 changes: 2 additions & 0 deletions gradle.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,3 @@
org.gradle.parallel=true
org.gradle.caching=true
org.gradle.vfs.watch=true
13 changes: 13 additions & 0 deletions settings.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,16 @@
// Configure the gradle enterprise plugin to enable build scans. Enabling the plugin at the top of the settings file allows the build scan to record
// as much information as possible.
plugins {
id "com.gradle.enterprise" version "3.4.1"
}

gradleEnterprise {
buildScan {
termsOfServiceUrl = "https://gradle.com/terms-of-service"
termsOfServiceAgree = "yes"
}
}

import groovy.io.FileType

rootProject.name = 'airbyte'
Expand Down