Re-organize and optimize GitHub Actions workflow

[jcfr]

Simplify GitHub Actions workflow

The changes below have been implemented to improve consistency, facilitate reuse, and streamline future maintenance (e.g generalization to other operating system)

• Remove superfluous env. variable
• Sort -D options alphabetically
• Rely on "actions/checkout" to efficiently download Slicer sources
• Set based of $QT_ROOT_DIR env. variable. This remove the hard-coded arch
• Simplify handling of source and build directories using -B and -S CMake options
• Specify type of CMake variable (e.g BOOL and PATH)
• Keep NumPy enabled as it is used by SlicerDMRI modules.
• Avoid explicit use build tool by leveraging "cmake --build --config "
• Since ninja automatically enables parallelism based on the number of available core, remove the use of $(nproc)

Along with the following optimizations

• cancel in-progress GitHub Actions workflow on repeated pushes
• do not trigger build workflow if .md files are modified

[jhlegarreta]

Thanks for doing this @jcfr. A few notes:

• I would use the ENH or STYLE, as these do not address compiler errors or warnings, and some are clearly STYLE changes (e.g. re-ordering the flags, using local variables, etc.).
• Can commit body messages be added to all commits, please? And the subject start with capitals in 1b1e625?
• Can this be split into 2 PRs: one for the refactoring of the GHA workflow commits, the other one for commit c636441?
• 1922edb and b6c3871 needd to be reworked, merged or refactored: e.g. a path is added in the first, that is then removed in the second.
• Split things in another way may clarify what is being done: e.g. reordering the flags can go in its own commit, changing the paths can go in its own commit, specifying the CMake variable types in the flags, etc. Otherwise, changes are difficult to review.
• Despite 263450a, and despite your PR to Slicer, -DSlicer_USE_PYTHONQT is still ON. Can this be turned off now?
• Why are we removing Ninja? Why do we prefer a second call to cmake instead of an explicit call to a specific compiler? If this is in line with streamline future maintenance (e.g generalization to other operating system), that should be clear close to the explanation about removing Ninja. And clearly, that is a separate commit. Also, if at some point we want to use Ninja instead of the default compiler CMake uses, will cmake do this, if GCC is always present?
  Also, part of the 1922edb commit message in here is contradicting

    - Avoid explicit use build tool by leveraging `cmake --build <build-dir> --config <config>`
    - Since ninja automatically enables parallelism based on the number of available
      core, remove the use of `$(nproc)`
  

  Is it cmake that automatically enables parallelism ?
  Also the commit message ends with an unmeaningful ferfer.
• Why do we turn on support for NumPy? Previous builds are passing without it. I would first demonstrate (maybe with the Python test commit in the separate PR) that tests do not pass otherwise.

Thanks again JC.

[jcfr]

re: ENG, STYLE, ...

Since most of these changes are related to continuous integration and compilation, I chose COMP, see https://slicer.readthedocs.io/en/latest/developer_guide/style_guide.html#commit-message-prefix

Can commit body messages be added to all commits, please?

I will do my best to have additional reference.

Can this be split into 2 PRs: one for the refactoring of the GHA workflow commits, the other one for commit

I was waiting for the CI to complete, since it is time consuming I deliberately decided to wait.

-DSlicer_USE_PYTHONQT is still ON. Can this be turned off now?

Python is currently a necessary requirement for this extension. This should be addressed on its own.

Why are we removing Ninja?

Ninja generator is still used. We are leveraging the CMake interface to build. This will then allow to choose different generator, support visual studio, etc ... without having to have custom invocation for the build tool.

that automatically enables parallelism ?

Ninja automatically automatically enable parallelism. So specifying number of process is not needed here.

Instead, we should look into having different for the link and build pool, but that is another topic. See https://discourse.slicer.org/t/ninja-build-using-too-many-cores/2304/2

Also the commit message ends with an unmeaningful ferfer

Good catch. Remaining of recent commit squashing. I will fix it.

Why do we turn on support for NumPy?

rg numpy
DMRIModuleTemplates/Tractography_Scripted/TemplateKey.py
6:from vtk.numpy_interface import dataset_adapter as dsa
9:import numpy as np
166:    fb_numpy = dsa.WrapDataObject(pd)
169:    ten_key = next(x for x in fb_numpy.PointData.keys() if "Tensor_" in x)
175:    tensors = fb_numpy.PointData[ten_key]
179:    fb_numpy.PointData.append(md, "CalculatedMD")

