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

Adding a script to check for line endings (valid UNIX ending) #2544

Merged
merged 1 commit into from
Mar 10, 2015

Conversation

kushalsingh007
Copy link
Member

-Created a bash script using grep to check for carriage return at line endings.
-Fix for issue #2281.

@kushalsingh007
Copy link
Member Author

Yet again the build fail is due to Travis being flaky , kindly re-run the build again :)

@miri64
Copy link
Member

miri64 commented Mar 5, 2015

Actually it fails because there are trailing whitespaces in your script. Please remove them ;)

@miri64
Copy link
Member

miri64 commented Mar 5, 2015

(It's flaky when some random board fails. The static tests are quite stable)

Speaking of: could you add your script to them (add a call to your script to dist/tools/travis-scripts/build_and_test.sh). Also, I think it would be best if it only applies to new and changed files. Check out the other check scripts as for example dist/tools/whitespacecheck/check.sh or dist/tools/cppcheck/check.sh for how to do this.

@kushalsingh007
Copy link
Member Author

Ok , I will have a look at it and make sure that the check applies only to new files :)

@OlegHahm OlegHahm self-assigned this Mar 6, 2015
@OlegHahm OlegHahm added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: tests Area: tests and testing framework labels Mar 6, 2015
@kushalsingh007
Copy link
Member Author

@authmillenon : I have added a new script which checks only for the newly added files and added a call to it in build_and_run.sh. However I do not have the permission to modify build_and_run.sh ( and that is why the static case fails ) . So have a look at it .

./dist/tools/endingcheck/check.sh master --diff-filter=AC
RESULT=$(set_result $? $RESULT)

./dist/tools/endingcheck/check.sh riot/master
Copy link
Member

Choose a reason for hiding this comment

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

One call of your script with riot/master (the current master at RIOT-OS/RIOT) and --diff-filter=MA (only look at modified [M] and added [A] files) should suffice (source: http://git-scm.com/docs/git-diff).

@miri64
Copy link
Member

miri64 commented Mar 8, 2015

However I do not have the permission to modify build_and_run.sh ( and that is why the static case fails ).

What do you mean? You obviously modified it (see my comment above). So how do you come to the impression you don't have any permission to do so? 😕

@kushalsingh007
Copy link
Member Author

That's because it is the reason build failed
( see the travis result https://travis-ci.org/RIOT-OS/RIOT/jobs/53509103: permission denied error )

@miri64
Copy link
Member

miri64 commented Mar 8, 2015

Ah... Your script is not executable. That's why Travis is saying the permissions were denied (because it has no permission to execute it). Git has an understanding of UNIX permission flags so you have to give your script execution permission:

chmod +x ./dist/tools/endingcheck/check.sh

@miri64
Copy link
Member

miri64 commented Mar 8, 2015

(and than of course add this change to git ;)

git add ./dist/tools/endingcheck/check.sh
git commit

@kushalsingh007
Copy link
Member Author

Done , the changes and the build

#

BRANCH=${1}
FILEREGEX='\.([sScHh]|cpp)$'
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 actually include all files, when I think about it, not just source code.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are things like images which should be excluded from it.

@miri64
Copy link
Member

miri64 commented Mar 8, 2015

Please squash only when the review is done, otherwise it can get a little bit confusing with bigger changes ;-)

@kushalsingh007
Copy link
Member Author

Ok , I'll keep that in mind from next time. So , can I squash now to change the file regex ?

@miri64
Copy link
Member

miri64 commented Mar 8, 2015

No, just mark the commits as squashable (I usually use a [SQUASH ME] tag in front of the commit description) and squash it, if you get the ACK ;-)

@miri64
Copy link
Member

miri64 commented Mar 8, 2015

(I usually use a [SQUASH ME] tag in front of the commit description)

See #2546 for example

@@ -45,6 +45,11 @@ then
./dist/tools/externc/check.sh master
RESULT=$(set_result $? $RESULT)

RESULT=$(set_result $? $RESULT)
Copy link
Member

Choose a reason for hiding this comment

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

Why is this duplicate line here?

@OlegHahm OlegHahm added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Mar 9, 2015
-Created a bash script using grep to check for carriage return at line endings.
-Works for modified and new files only (if choosen)
-Modified build_and_run.sh to include the new check.sh and run it.
@kushalsingh007
Copy link
Member Author

@OlegHahm : Done squashing.

@miri64 miri64 removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Mar 10, 2015
miri64 added a commit that referenced this pull request Mar 10, 2015
Adding a script to check for line endings (valid UNIX ending)
@miri64 miri64 merged commit b2beba5 into RIOT-OS:master Mar 10, 2015
@kushalsingh007 kushalsingh007 deleted the unix_line_ending branch March 25, 2015 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: tests Area: tests and testing framework Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants