[SPARK-3758] Script style checking #2612

sarutak · 2014-10-01T09:18:13Z

There are no way to check style of scripts.

SparkQA · 2014-10-01T09:24:43Z

QA tests have started for PR 2612 at commit 1fe76a1.

This patch merges cleanly.

SparkQA · 2014-10-01T10:31:38Z

QA tests have finished for PR 2612 at commit 1fe76a1.

This patch passes unit tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2014-10-01T10:31:41Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/21102/

pwendell · 2014-10-01T16:39:54Z

Does this actually cause an issue in practice? We run tests on windows for each release. I'm worried since most of us develop on mac/linux that this will end up in a weird state where there are mixed EOL characters.

sarutak · 2014-10-01T19:06:57Z

I've not seen issues related to EOL characters in Spark yet but I saw issues in other projects. One of the cases is in Jenkins. https://issues.jenkins-ci.org/browse/JENKINS-7478

In Spark, we may not have issues like JENKINS-7478 but I'm afraid that we have unexpected problems caused by LF-only EOL. We cannot know what's happen when we use wrong EOL.

Do you mean you want to avoid both LF and CRLF are contained in one file right?
If so, how about introducing check script running at test?

sarutak · 2014-10-02T07:08:10Z

@pwendell Actually, CRLF is already contained in compute-classpath.cmd so I think, we should unify CRLF or LF in *.cmd.

tsudukim · 2014-10-02T09:49:39Z

Generally, using LF as EOL character of *.cmd files may cause some troubles.
For example, when the *.cmd file includes LF and multibyte character, some characters of head of line might be removed internally for some reason. The problem @sarutak mentioned seems to be same as this.
In another case, "goto" may go to wrong place. This problem occured once in ruby-lang project. Their build script for Windows happened to be LF, and they faced "strange behaviour of cmd.exe".
https://bugs.ruby-lang.org/issues/10145

I know *.cmd with LF runs seemingly well in many cases, but sometimes cause inexplicable trouble because LF is defenetely not the proper EOL character of Windows *.cmd files.
So if possible, I think we should use CRLF as EOL to avoid such trouble.

SparkQA · 2014-10-03T21:39:35Z

QA tests have started for PR 2612 at commit 91fb0fd.

This patch merges cleanly.

SparkQA · 2014-10-03T22:46:36Z

QA tests have finished for PR 2612 at commit 91fb0fd.

This patch passes unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case e: Exception => logError("Source class " + classPath + " cannot be instantiated", e)

AmplabJenkins · 2014-10-03T22:46:40Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/21276/Test PASSed.

nchammas · 2014-10-04T15:34:00Z

I'm worried since most of us develop on mac/linux that this will end up in a weird state where there are mixed EOL characters.

I'd worry about this, too. If one of us edits one of these files on a Mac, we might unknowingly recreate the issue this PR is trying to fix.

Does it make sense to add some kind of style check to dev/run-tests that validates that all .cmd files use Windows-style newlines? Perhaps a dev/lint-windows-cmd script that gets called from dev/run-tests?

sarutak · 2014-10-05T03:47:57Z

Thanks @nchammas .
I added a basic script to check all of lines in *.cmd end with CRLF.

SparkQA · 2014-10-05T03:49:29Z

QA tests have started for PR 2612 at commit c7f32d7.

This patch merges cleanly.

AmplabJenkins · 2014-10-05T03:50:31Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/21307/Test FAILed.

SparkQA · 2014-10-05T03:59:31Z

QA tests have started for PR 2612 at commit c2b5de1.

This patch merges cleanly.

AmplabJenkins · 2014-10-05T05:08:54Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/21308/Test PASSed.

SparkQA · 2014-10-05T05:19:28Z

QA tests have started for PR 2612 at commit 7ced891.

This patch merges cleanly.

AmplabJenkins · 2014-10-05T06:28:35Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/21309/Test PASSed.

AmplabJenkins · 2014-10-06T14:27:21Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/21325/Test FAILed.

sarutak · 2014-10-06T14:28:30Z

retest this please.

SparkQA · 2014-10-06T14:34:53Z

QA tests have started for PR 2612 at commit 33376b1.

This patch merges cleanly.

SparkQA · 2014-10-06T15:43:29Z

QA tests have finished for PR 2612 at commit 33376b1.

This patch passes unit tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2014-10-06T15:43:33Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/21329/Test PASSed.

SparkQA · 2014-10-08T18:34:42Z

QA tests have started for PR 2612 at commit d825d6b.

This patch merges cleanly.

SparkQA · 2014-10-08T18:35:45Z

QA tests have finished for PR 2612 at commit d825d6b.

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

AmplabJenkins · 2014-10-08T18:35:46Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/21479/Test FAILed.

sarutak · 2014-10-08T18:43:31Z

I noticed .gitattributes should be merged before this PR otherwise the rules defined in .gitattributes doesn't applied.

sarutak · 2014-10-08T18:46:37Z

@pwendell Because of the reason I mentioned above, I intend to split this PR (for .gitattributes and for style-checking).
How do you think?

sarutak · 2014-10-09T02:39:45Z

@pwendell I've splitted the issue into this PR and #2726 .
#2726 is about EOL enforcement.
This PR is for script style checking.

SparkQA · 2014-10-09T03:09:40Z

QA tests have started for PR 2612 at commit 5f5fbdf.

This patch merges cleanly.