DMRIModuleTemplates/DWI_Scripted/TemplateKey.py
5:import numpy as np
171:    # get the image data as numpy arrays (no copy)
173:    grads_array = vtk.util.numpy_support.vtk_to_numpy(inputVolume.GetDiffusionGradients())
174:    bvals_array = vtk.util.numpy_support.vtk_to_numpy(inputVolume.GetBValues())

Modules/CLI/ExtractDWIShells/ExtractDWIShells.py
9:import numpy as np
11:from vtk.util import numpy_support
22:  import numpy.testing
47:    bvals = vtk.util.numpy_support.vtk_to_numpy(dw_node.GetBValues())
64:    numpy.testing.assert_allclose(bvals, bvals_expected, rtol=1e-05)
85:    numpy.testing.assert_allclose(bvals, bvals_expected, rtol=1e-5)
133:  bvals_in = numpy_support.vtk_to_numpy(node_in.GetBValues())
134:  grads_in = numpy_support.vtk_to_numpy(node_in.GetDiffusionGradients())
200:  node_out.SetBValues(numpy_support.numpy_to_vtk(bvals_out))
201:  node_out.SetDiffusionGradients(numpy_support.numpy_to_vtk(grads_out))

Modules/Scripted/TractographyDownsample/TractographyDownsample.py
9:import numpy as np

Modules/Scripted/FiberBundleToLabelMap/FiberBundleToLabelMap.py
5:import vtk, qt, ctk, slicer, numpy
203:      cellIdsAr = numpy.arange(startCellId,
205:                               dtype=vtk.util.numpy_support.ID_TYPE_CODE)

Modules/Scripted/TractographyExportPLY/TractographyExportPLY.py
4:import numpy as np

Previous builds are passing without it

Building without running the test and/or using the modules is not sufficient.

Disabling NumPy would prevent from running any tests.

[jcfr]

I believe all comments have been addressed. Thanks for the review.

[jhlegarreta]

A few comments:

• I know the below will be difficult as you will need to rebase interactively, and it will need another round of CIs, but the effort is worthwhile I think.
• Again, I would leave this for a separate PR: 5120df0: CIs are passing, so Python testing is not being used. And the rest of the commits in this PR are only commits that reorganize, improve the workflow file, but do not change any flags.
• C&P the commit subjects to replace the PR message ? A matter of preference maybe, but the current one is already nice, so leaving this to your preference.
• Shorten commit message subjects by rewording them ? e.g.
  STYLE: Update GHA worklfow sorting CMake options alphabetically
STYLE: Sort CMake options alphabetically in GHA worklfow
  This will avoid the commit 9254836 subject from overflowing the 72 character length:

  STYLE: Update GHA workflow specify type of CMake variable (e.g BOOL…
   … and `PATH`)
  

• Why don't we drop "This commit (...)" from all commit messages? They do not add any meaningful information, make reading longer/getting the message more difficult, and avoid a number of grammar mistakes: mainly missing or spare "s"/"ies"
  • 5001884: "Sorting options alphabetically allows (...)"
  • 375fcd5: "This commits removes the settings of options implicitly enabled or disabled.": so change to "Remove setting options implicitly (...)"; "This options (...)": "This option (...)" or "These options (...)"
  • 549722b: "This commit simplifyies (...)", "(...) as it decouples": "Simplify (...)"
  • 04c8185: "This commit refactor (...)": "Refactor (...)";
  • 0b4d1d9: "(...) based on the number of available cores, (...)"; unfinished sentence: > Ninja defaults to running commands in parallel anyway, so typically you don’t need to pass
  • 73b489f: "This commit disables the installation of Qt Pythons (...)" : "Do not install (...) as it is not used by this project"
  • ae1bb83: Start commit subject with capitals
  • 06d9988: "This commit switch to using the (...)": "Switch to using the (...)"
• A very minor comment: use "-" or "*" consistently in lists in commit messages; do not add blank lines across them.

[jcfr]

A very minor comment: use "-" or "*" consistently in lists in commit messages; do not add blank lines across them.

Adding the empty lines help visually parse the content. This is particularly important because:

1. we are dealing with regular text (it is not markdown content that is rendered)
2. each item spans over two lines

All comment have been address and dedicated PR will be pushed to include the NumPy updates