Skip to content

Conversation

@N-Dekker
Copy link
Collaborator

@N-Dekker N-Dekker commented Sep 8, 2020

No description provided.

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.

LGTM. Let's see CI results.

@N-Dekker
Copy link
Collaborator Author

N-Dekker commented Sep 8, 2020

LGTM. Let's see CI results.

Thanks Dženan. Apparently there's still an OpenCL issue in SuperElastix/elastix to be fixed:

_deps/elx-src/Common/OpenCL/ITKimprovements/itkOpenCLLogger.h:72:23: error: ‘PriorityLevelType’ has not been declared

https://github.com/InsightSoftwareConsortium/ITKElastix/runs/1086581143#step:4:1976

I'll have a look!

@N-Dekker N-Dekker force-pushed the Switch-to-SuperElastix-elastix branch 2 times, most recently from c8e042b to 4bd46e4 Compare September 9, 2020 08:17
@N-Dekker
Copy link
Collaborator Author

N-Dekker commented Sep 9, 2020

@dzenanz @thewtex My pull request still appears to have some link errors. Which seems to be related to the fact that the SuperElastix/elastix project now distinguished between two CMake targets: elastix_exe and elastix_lib (as I committed with SuperElastix/elastix@3944a13 ). When an application wants to link to the elastix library, it must now specify "elastix_lib", instead of just "elastix".

Now it appears that "${Elastix-Test_LIBRARIES}" should be adjusted to specify "elastix_lib" instead of just "elastix", for this CreateTestDriver call:

CreateTestDriver(Elastix "${Elastix-Test_LIBRARIES}" "${ElastixTests}")

Can you please tell me how to properly adjust "${Elastix-Test_LIBRARIES}"? I think it must evaluate to "elastix_lib;transformix_lib;ITKMetaIO;ITKTestKernel"

@dzenanz
Copy link
Member

dzenanz commented Sep 9, 2020

Since this is set, this might be sufficient:

list(APPEND Elastix-Test_LIBRARIES elastix_lib transformix_lib)
CreateTestDriver(Elastix "${Elastix-Test_LIBRARIES}" "${ElastixTests}" )

I looked around the code, and don't see how elastix.lib used to propagate to the executable. So the above suggestion is more of a guess.

@N-Dekker N-Dekker force-pushed the Switch-to-SuperElastix-elastix branch from 4bd46e4 to ff8683f Compare September 10, 2020 11:27
@N-Dekker
Copy link
Collaborator Author

@dzenanz Thanks for looking into this issue regarding those link errors. I think it is fixed now, by doing set(Elastix_LIBRARIES elastix_lib transformix_lib) in the root-level CMakeLists.

I'm currently looking into

/work/_skbuild/linux-x86_64-3.5/cmake-build/_deps/elx-src/Core/Kernel/elxElastixMain.h:33:10: fatal error: 'itkOpenCLContext.h' file not found
https://github.com/InsightSoftwareConsortium/ITKElastix/pull/58/checks?check_run_id=1096122268#step:4:2894

Please let me know if you have a clue! Note that 'itkOpenCLContext.h' is in Elastix, at https://github.com/SuperElastix/elastix/blob/develop/Common/OpenCL/ITKimprovements/itkOpenCLContext.h

@dzenanz
Copy link
Member

dzenanz commented Sep 10, 2020

Clue: OpenCL/ITKimprovements needs to be added to include directories, somewhere 😄

The above is somewhat of a joke, I am sure you can deduce that yourself. I looked a bit and don't have useful suggestions 😞

@N-Dekker N-Dekker force-pushed the Switch-to-SuperElastix-elastix branch from ff8683f to 874180f Compare September 11, 2020 12:15
N-Dekker added a commit to SuperElastix/elastix that referenced this pull request Sep 11, 2020
Moved the `#include "itkOpenCLContext.h"` from elxElastixMain.h to elxElastixMain.cxx and elxTransformixMain.cxx, hoping to ease fixing the error:

> /work/_skbuild/linux-x86_64-3.5/cmake-build/_deps/elx-src/Core/Kernel/elxElastixMain.h:33:10: fatal error: 'itkOpenCLContext.h' file not found

at https://github.com/InsightSoftwareConsortium/ITKElastix/pull/58/checks?check_run_id=1096122268#step:4:2894

(Pull request InsightSoftwareConsortium/ITKElastix#58 "ENH: Switch to latest version of SuperElastix/elastix (using ITK 5.1.1)")
N-Dekker added a commit to SuperElastix/elastix that referenced this pull request Sep 11, 2020
Moved the `#include "itkOpenCLContext.h"` from elxElastixMain.h to elxElastixMain.cxx and elxTransformixMain.cxx, hoping to ease fixing the error:

> /work/_skbuild/linux-x86_64-3.5/cmake-build/_deps/elx-src/Core/Kernel/elxElastixMain.h:33:10: fatal error: 'itkOpenCLContext.h' file not found

at https://github.com/InsightSoftwareConsortium/ITKElastix/pull/58/checks?check_run_id=1096122268#step:4:2894

(Pull request InsightSoftwareConsortium/ITKElastix#58 "ENH: Switch to latest version of SuperElastix/elastix (using ITK 5.1.1)")
@N-Dekker N-Dekker force-pushed the Switch-to-SuperElastix-elastix branch from 874180f to 58b0de1 Compare September 11, 2020 13:15
Removed obsolete `elastix::BaseComponent::InitializeElastixLibrary()`
calls, as Elastix is now initialized as a library by default:
"ENH: Replace InitializeElastixLibrary() by InitializeElastixExecutable()"
SuperElastix/elastix#248
SuperElastix/elastix@ef64fb2

Adjusted CMake `Elastix_LIBRARIES`, as the Elastix library CMake targets
are now named elastix_lib and transformix_lib:
"ENH: Allow building exe and lib of elastix and transformix together"
SuperElastix/elastix#232
SuperElastix/elastix@3944a13
@N-Dekker N-Dekker force-pushed the Switch-to-SuperElastix-elastix branch from 58b0de1 to 55c953e Compare September 11, 2020 16:47
@dzenanz dzenanz merged commit 55c953e into InsightSoftwareConsortium:master Sep 11, 2020
@N-Dekker
Copy link
Collaborator Author

@dzenanz Thanks but why did you merge already? It was still in draft status, and it still has failures from build-test-cxx (ubuntu-18.04)!

@dzenanz
Copy link
Member

dzenanz commented Sep 11, 2020

I accidentally pushed to all repositories instead of pulling from all repositories.

@dzenanz
Copy link
Member

dzenanz commented Sep 11, 2020

I was searching for this PR right now!

@dzenanz
Copy link
Member

dzenanz commented Sep 11, 2020

I think you will have to create a new PR. Reusing the same branch in your fork is fine. Sorry for this 😢

@N-Dekker
Copy link
Collaborator Author

Then what is the current status? Is it merged now? I don't see it being merged at https://github.com/InsightSoftwareConsortium/ITKElastix/commits/master

@dzenanz
Copy link
Member

dzenanz commented Sep 11, 2020

I quickly realized my mistake, and put master branch to point to the old commit. But it had a side-effect of closing this PR.

@dzenanz
Copy link
Member

dzenanz commented Sep 11, 2020

Please reference this PR in the new PR, in order to keep discussion history.

@N-Dekker
Copy link
Collaborator Author

I quickly realized my mistake, and put master branch to point to the old commit. But it had a side-effect of closing this PR.

So it's a GitHub bug that it doesn't allow reopening the PR! 😸

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants