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

Build ANTs on windows-2019 GitHub Actions CI runner #3

Merged
merged 27 commits into from
May 16, 2022

Conversation

joshuacwnewton
Copy link
Member

@joshuacwnewton joshuacwnewton commented May 13, 2022

This PR adds another workflow step to build-ants.yml to build ANTs for Windows.

This would supersede the locally-built binaries from spinalcordtoolbox/spinalcordtoolbox#3682.

@joshuacwnewton joshuacwnewton force-pushed the jn/build-ants-on-windows branch from 16422c2 to ba01bf9 Compare May 13, 2022 14:10
working-directory: antsbin
run: |
ls
make VERBOSE=1 -j 4
Copy link
Member Author

@joshuacwnewton joshuacwnewton May 13, 2022

Choose a reason for hiding this comment

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

During the make step, I ran into the exact same path-length issue that I ran into when building ITK locally on Windows (spinalcordtoolbox/spinalcordtoolbox#3682):

 CMake Error at CMakeLists.txt:109 (message):
  ITK source code directory path length is too long (64 > 50).Please move the
-- Configuring incomplete, errors occurred!
  ITK source code directory to a directory with a shorter path.
See also "D:/a/spinalcordtoolbox-ants/spinalcordtoolbox-ants/antsbin/ITKv5-build/CMakeFiles/CMakeOutput.log".

As a short-term fix (as to not impede debugging), I'm going to rename the repository from spinalcordtoolbox-ants (22 chars) to build_ANTs (10 chars) which should save 12x2=24 characters, putting us well under the 50 char limit.

But, in the long-term, it would be nice to see if there are any better workarounds.

Copy link
Member

Choose a reason for hiding this comment

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

There may be some hints on this github issue. In particular, there's a link to the ITK CMakeLists.txt file which includes the 50-character check, so it's an ITK-specific limit. It may be too conservative, since the Windows limit looks like it's 260 characters (does the ITK hierarchy require 260-50=210 characters?). It looks like the 50-character limit is not configurable, but there is an option ITK_SKIP_PATH_LENGTH_CHECKS which skips it entirely.

Copy link
Member Author

Choose a reason for hiding this comment

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

!! Thank you for the sleuthing!

Copy link
Member Author

@joshuacwnewton joshuacwnewton Sep 1, 2022

Choose a reason for hiding this comment

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

Thank you from the future, @mguaypaq! Your comment here saved me some trouble while working on jqmcginnis/build_biomedia_stitching#1, as I needed to build ITK once more for that project. 😄

Specifying -DITK_SKIP_PATH_LENGTH_CHECKS=ON worked like a charm.

@joshuacwnewton joshuacwnewton force-pushed the jn/build-ants-on-windows branch from 5fc081d to a4e7a97 Compare May 13, 2022 15:23
@joshuacwnewton joshuacwnewton force-pushed the jn/build-ants-on-windows branch from a4e7a97 to ed5617a Compare May 13, 2022 15:26
This is an attempt to fix https://github.com/spinalcordtoolbox/build_ANTs/pull/3/files#r872663217, but may have unintended consequences for SCT.

I'm just trying this out to see how the build goes across our platforms.
@joshuacwnewton joshuacwnewton force-pushed the jn/build-ants-on-windows branch from 89df670 to 07a466d Compare May 13, 2022 18:41
@joshuacwnewton joshuacwnewton force-pushed the jn/build-ants-on-windows branch from 139a5c3 to 6849dc8 Compare May 16, 2022 17:01
@joshuacwnewton joshuacwnewton changed the title Debug building ANTs on Windows Build ANTs on windows-2019 GitHub Actions CI runner May 16, 2022
@joshuacwnewton joshuacwnewton force-pushed the jn/build-ants-on-windows branch from a171bb6 to 3546114 Compare May 16, 2022 18:09
@joshuacwnewton
Copy link
Member Author

I'm going to merge this PR, as the resulting binaries pass SCT's test suite.

I plan on requesting further comments on the SCT pull request; I just want to keep the main discussion focused over there.

@joshuacwnewton joshuacwnewton merged commit ad68c79 into master May 16, 2022
@joshuacwnewton joshuacwnewton deleted the jn/build-ants-on-windows branch May 16, 2022 20:50
Copy link
Member

@mguaypaq mguaypaq left a comment

Choose a reason for hiding this comment

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

Thanks for figuring out the ITK hotfix to get it working on Windows, without bumping the version for the other platforms! Other than that this extra Windows build follows the pattern for the existing builds, and empirically it works, so LGTM :)

# snip out the apps we need for https://github.com/neuropoly/spinalcordtoolbox, since including all of ANTS is too much
mkdir sct-apps/
cp antsbin/ANTS-build/Examples/Release/{antsRegistration,antsSliceRegularizedRegistration,antsApplyTransforms,ComposeMultiTransform}.exe sct-apps
(cd sct-apps; for i in `ls`; do mv $i isct_$i; done)
Copy link
Member

@mguaypaq mguaypaq May 20, 2022

Choose a reason for hiding this comment

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

This is a nitpick, but for i in `ls` in bash is better written as for i in *, as per this article.

(This would have been out of scope for this PR anyway, since this pattern exists elsewhere in the file, and we shouldn't mess too much with something that's working. More of an FYI.)

joshuacwnewton added a commit that referenced this pull request May 24, 2022
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