itk-wasm icon indicating copy to clipboard operation
itk-wasm copied to clipboard

Use upstream cpp-base64 git url

Open jadh4v opened this issue 3 years ago • 3 comments

set(cpp_base64_GIT_REPOSITORY "https://github.com/CodeFinder2/cpp-base64.git")

Ideally improvement to dependency should be contributed upstream and if these contribution are under review and waiting for their integration is not possible, a more permanent fork should be created outside of a user specific account (e.g https://github.com/InsightSoftwareConsortium)

Considering that https://github.com/CodeFinder2/cpp-base64/commit/9144cd53be930b37235ae552a92b5d2aa51e9325 is available in the upstream, I then suggest to use it.

Originally posted by @jcfr in https://github.com/InsightSoftwareConsortium/itk-wasm/pull/670#discussion_r998600613

jadh4v avatar Oct 24 '22 15:10 jadh4v

Note that CodeFinder2 is not upstream for cpp-base64, it is ReneNyffenegger.

thewtex avatar Oct 24 '22 23:10 thewtex

Good point, we should then fork the upstream into either the KitwareMedical or InsightSoftwareConsortium and use our usual naming convention for staging branch.

That said, since the decode/encode is already available in ITK through Base64.h within the itksys module, I am wondering if we should simply remove that extra dependency.

jcfr avatar Oct 24 '22 23:10 jcfr

Good point, we should then fork the upstream into either the KitwareMedical or InsightSoftwareConsortium and use our usual naming convention for staging branch.

Yes.

Fork created here: https://github.com/InsightSoftwareConsortium/cpp-base64

That said, since the decode/encode is already available in ITK through Base64.h within the itksys module, I am wondering if we should simply remove that extra dependency.

May be worth investigating.

cpp-base64 has the following features:

  • C++ interface
  • C++17 support
  • Performance optimized

That said, it would be nice to remove the dependency if it found that there are not performance regressions and the interface is still usable.

thewtex avatar Oct 25 '22 00:10 thewtex