Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

[Review Required] Adding more file formats to license_header.py (.m, .mk, .R, .cfg) #9472

Closed
wants to merge 2 commits into from

Conversation

mbaijal
Copy link
Contributor

@mbaijal mbaijal commented Jan 18, 2018

Fixing Apache RAT failures - Part 1##

PLEASE HELP REVIEW! @bhavinthaker @gautamkmr
Added some file formats to the license_header.py script and then added the Apache license to the files that were failing the updated check.

For further details, Refer to this wiki -
https://cwiki.apache.org/confluence/display/MXNET/MXNet+Source+Licenses

Checklist

Essentials

  • Passed code style checking (make lint)
  • Changes are complete (i.e. I finished coding on this PR)
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • The main change is adding this to the license_header.py - '.m':'%', '.R':'#', '.mk':'#'
  • Added Apache License Header to all .R, .mk and .m files using the license_header.py script

Please Review the following -

  • correct comment markers are used for the new formats
  • It is ok to add a license to these files (nothing will break)
  • the updated files do fall under the Apache license
  • Please help make sure that NO CODE has been affected in this PR (only blank spaces and comments)

Comments -

Also To be added to the script (in a separate PR after release)

  • .proto
  • Makefile
  • .pyx

@marcoabreu
Copy link
Contributor

Would it be possible not to change the whitespaces inside the codebase?

@mbaijal
Copy link
Contributor Author

mbaijal commented Jan 18, 2018

Changes were made by the script automatically. Let me see if it can be avoided.

@marcoabreu
Copy link
Contributor

That'd be great, otherwise we could run into merge problems if this PR has to be reverted (think about the last case we've had when we were in Palo Alto)

@bhavinthaker
Copy link
Contributor

I think the license_header script is doing the correct thing by removing extraneous white spaces at the end of each line of code -- I would +1 to having them removed.

@marcoabreu
Copy link
Contributor

marcoabreu commented Jan 18, 2018 via email

@mbaijal mbaijal changed the title [WIP][DoNotMerge] Adding more file formats to license_header.py (.m, .mk, .R) [Review Required] Adding more file formats to license_header.py (.m, .mk, .R) Jan 19, 2018
@mbaijal mbaijal changed the title [Review Required] Adding more file formats to license_header.py (.m, .mk, .R) [Review Required] Adding more file formats to license_header.py (.m, .mk, .R, .cfg) Jan 19, 2018
@mbaijal
Copy link
Contributor Author

mbaijal commented Jan 19, 2018

@marcoabreu I agree with Bhavin. Since the license_header.py script is performing this blankspace stripping for all files in the src, its best to keep it.

Making a separate PR is not straightforward since git does not provide an easy way of only committing the non whitespace changes.

@KellenSunderland
Copy link
Contributor

@mbaijal The issue that sometimes comes up with large commits is that when you squash two different types of changes together you make it difficult for other devs to merge the commit, and you make it quite hard to revert one of the changes at a later period in time. Given this I'd recommend splitting the whitespace changes into their own commit and then merging without squashing.

@mbaijal
Copy link
Contributor Author

mbaijal commented Jan 19, 2018

Thanks Marco, Kellen, Bhavin for your comments.
I created a new PR for the license updates that does not include whitespace changes.
#9499.
Do take a quick look to help review.

Closing this PR.

@mbaijal mbaijal closed this Jan 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants