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

utils: fix clang-format version check #3186

Merged
merged 1 commit into from
Oct 4, 2023

Conversation

nilason
Copy link
Contributor

@nilason nilason commented Sep 24, 2023

  • Adopt to the fact the result of clang-format --version may look slightly different on various platforms.
  • Make sure 'clang-format' version 15 is used (v 16+ results in unwanted changes)

Closes #3183

- Adopt to the fact the result of `clang-format --version` may look slightly different
  on various platforms.
- Make sure 'clang-format' version 15 is used (v 16+ results in unwanted changes)

Closes OSGeo#3183
@nilason nilason added the bug Something isn't working label Sep 24, 2023
@nilason nilason requested a review from marisn September 24, 2023 07:59
Copy link
Contributor

@marisn marisn left a comment

Choose a reason for hiding this comment

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

Version extraction works. Can not comment on requirement of exact version match.

@nilason
Copy link
Contributor Author

nilason commented Sep 25, 2023

Version extraction works. Can not comment on requirement of exact version match.

Thanks for testing!

I'm afraid we have to stick to clang-format version 15 for the time being. I tested with version 16 and following files were changed:

lib/gis/lz4.c
lib/gis/lz4.h
lib/gmath/solvers_direct.c
lib/gpde/test/test_arrays.c
lib/rst/interp_float/segmen2d_parallel.c
raster/r.mfilter/execute.c
raster/r.proj/main.c
raster/r.sim/simlib/hydro.c
raster/r.slope.aspect/main.c
raster/r.sun/main.c

Just for testing, I ran clang-format again on the above changed files, now with version 15. The following files changed again:

lib/gis/lz4.h
lib/rst/interp_float/segmen2d_parallel.c
raster/r.mfilter/execute.c
raster/r.proj/main.c
raster/r.sim/simlib/hydro.c
raster/r.slope.aspect/main.c
raster/r.sun/main.c

So, mixing versions will not work, the same as with Black.

@wenzeslaus
Copy link
Member

So, mixing versions will not work, the same as with Black.

Any idea how and when to update the version? For Black, the major (breaking) releases are yearly and having the latest seems to me like the only reasonable policy because everything else seems arbitrary. At the same time, using pip to install Black was necessary at least up until recently, so having the latest version was not an issue.

@nilason
Copy link
Contributor Author

nilason commented Sep 25, 2023

So, mixing versions will not work, the same as with Black.

Any idea how and when to update the version? For Black, the major (breaking) releases are yearly and having the latest seems to me like the only reasonable policy because everything else seems arbitrary. At the same time, using pip to install Black was necessary at least up until recently, so having the latest version was not an issue.

I don't think there will be reason to change this in at least a year. Most platforms provide packages with versioned clang-format and as I understand it version 15 is broadly supported. For reference: GDAL just updated to use clang-format 15.

The changes after running version 16 was mainly (if not exclusively) re-formatting of pragma statements. Nothing urgent.

@nilason nilason merged commit b37c518 into OSGeo:main Oct 4, 2023
@neteler neteler added this to the 8.4.0 milestone Oct 5, 2023
landam pushed a commit to landam/grass that referenced this pull request Oct 25, 2023
- Adopt to the fact the result of `clang-format --version` may look slightly different
  on various platforms.
- Make sure 'clang-format' version 15 is used (v 16+ results in unwanted changes)

Closes OSGeo#3183
neteler pushed a commit to nilason/grass that referenced this pull request Nov 7, 2023
- Adopt to the fact the result of `clang-format --version` may look slightly different
  on various platforms.
- Make sure 'clang-format' version 15 is used (v 16+ results in unwanted changes)

Closes OSGeo#3183
HuidaeCho pushed a commit to HuidaeCho/grass that referenced this pull request Jan 9, 2024
- Adopt to the fact the result of `clang-format --version` may look slightly different
  on various platforms.
- Make sure 'clang-format' version 15 is used (v 16+ results in unwanted changes)

Closes OSGeo#3183
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] utils/grass_clang_format.sh fails to detect version
4 participants