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

Add style checks #6571

Merged
merged 2 commits into from
Jan 15, 2021
Merged

Add style checks #6571

merged 2 commits into from
Jan 15, 2021

Conversation

rhuanjl
Copy link
Collaborator

@rhuanjl rhuanjl commented Jan 9, 2021

  1. Add in the style checks from the old CI
  2. Update copyright check so all edited files have to have new copyright (also added the "All Rights Reserved" statement to the copyright in the license)
  3. delete the defunct dotnet CI config script

I considered renaming the jenkins folder as it's nothing to do with jenkins - but there were a lot of things to change based on that and it seemed like too big a diff to be worth it.

For reference these "Style" checks do:

  1. Copyright: check all edited files in the PR contain the correct copyright Banner
  2. Ascii check: check all edited source code is ascii only (this is so anyone compiling on a machine from a different locale doesn't get errors)
  3. EOL check: check all edited files use LF only line endings (no CR) this excludes .baseline files for tests which have to use CRLF due to an awkwardness of how testing works on windows
  4. Tab check: check all edited source files contain no tab characters

For #6547
Fixes: #6548

@rhuanjl rhuanjl force-pushed the moreCI branch 3 times, most recently from 2bb0c8d to 570d795 Compare January 9, 2021 13:47
@rhuanjl rhuanjl requested a review from ppenzin January 9, 2021 14:04
@@ -18,7 +19,7 @@ rm -f $ERRFILE
rm -f $ERRFILETEMP

echo "Check Copyright > Begin Checking..."
git diff --name-only `git merge-base origin/$ghprbTargetBranch HEAD` HEAD |
git diff --name-only `git merge-base origin/master HEAD` HEAD |
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This instruction didn't work - edited to fix (by bringing in line with the logic in the other check scripts

@rhuanjl
Copy link
Collaborator Author

rhuanjl commented Jan 10, 2021

Test failure is a timeout on a wasm test - I did a re-run and it timed out again though on a different test - maybe just a slow time of night or something? (the runtests script counts any test taking more than 1 minut as a timeout and fails it)

@Fly-Style
Copy link
Contributor

Fly-Style commented Jan 11, 2021

@rhuanjl @ppenzin BTW, I have a good experience with clang-format and integration it as GitHub Action for automated code formatting.
Probably, it's a good idea to discuss about migration to clang-format ... someday

@rhuanjl
Copy link
Collaborator Author

rhuanjl commented Jan 11, 2021

@rhuanjl @ppenzin BTW, I have a good experience with clang-format and integration it as GitHub Action for automated code formatting.
Probably, it's a good idea to discuss about migration to clang-format ... someday

Clang format for the whole repository could well be quite a significant change - the historic style checks that this includes are quite minimal and aimed at avoiding specific problems:

  • tabs are banned because different editors make them different sizes
  • line endings are LF only as it's more portable
  • non-ascii code in source is banned because with different locale settings it can fail to compile
  • copyright banners are forced for obvious reasons

A more extensive consistent style across the whole code base could be very nice for welcoming new contributors but how much work would it be? And how beneficially would it be? (That said if you'd like to try it and open a PR I'll take a look)

Copy link
Member

@ppenzin ppenzin left a comment

Choose a reason for hiding this comment

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

Aside from feeling ambivalent about MSFT copyright in new files, this looks good.

@@ -59,7 +60,7 @@ if [ -e $ERRFILE ]; then # if error file exists then there were errors
>&2 echo "--- ERRORS ---"
cat $ERRFILE 1>&2 # send output to stderr so it can be redirected as error if desired
>&2 echo "--------------"
exit 1 # tell the caller there was an error (so Jenkins will fail the CI task)
Copy link
Member

Choose a reason for hiding this comment

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

If you want to keep make it more future-proof, you can remove the name of the CI system we are using. I don't think it is critical, we can keep Azure here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

I think you might still need exit 1, otherwise Azure may not pick the failure up.

@rhuanjl
Copy link
Collaborator Author

rhuanjl commented Jan 12, 2021

@ppenzin I've done the updates and exclude .yml from the copyright check so we don't have to say copyright microsoft in the CI script.

@rhuanjl rhuanjl merged commit a804755 into chakra-core:master Jan 15, 2021
@rhuanjl rhuanjl deleted the moreCI branch March 23, 2021 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update the copyright text
3 participants