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

ENH: Miscellaneous fixes in introduction and installation #139

Conversation

jhlegarreta
Copy link
Member

@jhlegarreta jhlegarreta commented May 12, 2020

Miscellaneus fixes in the Introduction and Installation chapters:

Take advantage of the commit for other minor style changes (e.g. period at
the end of sentences, proper LaTex quote syntax, etc.).

Take advantage of the commit to limit the line length to 79 characters in
the ITK's folder structure bullet points.

@jhlegarreta
Copy link
Member Author

The choice of 2016 as the first year for the table was somewhat arbitrary (first year for the deprecation cycle of GCC 4.2).

@jhlegarreta jhlegarreta force-pushed the MiscellaneousFixesInIntroductionAndInstallation branch from 80430f7 to 4b0014f Compare May 12, 2020 23:49
@jhlegarreta jhlegarreta force-pushed the MiscellaneousFixesInIntroductionAndInstallation branch from 4b0014f to 94e1127 Compare May 13, 2020 00:13
@jhlegarreta
Copy link
Member Author

Circle CI failures seem unrelated to the patch set:

FAILED: Modules/IO/NRRD/test/CMakeFiles/ITKIONRRDTestDriver.dir/itkNrrdImageIOTest.cxx.o 
/usr/bin/ccache /usr/bin/c++   
-IModules/ThirdParty/Eigen3/src 
-I/ITKSoftwareGuide-build/ITK/Modules/ThirdParty/Eigen3/src 
-IModules/ThirdParty/KWIML/src 
-I/ITKSoftwareGuide-build/ITK/Modules/ThirdParty/KWIML/src 
-IModules/ThirdParty/KWSys/src 
-I/ITKSoftwareGuide-build/ITK/Modules/ThirdParty/VNL/src/vxl/v3p/netlib 
-I/ITKSoftwareGuide-build/ITK/Modules/ThirdParty/VNL/src/vxl/vcl 
-I/ITKSoftwareGuide-build/ITK/Modules/ThirdParty/VNL/src/vxl/core 
-IModules/ThirdParty/VNL/src/vxl/v3p/netlib 
-IModules/ThirdParty/VNL/src/vxl/vcl 
-IModules/ThirdParty/VNL/src/vxl/core 
-IModules/Core/Common 
-I/ITKSoftwareGuide-build/ITK/Modules/Core/Common/include 
-IModules/IO/ImageBase 
-I/ITKSoftwareGuide-build/ITK/Modules/IO/ImageBase/include 
-I/ITKSoftwareGuide-build/ITK/Modules/IO/NRRD/include 
-I/ITKSoftwareGuide-build/ITK/Modules/ThirdParty/DoubleConversion/src 
-IModules/ThirdParty/DoubleConversion/src/double-conversion 
-I/ITKSoftwareGuide-build/ITK/Modules/Core/TestKernel/include 
-I/ITKSoftwareGuide-build/ITK/Modules/IO/NRRD 
-isystem /ITKSoftwareGuide-build/ITK/Modules/ThirdParty/Eigen3/src/itkeigen/.. 
-isystem Modules/ThirdParty/Eigen3/src/itkeigen/.. 
-I/ITKSoftwareGuide-build/ITK/Modules/ThirdParty/VNL/src/vxl/core/vnl/algo 
-I/ITKSoftwareGuide-build/ITK/Modules/ThirdParty/VNL/src/vxl/core/vnl 
-Wall -Wcast-align -Wdisabled-optimization -Wextra -Wformat=2 -Winvalid-pch 
-Wno-format-nonliteral -Wpointer-arith -Wshadow -Wunused -Wwrite-strings -funit-at-a-time 
-Wno-strict-overflow 
-Wno-deprecated -Wno-invalid-offsetof -Woverloaded-virtual -Wstrict-null-sentinel 
-fPIC -mtune=native -march=corei7  
-Os -DNDEBUG -fPIE   -std=c++11 -MD 
-MT Modules/IO/NRRD/test/CMakeFiles/ITKIONRRDTestDriver.dir/itkNrrdImageIOTest.cxx.o 
-MF Modules/IO/NRRD/test/CMakeFiles/ITKIONRRDTestDriver.dir/itkNrrdImageIOTest.cxx.o.d 
-o Modules/IO/NRRD/test/CMakeFiles/ITKIONRRDTestDriver.dir/itkNrrdImageIOTest.cxx.o 
-c /ITKSoftwareGuide-build/ITK/Modules/IO/NRRD/test/itkNrrdImageIOTest.cxx
c++: internal compiler error: Killed (program cc1plus)

However, before ITK 5.1 is released, maybe these should be fixed:
https://open.cdash.org/viewBuildError.php?type=1&buildid=6520385

@dzenanz
Copy link
Member

dzenanz commented May 13, 2020

We might only need to reduce the column limit here:
https://github.com/InsightSoftwareConsortium/ITK/blob/v5.1.0/Examples/.clang-format#L75
Did you mean before 5.1.0 is announced? Because it has been tagged.

