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

DOC: Clarify further remote module version bumps with ITK minor releases #4769

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jhlegarreta
Copy link
Member

Clarify further remote module major version bumps with ITK minor releases.

PR Checklist

  • No API changes were made (or the changes have been approved)
  • No [major design changes]

Clarify further remote module major version bumps with ITK minor
releases.
@github-actions github-actions bot added type:Documentation Documentation improvement or change area:Documentation Issues affecting the Documentation module labels Jul 4, 2024
@jhlegarreta
Copy link
Member Author

@SimonRit following the thread in PR #4765 and Matt's clarification.

@blowekamp
Copy link
Member

Why is this preferred over more specific pinning of the package install requirements to something like "itk~=5.4.0"?

@SimonRit
Copy link

SimonRit commented Jul 5, 2024

Thanks for integrating Matt's comment to the documentation. I still don't really understand why the major version of the remote module should be bumped but not the major version of ITK but this is most likely due to a lack of knowledge on my side.

@blowekamp
Copy link
Member

My suspicion is that this line is not correct:
https://github.com/InsightSoftwareConsortium/ITKIsotropicWavelets/blob/master/setup.py#L57

and should be something like "itk~=5.4.0" to only allow the ITK patch version to very.

@jhlegarreta
Copy link
Member Author

My suspicion is that this line is not correct:
https://github.com/InsightSoftwareConsortium/ITKIsotropicWavelets/blob/master/setup.py#L57
and should be something like "itk~=5.4.0" to only allow the ITK patch version to very.

InsightSoftwareConsortium/ITKIsotropicWavelets#159 and #4748 (comment).

While we attempt to maintain API compatibility between ITK minor releases, the
ABI generally changes. This can be due to features and bug fixes in the ITK
source code, but there are also usually build system changes that cause ABI
changes. Thus, remote module versions should be bumped into a new major
Copy link
Member

@blowekamp blowekamp Jul 5, 2024

Choose a reason for hiding this comment

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

So, While ITK's main Python packages do not follow semantic versioning, it is recommended that remote module bump the major version when the minor version of ITK change to follow semantic versioning best practices.

Is that clearer?

@thewtex
Copy link
Member

thewtex commented Jul 5, 2024

Perhaps it is clearer to have remote module package versions follow their own semantic versioning path, i.e. bump the minor version when bumping the itk version they build against ?. However, have the dependency on itk-X.X.* indicate the ABI compatibility in this explanation.

@SimonRit
Copy link

SimonRit commented Jul 6, 2024

Perhaps it is clearer to have remote module package versions follow their own semantic versioning path, i.e. bump the minor version when bumping the itk version they build against ?. However, have the dependency on itk-X.X.* indicate the ABI compatibility in this explanation.

This has my preference but it might be because I didn't get the explanation...

@thewtex
Copy link
Member

thewtex commented Jul 8, 2024

I initially thought a remote module update should be considered a breaking change because it forces an itk version update, but this seems too stringent.

@thewtex
Copy link
Member

thewtex commented Jul 8, 2024

Does everyone agree ( 👍 👎 )that we should recommend bumping the minor version on remote module packages with a new itk major or minor release?

@jhlegarreta
Copy link
Member Author

Folks, please, go ahead and edit the change and push force so that it reflects better what the policy should be so that the policy and the underlying rationale are clear onwards.

@jhlegarreta
Copy link
Member Author

The below will need to be changed in accordance to what is adopted here:
https://github.com/InsightSoftwareConsortium/ITK/blob/master/Utilities/Maintenance/UpdateRequiredITKVersionInRemoteModules.sh#L26-L27

as well as the logic in the script.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Documentation Issues affecting the Documentation module type:Documentation Documentation improvement or change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants