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

Strip trailing spaces and convert tabs to spaces #157

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jcfr
Copy link
Contributor

@jcfr jcfr commented Apr 1, 2024

No description provided.

@jcfr
Copy link
Contributor Author

jcfr commented Apr 1, 2024

Changes originally contributed in #149

@jcfr
Copy link
Contributor Author

jcfr commented Apr 1, 2024

cc: @tashrifbillah @pieper

@tashrifbillah
Copy link
Collaborator

Jean, thank you for detecting this style issues. However, is it your cmake convention to use two spaces for tab? Asking because we always use four spaces across Python, MATLAB, Bash, JavaScript.

@jcfr
Copy link
Contributor Author

jcfr commented Apr 1, 2024

The use of 2-space has been the convention applied in CMake itself1 and all of the CMake-based build-system I have been working with. Moving forward with this change will help developer familiar with maintaining such system.

Once this is integrated, I would also suggest moving forward with adding .pre-commit-config.yaml like we have in Slicer, this will help minimize style discrepancies and enforce consistency throughout the code base.

Footnotes

  1. https://github.com/Kitware/CMake/blob/d166e7d740bfb53c18221f1126e984b668923208/.editorconfig#L8-L10

Use 1-space and align consecutive comments
@jcfr
Copy link
Contributor Author

jcfr commented Apr 1, 2024

Once integrated, these style commits will be added to a file called .git-blame-ignore-revs.

This file lists revisions that should be ignored when considering attribution for the actual code written. Code style changes should not be considered as modifications with regards to attribution.

To see clean and meaningful blame information.

$ git blame important.py --ignore-revs-file .git-blame-ignore-revs

To configure git to automatically ignore revisions listed in a file on every call to git blame.

$ git config blame.ignoreRevsFile .git-blame-ignore-revs

See https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view

For an example of with/without this file in the context of the GitHub UI, see Slicer/Slicer#6306 (comment)

if(gccToolset STREQUAL "cc")
set(gccToolset "gcc")
endif()
list(APPEND Boost_b2_Command toolset=${gccToolset})
endif()
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jcfr , I do not think this PR is still right. Lines 93-101 should be inside the 90's if block.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Jean, would you like me to hand-correct it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good morning @jcfr :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ping @jcfr ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ping @jcfr ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ping @jcfr ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ping @jcfr ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ping @jcfr ...

@tashrifbillah
Copy link
Collaborator

tashrifbillah commented Apr 10, 2024

Hi @pieper , I installed the latest cmake-3.29.0-rc4 from Kitware website but the bin directory does not contain ccmake:

$ ls cmake-3.29.0-rc4/build/bin/
cmake  cpack  ctest  ctresalloc

Has there been a change in how they are packaging it now? In other words, how can I get ccmake?

@pieper
Copy link
Contributor

pieper commented Apr 10, 2024

Maybe it was an issue with 3.29.0-rc4. 3.29.1 seems to have it:

image

@tashrifbillah
Copy link
Collaborator

tashrifbillah commented Apr 11, 2024

Hi @pieper , thank you but how did you get that packages?

I downloaded source from https://cmake.org/download/ , and did:

tar -xzf cmake-3.29.1.tar.gz
cd cmake-3.29.1 && mkdir build && cd build
../configure --prefix=`pwd`
gmake -j 4

This is what I always did but the build/bin of 3.29.1 still does not contain ccmake. I tried Googling but couldn't really find a complete building instruction. If you know, can you point me to?

@tashrifbillah
Copy link
Collaborator

Hi Steve, I found a README.rst within cmake-3.29.1 directory. I am following that now. Thanks.

@pieper
Copy link
Contributor

pieper commented Apr 11, 2024

Also fwi @tashrifbillah I downloaded the binaries and ccmake was included. You may need to enable ccmake before building. It could be a configure option.

@tashrifbillah
Copy link
Collaborator

Hi @pieper and @jcfr , could you tell me what latest GCC version ITK supports? I tried with both GCC 11.4.1 (RHEL 9 default) and GCC 9.5.0. Both are reported as failure by ITK:

"dunno about this GCC"

@pieper
Copy link
Contributor

pieper commented Apr 12, 2024

latest GCC version ITK supports

I don't know off the top of my head. It would be a good question for the ITK forum.

@tashrifbillah tashrifbillah mentioned this pull request Apr 12, 2024
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.

3 participants