Skip to content

ITK_DIR avoid override #134

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

thewtex
Copy link
Member

@thewtex thewtex commented Dec 19, 2020

  • BUG: Do not override ITK_DIR in CMake package configurations
  • COMP: Update JsonCpp API calls

Enable a different ITK to build built against when building the CLI,
e.g. when cross-compiling.
To address:

[3/5] Building CXX object CMakeFiles/CLPExample1Lib.dir/CLPExample1.cxx.o
In file included from /home/matt/src/itk-js/test/CLPExample1/CLPExample1.cxx:1:
/home/matt/src/itk-js/test/CLPExample1/CLPExample1.cxx: In function ‘int ModuleEntryPoint(int, char**)’:
./CLPExample1CLP.h:36:20: warning: ‘Reader’ is deprecated: Use CharReader and CharReaderBuilder instead. [-Wdeprecated-declarations]
   36 |       Json::Reader reader; \
      |                    ^~~~~~
./CLPExample1CLP.h:715:62: note: in expansion of macro ‘GENERATE_DESERIALIZATION’
  715 | #define PARSE_ARGS GENERATE_LOGO;GENERATE_XML;GENERATE_TCLAP;GENERATE_DESERIALIZATION;GENERATE_ECHOARGS;GENERATE_ProcessInformationAddressDecoding;GENERATE_SERIALIZATION;
      |                                                              ^~~~~~~~~~~~~~~~~~~~~~~~
/home/matt/src/itk-js/test/CLPExample1/CLPExample1.cxx:5:3: note: in expansion of macro ‘PARSE_ARGS’
    5 |   PARSE_ARGS;
      |   ^~~~~~~~~~
In file included from /home/matt/src/jsoncpp/include/json/json.h:11,
                 from /home/matt/src/SlicerExecutionModel/ModuleDescriptionParser/JsonSerializationUtilities.h:18,
                 from ./CLPExample1CLP.h:15,
                 from /home/matt/src/itk-js/test/CLPExample1/CLPExample1.cxx:1:
/home/matt/src/jsoncpp/include/json/reader.h:37:63: note: declared here
   37 |     "Use CharReader and CharReaderBuilder instead.") JSON_API Reader {
      |                                                               ^~~~~~
In file included from /home/matt/src/itk-js/test/CLPExample1/CLPExample1.cxx:1:
./CLPExample1CLP.h:36:20: warning: ‘Json::Reader::Reader()’ is deprecated: Use CharReader and CharReaderBuilder instead [-Wdeprecated-declarations]
   36 |       Json::Reader reader; \
      |                    ^~~~~~
./CLPExample1CLP.h:715:62: note: in expansion of macro ‘GENERATE_DESERIALIZATION’
  715 | #define PARSE_ARGS GENERATE_LOGO;GENERATE_XML;GENERATE_TCLAP;GENERATE_DESERIALIZATION;GENERATE_ECHOARGS;GENERATE_ProcessInformationAddressDecoding;GENERATE_SERIALIZATION;
      |                                                              ^~~~~~~~~~~~~~~~~~~~~~~~
/home/matt/src/itk-js/test/CLPExample1/CLPExample1.cxx:5:3: note: in expansion of macro ‘PARSE_ARGS’
    5 |   PARSE_ARGS;
      |   ^~~~~~~~~~
In file included from /home/matt/src/jsoncpp/include/json/json.h:11,
                 from /home/matt/src/SlicerExecutionModel/ModuleDescriptionParser/JsonSerializationUtilities.h:18,
                 from ./CLPExample1CLP.h:15,
                 from /home/matt/src/itk-js/test/CLPExample1/CLPExample1.cxx:1:
/home/matt/src/jsoncpp/include/json/reader.h:56:3: note: declared here
   56 |   Reader();
      |   ^~~~~~
In file included from /home/matt/src/itk-js/test/CLPExample1/CLPExample1.cxx:1:
./CLPExample1CLP.h:89:32: warning: ‘StyledStreamWriter’ is deprecated: Use StreamWriterBuilder instead [-Wdeprecated-declarations]
   89 |       Json::StyledStreamWriter writer; \
      |                                ^~~~~~
./CLPExample1CLP.h:715:148: note: in expansion of macro ‘GENERATE_SERIALIZATION’
  715 | #define PARSE_ARGS GENERATE_LOGO;GENERATE_XML;GENERATE_TCLAP;GENERATE_DESERIALIZATION;GENERATE_ECHOARGS;GENERATE_ProcessInformationAddressDecoding;GENERATE_SERIALIZATION;
      |                                                                                                                                                    ^~~~~~~~~~~~~~~~~~~~~~
/home/matt/src/itk-js/test/CLPExample1/CLPExample1.cxx:5:3: note: in expansion of macro ‘PARSE_ARGS’
    5 |   PARSE_ARGS;
      |   ^~~~~~~~~~
In file included from /home/matt/src/jsoncpp/include/json/json.h:13,
                 from /home/matt/src/SlicerExecutionModel/ModuleDescriptionParser/JsonSerializationUtilities.h:18,
                 from ./CLPExample1CLP.h:15,
                 from /home/matt/src/itk-js/test/CLPExample1/CLPExample1.cxx:1:
/home/matt/src/jsoncpp/include/json/writer.h:298:5: note: declared here
  298 |     StyledStreamWriter {
      |     ^~~~~~~~~~~~~~~~~~
@thewtex thewtex changed the title itk dir avoid override ITK_DIR avoid override Dec 19, 2020
Copy link
Member

@jcfr jcfr left a comment

Choose a reason for hiding this comment

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

Since ensuring the developer have the same ITK build tree is important is the general case, should we test for CMAKE_CROSSCOMPILING variable ?

@thewtex
Copy link
Member Author

thewtex commented Jan 11, 2021

This is not only useful for cross-compiling -- in general different ITK version or build configuration may be desired for the generated CLI.

@hjmjohnson
Copy link
Member

@jcfr Any other comments on this? Is it OK as is?

@jcfr
Copy link
Member

jcfr commented Aug 13, 2021

Thanks for the feedback. After further consideration, the proposed changes make sens.

@jcfr jcfr removed the request for review from johan-andruejol August 13, 2021 18:49
Copy link
Member

@jcfr jcfr left a comment

Choose a reason for hiding this comment

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

Let's address #136 before merging

@hjmjohnson
Copy link
Member

@jcfr #136 is addressed, and this seems to have approval.

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

Successfully merging this pull request may close these issues.

3 participants