dzenanz
dzenanz previously approved these changes May 13, 2020
Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

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

Compiler table comments in-line. The rest looks good.

SoftwareGuide/Latex/Introduction/Installation.tex Outdated Show resolved Hide resolved
@dzenanz dzenanz requested a review from blowekamp May 13, 2020 13:58
@jhlegarreta
Copy link
Member Author

We might only need to reduce the column limit here:
https://github.com/InsightSoftwareConsortium/ITK/blob/v5.1.0/Examples/.clang-format#L75

Thanks for having a look @dzenanz. I do not remember if that is the flag that applies to documentation pieces that are stripped from the code to populate the SW Guide or whether it applies to the code itself that is stripped for the SW Guide.

According to the link in this comment, LaTeX complains when the line length exceeds 80 characters. Other PRs in the history have also been motivated by the same limit.

Did you mean before 5.1.0 is announced? Because it has been tagged.

In that case I guess it is not that important then. The warnings can be fixed at any time.

@dzenanz
Copy link
Member

dzenanz commented May 13, 2020

That should apply to the code itself that is stripped for the SW Guide.

@jhlegarreta
Copy link
Member Author

That should apply to the code itself that is stripped for the SW Guide.

I guess the choice was justified and has worked well I think, so I see no reason to change it. Should I be wrong, please go ahead and change it.

@dzenanz
Copy link
Member

dzenanz commented May 13, 2020

@hjmjohnson how did you come up with number 88 in InsightSoftwareConsortium/ITK@e18315f? Was it guesstimate or some other method?

@jhlegarreta But if other things were updated (LaTeX version or other settings), then we might need to reduce clang-format column limit to avoid LaTeX overflows. After fixing things in ITK we will also need to update ITK's hash in ITKSoftwareGuide.

@jhlegarreta
Copy link
Member Author

@jhlegarreta But if other things were updated (LaTeX version or other settings), then we might need to reduce clang-format column limit to avoid LaTeX overflows. After fixing things in ITK we will also need to update ITK's hash in ITKSoftwareGuide.

I have no for/against argument. I do not see that the LaTeX file contents themselves will be affected. As said, if it needs to be done, please go ahead and update the ITK hash in the SW Guide.

@hjmjohnson
Copy link
Member

@dzenanz 88 is the optimal result from analyzing millions of lines of python code by the black python code formatted. It was just a first pass guestimate here.

There is also evidence that more than 100 characters can cause troubles for people with vision problems, so I would have a small preference to keep it less than 100 characters.

@dzenanz
Copy link
Member

dzenanz commented May 13, 2020

@hjmjohnson suggestion was to reduce it from 88 to 80, 78 or maybe 79, because the warnings occur at column 80.

@hjmjohnson
Copy link
Member

hjmjohnson commented May 13, 2020 via email

@dzenanz
Copy link
Member

dzenanz commented May 13, 2020

Yes, this is primarily for the software guide, but it will affect all code in the examples directory.

@jhlegarreta
Copy link
Member Author

However, before ITK 5.1 is released, maybe these should be fixed:
https://open.cdash.org/viewBuildError.php?type=1&buildid=6520385

Being tracked in issue #131.

@thewtex
Copy link
Member

thewtex commented May 14, 2020

Rebasing on master...

@thewtex thewtex force-pushed the MiscellaneousFixesInIntroductionAndInstallation branch from 94e1127 to e3698d3 Compare May 14, 2020 13:09
Miscellaneus fixes in the `Introduction` and `Installation` chapters:
- Fix the development environment names.
- Rework the compiler list: update to currently supported versions.
- Remove the compiler support/deprecation cycle table due to difficulties
  in keeping it up to date.
- Remove the deprecated reference to the Wiki Examples: examples are
  entirely hosted in https://itk.org/ITKExamples/ since a series of PRs
  including
  InsightSoftwareConsortium/ITKSphinxExamples#108 and
  InsightSoftwareConsortium/ITKSphinxExamples#109.
- Update the `vxl` link.
- Fix the `Utilities` and `Wrapping` parent folders.
- Mention the raw binary files and the SHA512 hash files in the `Testing`
  folder paragraph.
- Take advantage of the commit to order alphabetically the ITK's main
  folders.

Take advantage of the commit for other minor style changes (e.g. period at
the end of sentences, proper LaTex quote syntax, etc.).

Take advantage of the commit to limit the line length to 79 characters in
the ITK's folder structure bullet points.
@jhlegarreta jhlegarreta force-pushed the MiscellaneousFixesInIntroductionAndInstallation branch from e3698d3 to a5f9673 Compare May 14, 2020 18:05
Copy link
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

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

image

🥇 thanks @jhlegarreta ! Wonderful to have these updates for 5.1.0.

@thewtex thewtex merged commit 344c069 into InsightSoftwareConsortium:master May 15, 2020
@jhlegarreta jhlegarreta deleted the MiscellaneousFixesInIntroductionAndInstallation branch May 15, 2020 02:38
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.

4 participants