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: Bump reusable workflow for wheel validation #71

Merged
merged 2 commits into from
Jan 21, 2023

Conversation

tbirdso
Copy link
Contributor

@tbirdso tbirdso commented Jan 19, 2023

Includes checks for wheel existence and naming scheme after wheel build routines.

@tbirdso tbirdso requested a review from dzenanz January 19, 2023 18:04
@tbirdso tbirdso marked this pull request as ready for review January 19, 2023 18:04
@dzenanz
Copy link
Member

dzenanz commented Jan 19, 2023

Maybe this CMake warning is the culprit for breakage?

CMake Warning (dev) at /home/runner/work/_temp/1570127931/cmake-3.24.2-linux-x86_64/share/cmake-3.24/Modules/ExternalProject.cmake:3074 (message):
  The DOWNLOAD_EXTRACT_TIMESTAMP option was not given and policy CMP0135 is
  not set.  The policy's OLD behavior will be used.  When using a URL
  download, the timestamps of extracted files should preferably be that of
  the time of extraction, otherwise code that depends on the extracted
  contents might not be rebuilt if the URL changes.  The OLD behavior
  preserves the timestamps from the archive instead, but this is usually not
  what you want.  Update your project to the NEW behavior or specify the
  DOWNLOAD_EXTRACT_TIMESTAMP option with a value of true to avoid this
  robustness issue.
Call Stack (most recent call first):
  /home/runner/work/_temp/1570127931/cmake-3.24.2-linux-x86_64/share/cmake-3.24/Modules/ExternalProject.cmake:4170 (_ep_add_download_command)
  /home/runner/work/ITKSplitComponents/ITK/Utilities/ClangFormat/DownloadClangFormat.cmake:37 (ExternalProject_add)
  /home/runner/work/ITKSplitComponents/ITK/CMake/ITKModuleClangFormat.cmake:14 (include)
  /home/runner/work/ITKSplitComponents/ITK/CMake/ITKModuleMacros.cmake:9 (include)
  /home/runner/work/ITKSplitComponents/ITK/CMake/ITKModuleExternal.cmake:140 (include)
  CMakeLists.txt:9 (include)

@tbirdso
Copy link
Contributor Author

tbirdso commented Jan 19, 2023

Yes, though I am curious about where the issue is introduced. I am re-running checks in master for comparison: https://github.com/InsightSoftwareConsortium/ITKSplitComponents/actions/runs/3961122231

@dzenanz
Copy link
Member

dzenanz commented Jan 19, 2023

Was CMake version bumped? I think this warning starts with CMake 3.24.

@tbirdso
Copy link
Contributor Author

tbirdso commented Jan 19, 2023

You're right, the CMake version was bumped as part of warning fixes.

InsightSoftwareConsortium/ITKRemoteModuleBuildTestPackageAction@6e81bbd

It seems like this fix should be handled in ITK proper since it involves setting a CMake policy?

EDIT: Already logged in InsightSoftwareConsortium/ITK#3846

@tbirdso
Copy link
Contributor Author

tbirdso commented Jan 19, 2023

Re-running checks in master with CMake 3.24 has passed without warnings: https://github.com/InsightSoftwareConsortium/ITKSplitComponents/actions/runs/3961122231/jobs/6788272317

InsightSoftwareConsortium/ITK#3846 documents the issue but also includes references to where fixes where already introduced to ITK. Unclear where the error is suddenly coming from here.

@dzenanz
Copy link
Member

dzenanz commented Jan 19, 2023

ITKSplitComponents might need to set the policy, like this:
https://github.com/InsightSoftwareConsortium/ITKIOOMEZarrNGFF/blob/v0.1.0/CMakeLists.txt#L2-L4

@tbirdso
Copy link
Contributor Author

tbirdso commented Jan 19, 2023

Could this be added to ITKModuleExternal.cmake rather than requiring that each individual module set its own policy?

@tbirdso
Copy link
Contributor Author

tbirdso commented Jan 19, 2023

Error appears to have cleared on re-run for no reason? 😕

@dzenanz
Copy link
Member

dzenanz commented Jan 19, 2023

Could this be added to ITKModuleExternal.cmake rather than requiring that each individual module set its own policy?

Requiring a newer version of CMake will automatically set all previously introduced policies to NEW. Setting things in ITKModuleExternal.cmake would affect all remotes, even non-trivial ones. Maybe DownloadClangFormat.cmake could set that policy to NEW? Or push policy, set 135 to NEW, then pop?

@tbirdso
Copy link
Contributor Author

tbirdso commented Jan 20, 2023

Setting things in ITKModuleExternal.cmake would affect all remotes, even non-trivial ones.

Yes, that would be the goal. If that's not desirable then I'm fine with setting the policy here as needed. However, it seems like the issue may not be present in the latest run. Will do a third run to check again.

EDIT: MacOS builds have failed with warnings on the third, fourth, and fifth runs. No clue why the second passed. Will add a line to set the policy in CMakeLists.txt.

Includes checks for wheel existence and naming scheme after wheel build
routines.
Resolves CMake warning regarding CMP135 policy introduced in CMake 3.24
@tbirdso tbirdso merged commit 3030e1e into InsightSoftwareConsortium:master Jan 21, 2023
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.

2 participants