SparkQA · 2014-10-09T03:10:45Z

QA tests have finished for PR 2612 at commit 5f5fbdf.

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

AmplabJenkins · 2014-10-09T03:10:46Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/21513/Test FAILed.

SparkQA · 2014-10-09T04:04:38Z

QA tests have started for PR 2612 at commit 41da277.

This patch merges cleanly.

SparkQA · 2014-10-09T05:08:14Z

QA tests have finished for PR 2612 at commit 41da277.

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

AmplabJenkins · 2014-10-09T05:08:18Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/21517/Test PASSed.

SparkQA · 2014-10-11T03:29:47Z

QA tests have started for PR 2612 at commit 894daf8.

This patch merges cleanly.

SparkQA · 2014-10-11T03:31:34Z

QA tests have finished for PR 2612 at commit 894daf8.

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

AmplabJenkins · 2014-10-11T03:31:35Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/21620/Test FAILed.

SparkQA · 2014-10-11T03:45:00Z

QA tests have started for PR 2612 at commit 96a5a52.

This patch merges cleanly.

SparkQA · 2014-10-11T04:47:43Z

QA tests have finished for PR 2612 at commit 96a5a52.

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

AmplabJenkins · 2014-10-11T04:47:47Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/21622/Test PASSed.

pwendell · 2015-01-19T09:38:12Z

Given that no one seems super interested in this refactoring, I'll close this issue as a wontfix. If someone wants to come and revive it later, please feel free.

sarutak · 2015-01-19T19:53:02Z

O.K, I'll close this PR for now.

Modified EOL character from LF to CRLF in *.cmd

1fe76a1

Fixed conflicts

91fb0fd

Added dev/lint-windows-cmd

c7f32d7

sarutak force-pushed the SPARK-3758 branch from fb91625 to c7f32d7 Compare October 5, 2014 03:46

Fixed mode of dev/lint-windows.cmd

c2b5de1

Changed lint-windows-cmd to print pass/error

7ced891

Merge branch 'master' of git://git.apache.org/spark into SPARK-3758

33376b1

Modified error handling of lint-scripts

d825d6b

sarutak changed the title ~~[SPARK-3758] [Windows] Wrong EOL character in *.cmd~~ [SPARK-3758] Script style checking Oct 9, 2014

sarutak added 2 commits October 9, 2014 11:59

Merge branch 'master' of git://git.apache.org/spark into SPARK-3758

30314e3

Reverted changes for .gitattributes

5f5fbdf

Fixed EOL of *.cmd

41da277

sarutak added 2 commits October 11, 2014 12:24

Merge branch 'SPARK-3758' of github.com:sarutak/spark into SPARK-3758

2a90914

Merge branch 'master' of git://git.apache.org/spark into SPARK-3758

894daf8

Normalize EOL

96a5a52

sarutak closed this Jan 19, 2015

sarutak deleted the SPARK-3758 branch April 11, 2015 05:21

[SPARK-3758] Script style checking #2612

[SPARK-3758] Script style checking #2612

Uh oh!

Conversation

sarutak commented Oct 1, 2014

Uh oh!

SparkQA commented Oct 1, 2014

Uh oh!

SparkQA commented Oct 1, 2014

Uh oh!

AmplabJenkins commented Oct 1, 2014

Uh oh!

pwendell commented Oct 1, 2014

Uh oh!

sarutak commented Oct 1, 2014

Uh oh!

sarutak commented Oct 2, 2014

Uh oh!

tsudukim commented Oct 2, 2014

Uh oh!

SparkQA commented Oct 3, 2014

Uh oh!

SparkQA commented Oct 3, 2014

Uh oh!

AmplabJenkins commented Oct 3, 2014

Uh oh!

nchammas commented Oct 4, 2014

Uh oh!

sarutak commented Oct 5, 2014

Uh oh!

SparkQA commented Oct 5, 2014

Uh oh!

AmplabJenkins commented Oct 5, 2014

Uh oh!

SparkQA commented Oct 5, 2014

Uh oh!

AmplabJenkins commented Oct 5, 2014

Uh oh!

SparkQA commented Oct 5, 2014

Uh oh!

AmplabJenkins commented Oct 5, 2014

Uh oh!

AmplabJenkins commented Oct 6, 2014

Uh oh!

sarutak commented Oct 6, 2014

Uh oh!

SparkQA commented Oct 6, 2014

Uh oh!

SparkQA commented Oct 6, 2014

Uh oh!

AmplabJenkins commented Oct 6, 2014

Uh oh!

SparkQA commented Oct 8, 2014

Uh oh!

SparkQA commented Oct 8, 2014

Uh oh!

AmplabJenkins commented Oct 8, 2014

Uh oh!

sarutak commented Oct 8, 2014

Uh oh!

sarutak commented Oct 8, 2014

Uh oh!

sarutak commented Oct 9, 2014

Uh oh!

SparkQA commented Oct 9, 2014

Uh oh!

SparkQA commented Oct 9, 2014

Uh oh!

AmplabJenkins commented Oct 9, 2014

Uh oh!

SparkQA commented Oct 9, 2014

Uh oh!

SparkQA commented Oct 9, 2014

Uh oh!

AmplabJenkins commented Oct 9, 2014

Uh oh!

SparkQA commented Oct 11, 2014

Uh oh!

SparkQA commented Oct 11, 2014

Uh oh!