Skip to content

COMP: Add minimum version for double-conversion#3160

Merged
dzenanz merged 1 commit into
InsightSoftwareConsortium:masterfrom
RafaelPalomar:fix/double-conversion_system_version
Feb 4, 2022
Merged

COMP: Add minimum version for double-conversion#3160
dzenanz merged 1 commit into
InsightSoftwareConsortium:masterfrom
RafaelPalomar:fix/double-conversion_system_version

Conversation

@RafaelPalomar

Copy link
Copy Markdown
Contributor

ITK requires double-conversion >=3.1.6. This commit adds the minimum
version for the case where ITK_USE_SYSTEM_DOUBLECONVERSION=ON

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.

ITK requires double-conversion >=3.1.6. This commit adds the minimum
version for the case where ITK_USE_SYSTEM_DOUBLECONVERSION=ON
@github-actions github-actions Bot added area:ThirdParty Issues affecting the ThirdParty module type:Compiler Compiler support or related warnings type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots labels Feb 4, 2022
@dzenanz dzenanz merged commit 701c8c2 into InsightSoftwareConsortium:master Feb 4, 2022
@RafaelPalomar RafaelPalomar deleted the fix/double-conversion_system_version branch February 5, 2022 08:25
@N-Dekker

N-Dekker commented Feb 11, 2022

Copy link
Copy Markdown
Contributor

@RafaelPalomar @hjmjohnson @dzenanz I'm very glad to see ITK requires double-conversion >=3.1.6 now, as version 3.1.6 has included my contribution from last year, pull request google/double-conversion#158 commit google/double-conversion@eee1a45 "Add StringToDoubleConverter::StringTo<T> member function templates" @RafaelPalomar Was that also why you wanted double-conversion >=3.1.6, or did you have a different reason?

@hjmjohnson Can you possibly also upgrade the double-conversion version at https://github.com/InsightSoftwareConsortium/ITK/tree/master/Modules/ThirdParty/DoubleConversion/src ? You did so before with pull request #2214 commit ee51ee1, but that was still in 2020, while the minimum required double-conversion version v3.1.6 is from December 2021!

