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

Jump to bottom

[SPARK-24573][INFRA] Runs SBT checkstyle after the build to work around a side-effect #21579

Closed
wants to merge 10 commits into from
Closed

[SPARK-24573][INFRA] Runs SBT checkstyle after the build to work around a side-effect #21579

wants to merge 10 commits into from

Conversation

HyukjinKwon
Copy link
Member

@HyukjinKwon HyukjinKwon commented Jun 17, 2018 •
edited
Loading

What changes were proposed in this pull request?

Seems checkstyle affects the build in the PR builder in Jenkins. I can't reproduce in my local and seems it can only be reproduced in the PR builder.

I was checking the places it goes through and this is just a speculation that checkstyle's compilation in SBT has a side effect to the assembly build.

This PR proposes to run the SBT checkstyle after the build.

How was this patch tested?

Jenkins tests.

@SparkQA
Copy link

SparkQA commented Jun 17, 2018

Test build #91979 has started for PR 21579 at commit 8307cbd.

@SparkQA
Copy link

SparkQA commented Jun 17, 2018

Test build #91980 has finished for PR 21579 at commit 96f5458.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jun 17, 2018

Test build #91981 has started for PR 21579 at commit 01a9bc8.

@HyukjinKwon HyukjinKwon changed the title [DO-NOT-MERGE] Investigate SBT Java stylecheck affecting the build [SPARK-24573][INFRA] Delays SBT checkstyle after the build to work around a side-effect Jun 17, 2018
@SparkQA
Copy link

SparkQA commented Jun 17, 2018

Test build #91986 has finished for PR 21579 at commit 7f46304.

  • This patch fails Java style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

HyukjinKwon
HyukjinKwon commented Jun 17, 2018
View reviewed changes
dev/run-tests.py
@@ -366,6 +366,9 @@ def build_spark_assembly_sbt(hadoop_version):
" ".join(profiles_and_goals))
exec_sbt(profiles_and_goals)

if checkstyle:
run_java_style_checks()
Copy link
Member Author

@HyukjinKwon HyukjinKwon Jun 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This takes 2-ish mins now given my past tries.

@HyukjinKwon
Copy link
Member Author

Build / checkstyle passed in https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/91981/console

@HyukjinKwon
Copy link
Member Author

cc @cloud-fan

@HyukjinKwon HyukjinKwon changed the title [SPARK-24573][INFRA] Delays SBT checkstyle after the build to work around a side-effect [SPARK-24573][INFRA] Runs SBT checkstyle after the build to work around a side-effect Jun 17, 2018
@SparkQA
Copy link

SparkQA commented Jun 17, 2018

Test build #91988 has started for PR 21579 at commit 6529622.

@SparkQA
Copy link

SparkQA commented Jun 17, 2018

Test build #91989 has started for PR 21579 at commit 73a494e.

@SparkQA
Copy link

SparkQA commented Jun 17, 2018

Test build #91987 has finished for PR 21579 at commit 14351d6.

  • This patch fails to generate documentation.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HyukjinKwon
Copy link
Member Author

^ I just double checked if it affects Unidoc too which we run after the build.

@viirya
Copy link
Member

viirya commented Jun 17, 2018

Any idea why this issue suddenly happens?

@HyukjinKwon
Copy link
Member Author

HyukjinKwon commented Jun 17, 2018 •
edited
Loading

One module was recently added and I was suspecting that it caused a effect from compilation by checkstyle to the build. I am not fully sure but I just suspected this by reading related places ... seems at least it fixes the issue though .. Will take it back for now actually. I think I am not fully sure why it suddenly happened.

@HyukjinKwon
Copy link
Member Author

HyukjinKwon commented Jun 17, 2018 •
edited
Loading

Other builds above that don't have the results were aborted intentionally. The last two build currently running are:

  1. Current proposal having a Java change to make sure this PR fixes it (7eba44a).
  2. Reverted the Java change by 1 (d902b10).

@SparkQA
Copy link

SparkQA commented Jun 17, 2018

Test build #91997 has finished for PR 21579 at commit d902b10.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HyukjinKwon
Copy link
Member Author

retest this please

@SparkQA
Copy link

SparkQA commented Jun 17, 2018

Test build #91995 has finished for PR 21579 at commit 7eba44a.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jun 17, 2018

Test build #91999 has finished for PR 21579 at commit d902b10.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@viirya
Copy link
Member

viirya commented Jun 18, 2018

ping @cloud-fan Shall we merge this to unblock the testing of other PRs?

@HyukjinKwon
Copy link
Member Author

Let me merge this one. We can revert this at any moment if we see other problems and go another root, for example, clean before assembly.

@HyukjinKwon
Copy link
Member Author

Or, please revert this and propose another change if anyone is clear on this. I don't mind.

Merged to master.

@HyukjinKwon
Copy link
Member Author

Thanks for reviewing this @viirya and @felixcheung.

@asfgit asfgit closed this in b0a9352 Jun 18, 2018
@GulajavaMinistudio GulajavaMinistudio mentioned this pull request Jun 18, 2018
Merged
@cloud-fan
Copy link
Contributor

thanks for the fix!

@HyukjinKwon HyukjinKwon deleted the investigate-javastyle branch October 16, 2018 12:43
@HyukjinKwon HyukjinKwon mentioned this pull request Dec 9, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felixcheung felixcheung felixcheung approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@HyukjinKwon @SparkQA @viirya @cloud-fan @felixcheung