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

Fail the build if a referenced init script does not exist #4845

Merged
merged 1 commit into from Apr 17, 2018

Conversation

@patrikerdes
Contributor

patrikerdes commented Mar 26, 2018

If any of the init scripts specified on the command line does not exist
or is not a file, the build will fail.

Issue: #4672

Signed-off-by: Patrik Erdes patrik@erdes.se

Context

I thought I could attempt to fix #4672. It follows the existing pattern for checking if files exist, both in the implementation and in the test.

Contributor Checklist

  • Review Contribution Guidelines
  • Make sure that all commits are signed off to indicate that you agree to the terms of Developer Certificate of Origin.
  • Check "Allow edit from maintainers" option in pull request so that additional changes can be pushed by Gradle team
  • Provide integration tests (under <subproject>/src/integTest) to verify changes from a user perspective
  • Provide unit tests (under <subproject>/src/test) to verify logic
  • Update User Guide, DSL Reference, and Javadoc for public-facing changes
  • Ensure that tests pass locally: ./gradlew <changed-subproject>:check

Gradle Core Team Checklist

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation
  • Recognize contributor in release notes
@marcphilipp

Looks good to me!

@wolfs @big-guy Would you like to take a look since you created/commented on #4672?

@marcphilipp marcphilipp removed their assignment Apr 9, 2018