P.S. double-conversion version 3.1.6 has included an integer constant that I asked for (issue google/double-conversion#146), kMaxCharsEcmaScriptShortest, with pull request google/double-conversion#150 commit google/double-conversion@a3dc3ab so yes, I do look forward to double-conversion 3.1.6 😃

@RafaelPalomar

Copy link
Copy Markdown
Contributor Author

Hello @N-Dekker. I'm writing a Gentoo ebuild for ITK and realized that using the system double-conversion 3.1.5 provided by Gentoo Linux, double-conversion/double-to-string.h was missing and 3.1.6 did provide it. This is currently used by itkNumberToString.cxx

@dzenanz

dzenanz commented Feb 11, 2022

Copy link
Copy Markdown
Member

@N-Dekker

Copy link
Copy Markdown
Contributor

@dzenanz I have no experience running UpdateDoubleConversionFromGoogle.sh so I'm just hoping that someone more experienced will do it 😄

I just noticed that they recently started tagging double-conversion more regularly: https://github.com/google/double-conversion/tags So maybe it's worth for ITK to update double-conversion to a specific tag, rather than a commit SHA. v3.1.6 would then obviously be the minimum, but I guess v3.2.0 would also be fine.

@dzenanz

dzenanz commented Feb 11, 2022

Copy link
Copy Markdown
Member

With this and running on Windows git bash I get many merge conflicts:

Dzenan@Ryzenator MINGW64 /c/Misc/ITK-patches2 (DoubleConversion-update)
$ ./Modules/ThirdParty/DoubleConversion/UpdateDoubleConversionFromGoogle.sh
-------------------------------------------------------
This script will update source code for the double-conversion
library from http://code.google.com
-------------------------------------------------------
Initialized empty Git repository in C:/Misc/ITK-patches2/DoubleConversion-Tmp/.git/
remote: Enumerating objects: 172267, done.
remote: Counting objects: 100% (172267/172267), done.
remote: Compressing objects: 100% (38514/38514), done.
Receiving objects: 100% (172267/172267), 37.40 MiB | 2.85 MiB/s, done.
remote: Total 172267 (delta 135323), reused 166773 (delta 132439), pack-reused 0
Resolving deltas: 100% (135323/135323), done.
From ..
 * branch                DoubleConversion-upstream -> FETCH_HEAD
Updating files: 100% (8106/8106), done.
Cloning upstream HEAD from code.google.com
NOTE: to check out a particular revision for merging
you have to add a git checkout <hash>
or git checkout <branchname>
after the git clone command
Cloning into 'double-conversion'...
remote: Enumerating objects: 1338, done.
remote: Counting objects: 100% (182/182), done.
remote: Compressing objects: 100% (141/141), done.
remote: Total 1338 (delta 98), reused 83 (delta 34), pack-reused 1156
Receiving objects: 100% (1338/1338), 7.14 MiB | 18.66 MiB/s, done.
Resolving deltas: 100% (870/870), done.
LICENSE file unchanged
Enumerating objects: 24, done.
Counting objects: 100% (24/24), done.
Delta compression using up to 16 threads
Compressing objects: 100% (23/23), done.
Writing objects: 100% (23/23), 74.11 KiB | 1.19 MiB/s, done.
Total 23 (delta 5), reused 0 (delta 0), pack-reused 0
To ..
   c85be8d0b..ff41670d9  HEAD -> DoubleConversion-upstream
fatal: a branch named 'DoubleConversion-update' already exists
Performing inexact rename detection: 100% (177144/177144), done.
Performing inexact rename detection: 100% (169071/169071), done.
CONFLICT (add/add): Merge conflict in Modules/ThirdParty/DoubleConversion/src/double-conversion/utils.h
Auto-merging Modules/ThirdParty/DoubleConversion/src/double-conversion/utils.h
CONFLICT (add/add): Merge conflict in Modules/ThirdParty/DoubleConversion/src/double-conversion/strtod.h
Auto-merging Modules/ThirdParty/DoubleConversion/src/double-conversion/strtod.h
CONFLICT (add/add): Merge conflict in Modules/ThirdParty/DoubleConversion/src/double-conversion/strtod.cc
Auto-merging Modules/ThirdParty/DoubleConversion/src/double-conversion/strtod.cc
CONFLICT (add/add): Merge conflict in Modules/ThirdParty/DoubleConversion/src/double-conversion/string-to-double.h
Auto-merging Modules/ThirdParty/DoubleConversion/src/double-conversion/string-to-double.h
CONFLICT (add/add): Merge conflict in Modules/ThirdParty/DoubleConversion/src/double-conversion/string-to-double.cc
Auto-merging Modules/ThirdParty/DoubleConversion/src/double-conversion/string-to-double.cc
CONFLICT (add/add): Merge conflict in Modules/ThirdParty/DoubleConversion/src/double-conversion/ieee.h
Auto-merging Modules/ThirdParty/DoubleConversion/src/double-conversion/ieee.h
CONFLICT (add/add): Merge conflict in Modules/ThirdParty/DoubleConversion/src/double-conversion/fixed-dtoa.cc
Auto-merging Modules/ThirdParty/DoubleConversion/src/double-conversion/fixed-dtoa.cc
CONFLICT (add/add): Merge conflict in Modules/ThirdParty/DoubleConversion/src/double-conversion/fast-dtoa.cc
Auto-merging Modules/ThirdParty/DoubleConversion/src/double-conversion/fast-dtoa.cc
CONFLICT (add/add): Merge conflict in Modules/ThirdParty/DoubleConversion/src/double-conversion/double-to-string.h
Auto-merging Modules/ThirdParty/DoubleConversion/src/double-conversion/double-to-string.h
CONFLICT (add/add): Merge conflict in Modules/ThirdParty/DoubleConversion/src/double-conversion/double-to-string.cc
Auto-merging Modules/ThirdParty/DoubleConversion/src/double-conversion/double-to-string.cc
CONFLICT (add/add): Merge conflict in Modules/ThirdParty/DoubleConversion/src/double-conversion/bignum.cc
Auto-merging Modules/ThirdParty/DoubleConversion/src/double-conversion/bignum.cc
CONFLICT (add/add): Merge conflict in Modules/ThirdParty/DoubleConversion/src/double-conversion/bignum-dtoa.cc
Auto-merging Modules/ThirdParty/DoubleConversion/src/double-conversion/bignum-dtoa.cc
CONFLICT (modify/delete): Modules/ThirdParty/DoubleConversion/src/double-conversion/CMakeLists.txt deleted in DoubleConversion-upstream and modified in HEAD. Version HEAD of Modules/ThirdParty/DoubleConversion/src/double-conversion/CMakeLists.txt left in tree.
Automatic merge failed; fix conflicts and then commit the result.
---------------------------------
NOW Fix all conflicts and test new code.
---------------------------------
Commit the merged/fixed/verified code and run this command
git rev-parse --short=16 DoubleConversion-upstream
---------------------------------
to get the commit hash from which the DoubleConversion-upstream
branch must be started on the next update.
---------------------------------
edit the line "git branch DoubleConversion-upstream" above.
Once you have commited this change to the UpdateDoubleConversionFromGoogle.sh script,
use "git gerrit-push" to push this new update branch back to itk.org.

@dzenanz

dzenanz commented Feb 11, 2022

Copy link
Copy Markdown
Member

There are conflicts when I do this on Linux, too. I guess we should update the script (#3193) for this to work smoothly, or at least more smoothly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:ThirdParty Issues affecting the ThirdParty module type:Compiler Compiler support or related warnings type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants