Skip to content

[SPARK-30534][INFRA] Use mvn in dev/scalastyle #27242

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

Closed
wants to merge 4 commits into from
Closed

[SPARK-30534][INFRA] Use mvn in dev/scalastyle #27242

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
eeb8bfa
[SPARK-30534][INFRA] Use mvn in `dev/scalastyle`
dongjoon-hyun Jan 16, 2020
1e468ea
Move test source into source
dongjoon-hyun Jan 16, 2020
08e4400
use flag
dongjoon-hyun Jan 16, 2020
9c8e204
clean up
dongjoon-hyun Jan 16, 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
14 changes: 3 additions & 11 deletions dev/scalastyle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,10 @@
# limitations under the License.
#

SPARK_PROFILES=${1:-"-Pmesos -Pkubernetes -Pyarn -Pspark-ganglia-lgpl -Pkinesis-asl -Phive-thriftserver -Phive"}
SCRIPT_DIR="$( cd "$( dirname "$0" )" && pwd )"
Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Jan 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This came from lint-java.


# NOTE: echo "q" is needed because SBT prompts the user for input on encountering a build file
# with failure (either resolution or compilation); the "q" makes SBT quit.
ERRORS=$(echo -e "q\n" \
| build/sbt \
${SPARK_PROFILES} \
-Pdocker-integration-tests \
-Pkubernetes-integration-tests \
scalastyle test:scalastyle \
| awk '{if($1~/error/)print}' \
)
SPARK_PROFILES=${1:-"-Pmesos -Pkubernetes -Pyarn -Pspark-ganglia-lgpl -Pkinesis-asl -Phive-thriftserver -Phive -Pdocker-integration-tests -Pkubernetes-integration-tests"}
ERRORS=$($SCRIPT_DIR/../build/mvn $SPARK_PROFILES scalastyle:check | grep "^error file")
Copy link
Member

Choose a reason for hiding this comment

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

So, I guess this is a temp fix and we should fix SBT build to use HTTPs correctly .. am I correct?

If this was a non-temp fix, we should rather have another script like lint-java vs sbt-checkstyle. Ideally we should use SBT in PR builder to save times.

Copy link
Member Author

Choose a reason for hiding this comment

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

? This is a permanent fix for GitHub Action and this is not for PR Builder.
For SBT, if you want, you can make a PR~

Copy link
Member

@HyukjinKwon HyukjinKwon Jan 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The problem is it affects PR builder:

spark/dev/run-tests.py

Lines 182 to 187 in 32af700

def run_scala_style_checks(extra_profiles):
build_profiles = extra_profiles + modules.root.build_profile_flags
set_title_and_block("Running Scala style checks", "BLOCK_SCALA_STYLE")
profiles = " ".join(build_profiles)
print("[info] Checking Scala style using SBT with these profiles: ", profiles)
run_cmd([os.path.join(SPARK_HOME, "dev", "lint-scala"), profiles])

and switching to Maven wasn't encouraged in PR builder; so I had to add such script to use SBT sbt-checkstyle.

Copy link
Member

Choose a reason for hiding this comment

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

I would like to make a fix for SBT but wasn't able to reproduce. I removed all .m2 and .ivy2 but it still works and uses [info] downloading https://repo1.maven.org/maven2.

If I am not wrong SBT uses HTTPs:

spark/project/SparkBuild.scala

Line 227 in a3357df

DefaultMavenRepository,

https://www.scala-sbt.org/0.13/docs/Resolvers.html

So, I was wondering how to reproduce.

Copy link
Member

@HyukjinKwon HyukjinKwon Jan 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Why don't we simply copy this new file as mvn-scalastyle and old file as scalastyle as are?

And, Github Actions calls mvn-scalastyle and Jenkins PR builder calls scalastyle.

FYI, currently Jenkins PR builder directly calls sbt-checkstyle

spark/dev/run-tests.py

Lines 190 to 196 in 32af700

def run_java_style_checks(build_profiles):
set_title_and_block("Running Java style checks", "BLOCK_JAVA_STYLE")
# The same profiles used for building are used to run Checkstyle by SBT as well because
# the previous build looks reused for Checkstyle and affecting Checkstyle. See SPARK-27130.
profiles = " ".join(build_profiles)
print("[info] Checking Java style using SBT with these profiles: ", profiles)
run_cmd([os.path.join(SPARK_HOME, "dev", "sbt-checkstyle"), profiles])
for Java style instead of lint-java for similar reason.

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Jan 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'm +1 for any suggestion if you make a PR and pass all system (including GitHub Action). :)

BTW, did you try to checkout one of older commits than this, @HyukjinKwon ?

If it works in your environment even in that case, it sounds weird to me because of the following.

As you can see on the mailing list, this was reported by @tgravescs and I confirmed the situation. Then, I made this PR at that time. Also, GitHub Action log still shows the whole situation (the failures before this commit and the successes after this commit).

Screen Shot 2020-01-20 at 11 48 50 PM

For now, I don't have any clue about the difference (code side? or Maven server side? or your environment?).

In any way, you can move forward whiling keeping our system green. We had better discuss on your PR.

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Jan 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I checked this with dca8380 again. It still fails before this commit. Please try to checkout old commit in your environment, @HyukjinKwon .

[error] [FATAL] Non-resolvable parent POM: 
Could not transfer artifact org.apache:apache:pom:18 from/to central (
http://repo.maven.apache.org/maven2): Failed to look for file:
http://repo.maven.apache.org/maven2/org/apache/apache/18/apache-18.pom. Return code is: 501 and 'parent.relativePath' points at wrong local POM @ line 22, column 11

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for pointer, @dongjoon-hyun!


if test ! -z "$ERRORS"; then
echo -e "Scalastyle checks failed at following occurrences:\n$ERRORS"
Expand Down
2 changes: 1 addition & 1 deletion pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2760,7 +2760,7 @@
<configuration>
<verbose>false</verbose>
<failOnViolation>true</failOnViolation>
<includeTestSourceDirectory>false</includeTestSourceDirectory>
<includeTestSourceDirectory>true</includeTestSourceDirectory>
<failOnWarning>false</failOnWarning>
<sourceDirectory>${basedir}/src/main/scala</sourceDirectory>
<testSourceDirectory>${basedir}/src/test/scala</testSourceDirectory>
Expand Down