Add mechanisms to reformat and check code style, and reformat whole codebase (#128) #919
Conversation
Use an internal version of astyle (astyle 3.0). Scripts taken from QGIS. astyle.options from uclouvain#128 scripts/prepare-commit.sh can be used locally to automatically reformat edited files. Travis-CI will run scripts/verify-indentation.sh to verify committed files.
|
Example of (voluntary) failure of a Travis job due to format error: https://travis-ci.org/rouault/openjpeg/jobs/230491462 |
|
While adding GPL code to |
|
@detonin If you intend to do a release, I guess you might want to make it before I merge this. |
Actually the astyle source code is a X/MIT licence : https://github.com/rouault/openjpeg/blob/d4e54e9f35d532062533f1d369c159810b01d224/thirdparty/astyle/LICENSE.md
Someone could possibly later rewrite them if they find that the investment is worth, but my shell abilities are too poor and this would be wheel reinvention. Anyway anybody can safely strip the scripts/ directory for a tarball and still successfully build openjpeg if they are so afraid of GPL. |
|
@stweil packaging BSD and GPL files together doesn't trigger the GPL on the BSD project. It is only when the BSD proj is linked to the GPL that the GPL license is triggered. So, there shouldn't be any legal issues with putting a GPL script in the project. |
scripts/astyle.options
Outdated
| --lineend=linux | ||
| --indent=spaces=4 | ||
| --style=kr | ||
| --add-brackets |
There was a problem hiding this comment.
should be --add-braces (--add-brackets is deprecated)
| --pad-header | ||
| --pad-oper | ||
| --unpad-paren | ||
| --suffix=none |
There was a problem hiding this comment.
@rouault Where does theses options come from ? Are they copy-pasted from QGIS ?
@malaterre @mayeut Any comment on the astyle config before we merge the PR ?
There was a problem hiding this comment.
All those options come from #128 (comment)
| @@ -1,25 +1,37 @@ | |||
| language: c | |||
| language: cpp | |||
There was a problem hiding this comment.
Is that required ? Changing the whole project from C to C++ (also in cmake) just for opjstyle seems a bit overkill. No other option ?
There was a problem hiding this comment.
I found hard with Travis to make it pass on all configurations and I'm afraid I didn't find a better solution.
There was a problem hiding this comment.
Are there any implication of changing compiler from gcc to g++ ? Does this prevent to compile on some older platforms ?
@malaterre any advice on this ?
There was a problem hiding this comment.
cmake will require a c++ compiler to be detected, even if it doesn't use it (it is only used if WITH_ASTYLE is ON, which is not the default). There's perhaps a way to restrict c++ compiler detection only when WITH_ASTYLE is ON but I couldn't find it.
The C compiler (gcc) will still be used to compile the .c files.
Anyway we require CMake to be able to build openjpeg and CMake source code itself is C++, so the platform must have a working C++ compiler.
And at some point we might perhaps want to use C++ for openjpeg itself.
There was a problem hiding this comment.
@rouault you are describing the symptoms not the real issue :) The fact that cmake only works on arch with valid c++ compiler is outside the scope of this question. I also fail to understand why WITH-ASTYLE=ON imply a valid c++ compiler in the PATH. I do understand that it should have zero impact on the build itself (a bit more dependencies though), but I am just curious...
There was a problem hiding this comment.
@malaterre Astyle source code is C++ and apparently the fact that a project is c, c++ or both is declared in cmake at the PROJECT() scope, so as soon as you have C++ code you must enable C++. Is it really an issue to require a C++ compiler available ? I don't think so honestly.
There was a problem hiding this comment.
I totally failed to follow the fact that now openjpeg is bootstraping a c++ project (astyle). In that case, yes there isn't much solution. As said above
it should have zero impact on the build itself
, I was just curious.
There was a problem hiding this comment.
ok thanks for your feedback @malaterre .
@rouault I think we can proceed and merge now.
|
@detonin Reformat source code PR merged |
Use an internal version of astyle (astyle 3.0). Scripts taken from QGIS.
astyle.options from #128
scripts/prepare-commit.sh can be used locally to automatically reformat edited files.
Travis-CI will run scripts/verify-indentation.sh to verify committed files.