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.

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

[HOT-FIX][BUILD] Use the new location of checkstyle-suppressions.xml #11567

Closed
wants to merge 3 commits into from
Closed

[HOT-FIX][BUILD] Use the new location of checkstyle-suppressions.xml #11567

wants to merge 3 commits into from

Conversation

dongjoon-hyun
Copy link
Member

What changes were proposed in this pull request?

This PR fixes dev/lint-java and mvn checkstyle:check failures due the recent file location change.
The following is the error message of current master.

Checkstyle checks failed at following occurrences:
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:2.17:check (default-cli) on project spark-parent_2.11: Failed during checkstyle configuration: cannot initialize module SuppressionFilter - Cannot set property 'file' to 'checkstyle-suppressions.xml' in module SuppressionFilter: InvocationTargetException: Unable to find: checkstyle-suppressions.xml -> [Help 1]

How was this patch tested?

Manual. The following command should run correctly.

./dev/lint-java
mvn checkstyle:check

@dongjoon-hyun dongjoon-hyun changed the title [HOT-FIX] Use the new location of checkstyle-suppressions.xml [HOT-FIX][BUILD] Use the new location of checkstyle-suppressions.xml Mar 8, 2016
@dongjoon-hyun
Copy link
Member Author

Currently, Jenkins does not run lint-java.
So this failure is not caught before.

@dongjoon-hyun
Copy link
Member Author

Could you review this please?
@srowen and @rxin .

@SparkQA
Copy link

SparkQA commented Mar 8, 2016

Test build #52614 has finished for PR 11567 at commit 380ceb3.

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

@dongjoon-hyun
Copy link
Member Author

PySpark failure is irrelevant for this PR, but I rebased this PR to the master because this is still a problem.

@SparkQA
Copy link

SparkQA commented Mar 8, 2016

Test build #52627 has finished for PR 11567 at commit 4a58fba.

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

@dongjoon-hyun
Copy link
Member Author

Hi, @srowen and @rxin .

In the dev mailing list, there is another report about the location of scalastyle-config.xml.
In pom.xml,

<configLocation>dev/scalastyle-config.xml</configLocation>

Should I include that FIX here together?

It means moving scalastyle-config.xml into dev.
I guess it is the correct location @srowen designed.
Otherwise, please let me know. I'll fix pom.xml instead.

@rxin
Copy link
Contributor

rxin commented Mar 8, 2016

Yes please do that too. Thanks.

On Tuesday, March 8, 2016, Dongjoon Hyun notifications@github.com wrote:

Hi, @srowen https://github.com/srowen and @rxin
https://github.com/rxin .

In the dev mailing list, there is another report about the location of
scalastyle-config.xml.
In pom.xml,

dev/scalastyle-config.xml

Should I include that FIX here together?

It means moving scalastyle-config.xml into dev.
I guess it is the correct location @srowen https://github.com/srowen
designed.
Otherwise, please let me know. I'll fix pom.xml instead.


Reply to this email directly or view it on GitHub
#11567 (comment).

@dongjoon-hyun
Copy link
Member Author

Thank you! I see.

@SparkQA
Copy link

SparkQA commented Mar 8, 2016

Test build #52647 has finished for PR 11567 at commit 775fbdd.

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

@dongjoon-hyun
Copy link
Member Author

Thank you, @srowen . The pom.xml file is updated as you said.

@srowen
Copy link
Member

srowen commented Mar 8, 2016

Ah, OK I see now:

  • scalastyle config's location was referenced incorrectly. This was a change I should have backed out. I'm not sure why it passed the pull request builder tests then
  • I was also not sure why breaking the style rule did in fact got flagged as if it were working: [SPARK-13596] [BUILD] Move misc top-level build files into appropriate subdirs #11522 . I think it's because it found the config in its default location anyway?
  • checkstyle config was never properly updated, and this wasn't caught since it doesn't actually run in the pull request builder (?)

This looks correct then, even given the ?s above. If it passes for several people, great. I'll merge once these tests pass.

@dongjoon-hyun
Copy link
Member Author

About ?s part, @zsxwing gave us some note and pointer in #11438 .

FYI because mvn checkstyle:check depends on mvn install which cost huge time for sbt, java style checker is disabled now: https://github.com/apache/spark/blob/master/dev/run-tests.py#L546

@SparkQA
Copy link

SparkQA commented Mar 8, 2016

Test build #52648 has finished for PR 11567 at commit fbac4a1.

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

@srowen
Copy link
Member

srowen commented Mar 8, 2016

Merged to master. Thanks all, and sorry about that. I am not sure why the PR builder passed.

@asfgit asfgit closed this in 7771c73 Mar 8, 2016
@dongjoon-hyun
Copy link
Member Author

Thank you for merging.

roygao94 pushed a commit to roygao94/spark that referenced this pull request Mar 22, 2016
## What changes were proposed in this pull request?

This PR fixes `dev/lint-java` and `mvn checkstyle:check` failures due the recent file location change.
The following is the error message of current master.
```
Checkstyle checks failed at following occurrences:
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:2.17:check (default-cli) on project spark-parent_2.11: Failed during checkstyle configuration: cannot initialize module SuppressionFilter - Cannot set property 'file' to 'checkstyle-suppressions.xml' in module SuppressionFilter: InvocationTargetException: Unable to find: checkstyle-suppressions.xml -> [Help 1]
```

## How was this patch tested?

Manual. The following command should run correctly.
```
./dev/lint-java
mvn checkstyle:check
```

Author: Dongjoon Hyun <dongjoon@apache.org>

Closes apache#11567 from dongjoon-hyun/hotfix_checkstyle_suppression.
@dongjoon-hyun dongjoon-hyun deleted the hotfix_checkstyle_suppression branch March 29, 2016 05:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants