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

ITK Testing Macros should be prefixed with ITK_ #940

Closed
hjmjohnson opened this issue May 21, 2019 · 3 comments · Fixed by #999
Closed

ITK Testing Macros should be prefixed with ITK_ #940

hjmjohnson opened this issue May 21, 2019 · 3 comments · Fixed by #999
Labels
Good first issue A good issue for community members new to contributing type:Style Style changes: no logic impact (indentation, comments, naming)

Comments

@hjmjohnson
Copy link
Member

Description

In general we should have an ITK prefix for macros so we know where they come from and avoid conflicts. However, this should be the case with all the testing macros:

https://github.com/InsightSoftwareConsortium/ITK/blob/master/Modules/Core/TestKernel/include/itkTestingMacros.h

Expected coding style

ITK Macros should be prefixed with ITK_

Actual coding style

There are inconsistencies.

@hjmjohnson hjmjohnson added Good first issue A good issue for community members new to contributing type:Style Style changes: no logic impact (indentation, comments, naming) labels May 21, 2019
@dzenanz
Copy link
Member

dzenanz commented Jun 17, 2019

The remote module's should be migrated to new name with ITK_ prefix, but the ITK version used in remote's CI testing need to be updated to include the new definitions. Otherwise the CI will be broken. Should we update modules' CI to use the current master, or wait until we have released 5.1.0 and then update the remotes? @thewtex @jhlegarreta

@dzenanz dzenanz reopened this Jun 17, 2019
@jhlegarreta
Copy link
Member

IMHO, once a release is tagged, there is little benefit of linking the remotes' master against that release, since in the subsequent ITK release we will want the remotes to be successful with the new ITK release, not the former one. And that means that they should be successful against ITK master up to a given point in time. If they are only tested when the release is tagged and the Azure CI version updated, we may be holding a number of potential issues that will be uncovered at that point, which will concentrate the workload to that very moment.

If I do get it right, the Expected Nightly Remote Modules section in CDash tests nightly the remotes against ITK master. But it does so using the remote module commit hash that is present in the *.remote.cmake file, which may be behind the corresponding remote's master.

On the other hand, when generating the remotes' wheels to be uploaded to PyPI, we will want them to be tested against an ITK release version. Not sure whether this is the logic behind the PR Matt opened in the ITKModuleTemplate.

But I may be missing some piece of information to understand the mentioned PR, and the workload concerning the CI's that all this involves.

@dzenanz
Copy link
Member

dzenanz commented Jul 9, 2019

Follow-up work in #1015 and #1081.

@dzenanz dzenanz closed this as completed Jul 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good first issue A good issue for community members new to contributing type:Style Style changes: no logic impact (indentation, comments, naming)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants