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

Source the MINC tools path for brainbrowser #1843

Merged

Conversation

MounaSafiHarab
Copy link
Contributor

@MounaSafiHarab MounaSafiHarab commented May 25, 2016

This gets the MINC tools path from the config.xml file.
CHANGES REQUIRED TO THE CONFIG.XML FILE for existing projects:
Add the following lines (where /opt/minc/ is where the MINC tools are installed in the example below; replace with the appropriate path):

< MINCToolsPath >/opt/minc/< /MINCToolsPath >

Related to Loris-MRI Pull request 125:
aces/Loris-MRI#125

@MounaSafiHarab MounaSafiHarab added Release: Add to release notes PR whose changes should be highlighted in the release notes Release: Breaking changes PR that contains changes that might impact the code or accepted practices of active projects labels May 25, 2016
@MounaSafiHarab MounaSafiHarab added this to the 16.0 milestone May 25, 2016
@driusan
Copy link
Collaborator

driusan commented May 25, 2016

@MounaSafiHarab Can you add the MINCToolsPath to the sample config.xml used at install time?

@MounaSafiHarab
Copy link
Contributor Author

@driusan:

I did, and I updated the installation script.

@driusan
Copy link
Collaborator

driusan commented May 26, 2016

I'm a little afraid that the way it's trying to determine the minc path using which is too clever and will come back to bite us later, but it looks like it should work. Also, shouldn't it check that the MINC_TOOLKIT_DIR isn't empty and/or check the error code before doing the replace in the config.xml? Otherwise, it'll work unexpectedly if the minc toolkit isn't installed.

Other than that, it looks good to me, but should probably have @Jkat sign off on it too.

@MounaSafiHarab
Copy link
Contributor Author

@driusan @Jkat
I can check if the minc tools are installed first (we do this in the Loris-MRI installation), but the problem is the assumption that MINC tools are installed at this point which is not true given that we are installing Loris not Loris-MRI.

The other problem is this error by Travis:
"ConfigurationException: Config setting MINCToolsPath does not exist in database in /home/travis/build/aces/Loris/php/libraries/NDB_Config.class.inc on line 288"
I thought we can either put in config.xml OR the database?

@driusan
Copy link
Collaborator

driusan commented May 26, 2016

That's why I suggest checking either the error code returned by your which command (which I expect should be non-zero if there's an error), or if the variable is empty (since I'd expect it would have no output to stdout but send an error message to stderr if it's not found.)

@driusan
Copy link
Collaborator

driusan commented May 26, 2016

Travis uses the config.xml in test/config.xml

- cp test/config.xml project/config.xml

… check from the install; this has to be added in Loris-MRI install
@MounaSafiHarab
Copy link
Contributor Author

@driusan @Jkat

so now the Loris-MRI pull request (aces/Loris-MRI#125) takes care of checking the minc tools and whether they are installed and where, and then update the config.xml

@samirdas samirdas merged commit c7d21ea into aces:16.04-dev May 27, 2016
@christinerogers
Copy link
Contributor

Update: since Loris-MRI can't assume access to config.xml, an instruction to manually add the config tagset has been added to the Loris-MRI Readme: see aces/Loris-MRI#123

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release: Add to release notes PR whose changes should be highlighted in the release notes Release: Breaking changes PR that contains changes that might impact the code or accepted practices of active projects
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants