-
Notifications
You must be signed in to change notification settings - Fork 22
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
Add median calculation (and cfitsio) to automated regression tests #57
Conversation
…run from build directory
scripts/bamboo/build_and_test.sh
Outdated
@@ -28,7 +28,7 @@ export BUILD_DIR=build-${SYS_TYPE} | |||
|
|||
export COMPILER=${1:-gcc_4_8_5} | |||
export BUILD_TYPE=${2:-Release} | |||
export BUILD_OPTIONS="-DENABLE_STATS=On -DENABLE_CFITS=Off -DENABLE_FITS_TESTS=Off ${BUILD_OPTIONS}" | |||
export BUILD_OPTIONS="-DENABLE_STATS=On -DENABLE_CFITS=On -DENABLE_FITS_TESTS=On -DCFITS_LIBRARY_PATH=/g/g0/martymcf/.bin/toss_3_x86_64/lib -DCFITS_INCLUDE_PATH=/g/g0/martymcf/.bin/toss_3_x86_64/include ${BUILD_OPTIONS}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hard coded library path won't work in general.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree. This particular script is for our TOSS-3 CI machine. By design (not mine), the script needs to run as me and the CFITS files needed to be installed in my home directory. I could add these files to an environment variable, but the path would still be specific.
I need to think about this some more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have put a couple of minor comments in line. The main one is about the hard coded path to your directory.
Thanks @mayagokhale, I will add the comments and will figure out what the best thing is to do about the CI script. |
I wasn’t sure what would happen if someone else ran the CI script (like Eric). Could the file be put in the common dst workspace? It wouldn’t work outside LC, but at least other people could run it.
… On Oct 11, 2018, at 5:26 PM, Marty McFadden ***@***.***> wrote:
Thanks Maya, I will add the comments and will figure out what the best thing is to do about the CI script.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
This particular script is not meant for humans to run. It is meant for bamboo (our CI) to run. We have a different script under tests/regression_tests that humans can run. I think I will move this script to a 'bamboo' sub-directory and add a comment. I also think I can move the path definitions up into the bamboo definition and just reference that from this particular script. I'll also add a comment to this script to direct folks to where the human usable scripts are. |
Moved build platform environment specific data up to the bamboo project plan definition (ie. specification of location of dependent build files like the CFITS library installation location)
Hi @mayagokhale, I believe I have addressed the concerns. Please let me know and I will perform the merge operation (I will squash the several testing commits that were done into a single commit). Thank you very much for your review! |
I have reviewed and approved the changes. It’s fine to merge.
Maya
… On Oct 12, 2018, at 9:03 AM, Marty McFadden ***@***.***> wrote:
Hi @mayagokhale,
I believe I have addressed the concerns. Please let me know and I will perform the merge operation (I will squash the several testing commits that were done into a single commit).
Thank you very much for your review!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
This small set of changes will allow us to build with the cfitsio library on multiple systems including our TOSS3 CI system. This automated CI scripts will now also run our median calculation test as part of our standard regression tests.
I also fixed a bug with the logging demon not being launched when a user tries to run umap programs from the build directory instead of the install directory.