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

Disable bwc distribution caching in master branch #46686

Merged
merged 5 commits into from
Sep 18, 2019
Merged

Disable bwc distribution caching in master branch #46686

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f997642
Disable bwc distribution caching in master branch
mark-vieira Sep 12, 2019
dd987fe
Make this more robust
mark-vieira Sep 12, 2019
c4877dc
Merge branch 'master' into bwc-cache-optimizations
elasticmachine Sep 13, 2019
e084c61
Remove unused import
mark-vieira Sep 13, 2019
bd00970
Add cache if condition description
mark-vieira Sep 17, 2019
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
2 changes: 2 additions & 0 deletions buildSrc/src/main/java/org/elasticsearch/gradle/info/GlobalBuildInfoPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import java.io.IOException;
import java.io.InputStreamReader;
import java.io.UncheckedIOException;
import java.net.URL;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
Expand Down Expand Up @@ -95,6 +96,7 @@ public void apply(Project project) {
ext.set("gradleJavaVersion", Jvm.current().getJavaVersion());
ext.set("gitRevision", gitRevision(project.getRootProject().getRootDir()));
ext.set("buildDate", ZonedDateTime.now(ZoneOffset.UTC));
ext.set("isCi", System.getenv("JENKINS_URL") != null);
});
}

Expand Down
8 changes: 7 additions & 1 deletion distribution/bwc/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,8 +221,14 @@ bwcVersions.forPreviousUnreleased { BwcVersions.UnreleasedVersionInfo unreleased
Task bwcTask = createRunBwcGradleTask(buildBwcTaskName(projectName)) {
inputs.file("${project.buildDir}/refspec")
outputs.files(projectArtifact)
outputs.cacheIf { true }
outputs.cacheIf {
// Don't bother caching in 'master' since the BWC branches move too quickly to make this cost worthwhile
project.ext.isCi && System.getenv('GIT_BRANCH')?.endsWith("master") == false
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 love the fact that we have a CI specific conditional here, but don't have any alternatives to suggest.
Maybe some generic way to turn of caching for arbitrary tasks and configure that from CI ?

Also this won't work correctly for PRs that target master, guess that's not as big of an issue because those don't write to the cache ?

It would be so great if we could come up with a solution that does not involve special casing master.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also this won't work correctly for PRs that target master, guess that's not as big of an issue because those don't write to the cache ?

Correct, it doesn't really matter. They'll still make cache requests that will miss but that's pretty minimal.

Special logic for "if I'm in CI do this" is really commonplace and I'm honestly surprised we don't have instances of this already. To me, I'd much rather have this kind of thing in the build than in CI config (that is complicated enough) and it also allows us to do this branch-specific (CI config is the same for all branches). To me the more we can push logic out of JJB and into the build we should strive to do that. It's easier to reason about, cuts down on JJB and as mentioned, allows us to do branch-specific logic which we're limited in until we move to JJBB.

My preference was to somehow base this on version information the build had but I don't think that's possible. The 7x branch has no idea there is another newer major version that exists ahead of it.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not so much that we do something for CI specifically that bother me, and I hear you very well on the JJb complexity, it's more how hard to find it is, and how hard it will be to reason about if we keep adding these.
It's fine if we want to configure it in the build but it would be worth to do in a way that produces an overview of what the differences are.
For example including a build script conditionally on cI and having all the configuration there with wrapped in project(":distribution:bwc") {}

Copy link
Contributor Author

@mark-vieira mark-vieira Sep 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, there are pros/cons to that approach as well. As if I'm looking at a particular plugin or build script, I also have to consider if there's any CI-specific config in this separate script. This approach would be predicated on use assuming there's CI-specific config at play w/ regards to any particular issue.

Personally, if I'm tracking down a build config issue I'm naturally going to go to the source of that logic. I think I'd rather have any conditionals regarding CI or otherwise to live there. Again, I've dealt with may projects with CI-specific conditionally logic in this way and it's generally not been a problem or source of confusion.

To help out here specifically I've added added a description to this cacheIf condition (as we really should do in all cases) which will make this specific condition very obvious in build scans.

I'm not sure if I want one more place where I have to look in order to assemble the whole picture of build logic for CI. I already have to consider the build scripts/buildSrc itself, plus the .ci init scripts, plus whatever lives in the Jenkins job definition. Not sure having another location to look for stuff that might affect the behavior of the build will improve the situation.

}
args ":${projectDir.replace('/', ':')}:assemble"
if (project.gradle.startParameter.buildCacheEnabled) {
args "--build-cache"
}
doLast {
if (projectArtifact.exists() == false) {
throw new InvalidUserDataException("Building ${bwcVersion} didn't generate expected file ${projectArtifact}")
Expand Down