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.

Sign up for GitHub

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

Jump to bottom

ENH: Update to NIFTILIB3.0.1 #2630

Merged
merged 3 commits into from
Jul 9, 2021
Merged

ENH: Update to NIFTILIB3.0.1 #2630

merged 3 commits into from
Jul 9, 2021

Conversation

gdevenyi
Copy link
Contributor

@gdevenyi gdevenyi commented Jul 2, 2021 •
edited
Loading

Update embedded ITK version of NIFTILIB from snapshot of master to latest 3.0.1.

Fixes #1924

PR Checklist

  • No API changes were made (or the changes have been approved)
  • No major design changes were made (or the changes have been approved)
  • Added test (or behavior not changed)
  • Updated API documentation (or API not changed)
  • Added license to new files (if any)
  • Added Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5
  • Added ITK examples for all new major features (if any)

Refer to the ITK Software Guide for
further development details if necessary.

@github-actions github-actions bot added area:ThirdParty Issues affecting the ThirdParty module type:Enhancement Improvement of existing methods or implementation type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots labels Jul 2, 2021
@gdevenyi gdevenyi changed the title ENH: Update to NIFTILIB3.0.01 ENH: Update to NIFTILIB3.0.1 Jul 2, 2021
@gdevenyi gdevenyi marked this pull request as ready for review July 2, 2021 21:00
@gdevenyi
Copy link
Contributor Author

gdevenyi commented Jul 3, 2021

CC @hjmjohnson this swap in passes all tests. zlib-ng testing is stalled due to internal ITK zlib actually being heavily modified source.

dzenanz
dzenanz reviewed Jul 5, 2021
View reviewed changes
Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

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

There are some style complaints from ghostflow:
https://github.com/InsightSoftwareConsortium/ITK/pull/2630/checks?check_run_id=2975705588
but this looks great!

@gdevenyi
Copy link
Contributor Author

gdevenyi commented Jul 5, 2021

There are some style complaints from ghostflow:
#2630 (checks)
but this looks great!

How does this work here, these are defects upstream, are we expected to fix them?

@dzenanz
Copy link
Member

dzenanz commented Jul 5, 2021

trailing whitespace and blank line at EOF fixes will probably be accepted upstream? But as the message itself says, the warnings don't HAVE to be fixed. And as zlib->zlib-ng swap is already getting quite a big effort, we could leave these fixes for the future?

@gdevenyi
Copy link
Contributor Author

gdevenyi commented Jul 5, 2021

trailing whitespace and blank line at EOF fixes will probably be accepted upstream? But as the message itself says, the warnings don't HAVE to be fixed. And as zlib->zlib-ng swap is already getting quite a big effort, we could leave these fixes for the future?

@hjmjohnson is an upstream developer and so may be interested in the style reports of https://github.com/InsightSoftwareConsortium/ITK/pull/2630/checks?check_run_id=2975705588 then

@thewtex thewtex requested a review from hjmjohnson July 7, 2021 20:30
thewtex
thewtex reviewed Jul 7, 2021
View reviewed changes
Copy link
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

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

@gdevenyi thanks!!

It looks like there are support script that we do not want / need from the the upstream repository. Could they be removed in UpdateFromUpstream.sh?

gdevenyi and others added 3 commits July 8, 2021 09:12
@gdevenyi
ENH: Prepare for update to NIFTILIB3.0.1
68221fd
@gdevenyi
nifti 2020-08-07 (96b9954f)
9c2c9fa 
Code extracted from:

    https://github.com/NIFTI-Imaging/nifti_clib.git

at commit 96b9954fafcf3bd69a4ee2091b0d707ed2077697 (v3.0.1).
@gdevenyi
Merge branch 'upstream-nifti' into nifticlib3.0
faa754c 
# By NIFTI Upstream
* upstream-nifti:
  nifti 2020-08-07 (96b9954f)
@gdevenyi
Copy link
Contributor Author

gdevenyi commented Jul 8, 2021

Good catch @thewtex, I have no idea why nifti2 directrory was originally included, we disable nifti2 anyways.

@thewtex
Copy link
Member

thewtex commented Jul 9, 2021

@gdevenyi thanks!! ⭐

@thewtex thewtex merged commit cf3cea5 into InsightSoftwareConsortium:master Jul 9, 2021
@gdevenyi gdevenyi deleted the nifticlib3.0 branch December 24, 2021 15:07
@jcfr jcfr mentioned this pull request Feb 24, 2022
Closed
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Mar 12, 2024
@N-Dekker
STYLE: No longer set hooks-max-size for nifti2_io.c, doxygen.config.in
7be11a1 
nifti2_io.c was removed by:
- pull request InsightSoftwareConsortium#2630
commit 68221fd
"ENH: Prepare for update to NIFTILIB3.0.1", 9 July 2021

The two doxygen.config.in files were removed by:
- pull request InsightSoftwareConsortium#3311
commit a96d03f
"COMP: Use modern doxygen_add_docs command ITK_WRAP_DOCS", 11 March 2022
- pull request InsightSoftwareConsortium#4500
commit 0c5c638
"STYLE: Remove doxygen.config.in (superseded by DoxygenConfig.cmake)", 12 March
2024
@N-Dekker N-Dekker mentioned this pull request Mar 12, 2024
Merged
hjmjohnson pushed a commit that referenced this pull request Mar 23, 2024
@N-Dekker @hjmjohnson
STYLE: No longer set hooks-max-size for nifti2_io.c, doxygen.config.in
88bfbfd 
nifti2_io.c was removed by:
- pull request #2630
commit 68221fd
"ENH: Prepare for update to NIFTILIB3.0.1", 9 July 2021

The two doxygen.config.in files were removed by:
- pull request #3311
commit a96d03f
"COMP: Use modern doxygen_add_docs command ITK_WRAP_DOCS", 11 March 2022
- pull request #4500
commit 0c5c638
"STYLE: Remove doxygen.config.in (superseded by DoxygenConfig.cmake)", 12 March
2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thewtex thewtex thewtex approved these changes

@dzenanz dzenanz dzenanz approved these changes

@hjmjohnson hjmjohnson Awaiting requested review from hjmjohnson

Assignees
No one assigned
Labels
area:ThirdParty Issues affecting the ThirdParty module type:Enhancement Improvement of existing methods or implementation type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Integrate new nifticlib 3.0.0
3 participants
@gdevenyi @dzenanz @thewtex