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.

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

Update ACE package #145

Merged
merged 6 commits into from
Oct 4, 2018
Merged

Update ACE package #145

merged 6 commits into from
Oct 4, 2018

Conversation

paskino
Copy link
Contributor

@paskino paskino commented Aug 24, 2018

Reflects updates done on ACE.
Uses Gadgetron version that compiles with GPU support.

@paskino paskino requested a review from KrisThielemans August 24, 2018 16:46
Copy link
Member

@KrisThielemans KrisThielemans left a comment

Choose a reason for hiding this comment

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

seems good to me. Feel free to merge, but send email that you're updating the Gadgetron version.

but why the branch name?

@paskino
Copy link
Contributor Author

paskino commented Oct 3, 2018

SCARF is the STFC cluster, that's the origin of the name of this PR.

About the Gadgetron repo change:

  1. the Gadgetron version we currently have in master is from 24/11/2017.
  2. Gadgetron wouldn't build with GPU support. I patched it (the Gadgetron version we have in the Superbuild) and submitted a PR to Gadgetron.
  3. Gadgetron merged my PR on 28/7/2018
  4. So this patch is needed to have GPU support. It is linked to the same 24/11/2017 gadgetron version as before, but the repository is the CCPPETMR/gadgetron fork.

@KrisThielemans
Copy link
Member

I'm somewhat reluctant to point to our own gadgetron fork, as it gives the impression we're deviating from them. I vote for pointing to the main gadgetron repo somewhere after acceptance of your PR. @DANAJK , @ckolbPTB @johannesmayer any suggestions/opinions?

@DANAJK
Copy link
Contributor

DANAJK commented Oct 3, 2018

Similar to #134 , I have no objection to pointing to a Gadgetron commit that works for us. Hopefully more recent that one year though.

@paskino
Copy link
Contributor Author

paskino commented Oct 3, 2018

I tried to build Gadgetron after the PR was merged and it fails.
I had a first error coming from the CXX flag -Werror=terminate which isn't recognised.

Fixed that (requires changing Gadgetron CMakeLists.txt) and I got to

/home/sirfuser/devel/buildgadgetron/sources/Gadgetron/toolboxes/mri_core/mri_core_data.h:5:30: fatal error: ismrmrd/waveform.h: No such file or directory
compilation terminated.

@DANAJK
Copy link
Contributor

DANAJK commented Oct 3, 2018

waveforms are something that is being added to ISMRMRD (to support ECG information as an example).
Maybe you need to update ISMRMRD too?

@KrisThielemans
Copy link
Member

we should follow the same strategy for ISMRMRD as in #134 I think: occasional updates, with testing first. if we need ECG (or any other schanges in ISMRMRD) now or not I have no idea. As long as it doesn't break things, it's fine for me of course.

@KrisThielemans
Copy link
Member

ah sorry. I hadn't seen Edo's error. Seems Gadgetron doesn't check the ISMRMRD version properly then...

Then it needs updating ISMRMRD of course.

@paskino
Copy link
Contributor Author

paskino commented Oct 4, 2018

I'll see if I find a couple of Gadgetron/ISMRMRD which work for us.
The -Werror=terminate problem might mean that there is a minimum requirement on gcc version, and the one we have isn't sufficient.

@KrisThielemans
Copy link
Member

KrisThielemans commented Oct 4, 2018

I guess do a git blame to find out when they introduced that option. Not nice if it isn't guarded by appropriate checks on compiler.

@paskino
Copy link
Contributor Author

paskino commented Oct 4, 2018

OK, the new Gadgetron/master requires at least GCC version 6.

Ubuntu 16.04 has 5.4. On the VM it could be possible to install gcc version 6 following this link

I see a few options:

  1. Build a new VM with Ubuntu 18.04 which ships GCC 7.3.0
  2. Add the new unofficial repository to get GCC 6 onto the current VM (Ubuntu 16.04)
  3. Use conda
  4. Keep outdated Gadgetron in the short term.

Although option 3 is still in a work-in-progress phase I strongly believe that we should pursue that direction.

@paskino
Copy link
Contributor Author

paskino commented Oct 4, 2018

They introduced a check of the availability of -Werror=terminate (which I incidentally couldn't find documented anywhere) in the latest Gadgetron master branch. But along with that, they check minimum version of GCC >= 6.

@KrisThielemans
Copy link
Member

This discussion isn't really about this PR. I've created a new Issue for it: #146

This PR does 2 things: change ACE stuff, and change Gadgetron Tag. Those are independent, and the Gadgetron stuff is preventing us merging the ACE changes.

@paskino, please revert Gadgetron changes, change the title of the PR, and then squash-merge.

@paskino paskino changed the title Scarf Update ACE package Oct 4, 2018
@paskino paskino merged commit fc6a9c1 into master Oct 4, 2018
@paskino paskino deleted the scarf branch October 4, 2018 14:08
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.

3 participants