@@ -60,6 +62,16 @@ public Object execute(BuildAction action, BuildRequestContext requestContext, Bu
throw new IllegalArgumentException(String.format("The specified settings file '%s' is not a file.", startParameter.getSettingsFile()));
}
}
if (startParameter.getInitScripts().size() > 0) {

This comment has been minimized.

@lptr

lptr Apr 9, 2018

Member

This if is unnecessary.

This comment has been minimized.

@patrikerdes

patrikerdes Apr 13, 2018

Contributor

Agreed, removed in 2a24991

@lptr lptr self-requested a review Apr 9, 2018

@lptr

lptr requested changes Apr 9, 2018 edited

While failing the build with a missing init script is the right thing to do, this will be a breaking change. We should think about how this affects our users, and in the least we should mention it in release notes. For example, CI builds that were running with an incorrectly specified init script for a long time will now start to fail, which might be a problem for some users.

We should also mention the fact that missing scripts will fail the build in the init scripts chapter of the documentation.

@wolfs

wolfs approved these changes Apr 9, 2018

I added some comments, nothing major.

@@ -60,6 +62,16 @@ public Object execute(BuildAction action, BuildRequestContext requestContext, Bu
throw new IllegalArgumentException(String.format("The specified settings file '%s' is not a file.", startParameter.getSettingsFile()));
}
}
if (startParameter.getInitScripts().size() > 0) {
for (File initScript : startParameter.getInitScripts()) {
if (!initScript.isFile()) {

This comment has been minimized.

@wolfs

wolfs Apr 9, 2018

Member

It may be worth extracting this logic now into a method, given that we use it in three places now (buildFile, settingFile, initScripts).

This comment has been minimized.

@patrikerdes

patrikerdes Apr 13, 2018

Contributor

I agree, fixed in 5ac7723

This comment has been minimized.

@wolfs

wolfs Apr 13, 2018

Member

👍

public void buildFailsWhenSpecifiedInitScriptIsNotAFile() {
TestFile file = testFile("unknown");
ExecutionFailure result = inTestDirectory().usingInitScript(file).runWithFailure();

This comment has been minimized.

@wolfs

wolfs Apr 9, 2018

Member

I guess we should have some tests where we have more than one init script argument on the command line, too. WDYT?

This comment has been minimized.

@patrikerdes

patrikerdes Apr 13, 2018

Contributor

Sure, could be good to have. Does this look like what you had in mind? e283407

This comment has been minimized.

@wolfs

wolfs Apr 13, 2018

Member

👍

@lptr

lptr approved these changes Apr 13, 2018

@lptr lptr self-requested a review Apr 13, 2018

@lptr

The code and tests look good to me, but we still need this:

While failing the build with a missing init script is the right thing to do, this will be a breaking change. We should think about how this affects our users, and in the least we should mention it in release notes. For example, CI builds that were running with an incorrectly specified init script for a long time will now start to fail, which might be a problem for some users.

We should also mention the fact that missing scripts will fail the build in the init scripts chapter of the documentation.

@patrikerdes

This comment has been minimized.

Contributor

patrikerdes commented Apr 13, 2018

@lptr I agree that there is a risk of this breaking builds for people. How big that risk is, and what precautions are needed, I think all of you are better at judging than me.

I gave it a shot to update the release notes and docs: I rebased the branch on the master branch to get the current release notes, and added a note under "Potential breaking changes". I also updated the documentation page for init scripts.

@lptr

This comment has been minimized.

Member

lptr commented Apr 13, 2018

Oops, sorry, I missed that.

@lptr

lptr approved these changes Apr 13, 2018

@lptr

This comment has been minimized.

Member

lptr commented Apr 13, 2018

@patrikerdes can you please sign off on your commits? You can use git rebase --signoff for that: https://git-scm.com/docs/git-rebase#git-rebase---signoff

After that I think we are good to merge.

@lptr lptr added this to the 4.8 RC1 milestone Apr 13, 2018

@lptr lptr self-assigned this Apr 13, 2018

@lptr

This comment has been minimized.

Member

lptr commented Apr 13, 2018

@patrikerdes

This comment has been minimized.

Contributor

patrikerdes commented Apr 13, 2018

No you don't need to be sorry, you did not miss anything. I had not yet done the release notes and docs when you wrote your comment :)

Ok, I just did git rebase --signoff and force pushed.

@patrikerdes

This comment has been minimized.

Contributor

patrikerdes commented Apr 13, 2018

But now it complains about me signing off the commit by Marc Philipp. Not sure how to fix that...

Fail the build if a referenced init script does not exist
If any of the init scripts specified on the command line does not exist
or is not a file, the build will fail.

Issue: #4672

Signed-off-by: Patrik Erdes <patrik@erdes.se>
@patrikerdes

This comment has been minimized.

Contributor

patrikerdes commented Apr 13, 2018

Regarding my previous comment about the DCO check failing: I assumed that the individual commits were not that interesting, so I just squashed it all together into one big commit. So now all checks pass.

@wolfs

wolfs approved these changes Apr 13, 2018

@lptr

This comment has been minimized.

Member

lptr commented Apr 16, 2018

@lptr lptr merged commit 44aa98d into gradle:master Apr 17, 2018

7 checks passed

Branch Build Accept (Trigger) (Check) TeamCity build finished
Details
DCO All commits have a DCO sign-off from the author
Performance Test Coordinator - Linux (Branch Build Accept) TeamCity build finished
Details
Quick Feedback (Trigger) (Check) TeamCity build finished
Details
Quick Feedback - Linux Only (Trigger) (Check) TeamCity build finished
Details
Sanity Check (Quick Feedback - Linux Only) TeamCity build finished
Details
WIP ready for review
Details
@lptr

This comment has been minimized.

Member

lptr commented Apr 17, 2018

Duh, will need to re-merge this because of CI troubles. Will try to do that today.

@lptr

This comment has been minimized.

Member

lptr commented Apr 18, 2018

Retry merge as #5078.

@lptr

This comment has been minimized.

Member

lptr commented Apr 18, 2018

Merged. This will be delivered as part of Gradle 4.8. Thanks @patrikerdes for your contribution!

@lptr

This comment has been minimized.

Member

lptr commented Apr 20, 2018

And of course the first build this broke was ours:

* What went wrong:
The specified initialization script '/home/tcagent1/agent/work/668602365d1521fc/gradle/gradle/init-scripts/build-scan.init.gradle.kts' does not exist.

https://builds.gradle.org/viewLog.html?buildId=12367636&tab=buildResultsDiv&buildTypeId=Gradle_Check_SanityCheck

@patrikerdes patrikerdes deleted the patrikerdes:4672 branch Apr 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment