Fixes BUILDINSOURCE detection

[remram44]

Fixes #713

Amends #736; paths are not regexps.

Re-applies #791 wrongly merged and reverted by #797; please test BEFORE merging.

(low priority)

[aashish24]

thanks @remram44. @dlonie let's not merge it until we tag 2.0 and put the binaries out.

[remram44]

Note: apparently this introduced a bug for Brian Smith (who's not on Github)

[alliepiper]

No problem @aashish24 -- since you pinged me on it this morning, I thought it was needed.

As for testing, the travis CI builds had passed.

[aashish24]

@dlonie no problem. The travis might not check for all possible conditions. When you get a chance, please look at the code.

[alliepiper]

Well, we know it doesn't work for at least one case. @remram44 can you fix Brian's issue, test the implementation a bit more before pushing, and update this branch?

I was going to ask on the other pull request, but figured this patch was needed at the time...Since that's not the case, is this change actually necessary? True, paths are not regular expressions, but they are strings, which regex's are designed to test/analyze...Is there a non-pathological case where the current implementation fails?

[remram44]

The problem is not the right-hand-side of the STRING(REGEX MATCH ...) test, it's the left-hand-side...

Brian's issue is not known to me.

[alliepiper]

Bah, the thread with his output is yet another discussion that wasn't on the mailing list...We really should be using the list to discuss these things so the relevant people stay notified. Here's the relevant bit:

bs1mbpro:build bs1$ /usr/local/CMake.app/Contents/bin/cmake ../source -DGIT_PROTOCOL=http://
-- The C compiler identification is AppleClang 5.1.0.5030040
-- The CXX compiler identification is AppleClang 5.1.0.5030040
-- Check for working C compiler: /usr/bin/cc
-- Check for working C compiler: /usr/bin/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++
-- Check for working CXX compiler: /usr/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
CMake Error at CMakeLists.txt:13 (STRING):
  string end index: 31 is out of range -1 - 30

At a glance, it looks like an off by one error in the substring call.

Can you be more specific about the regex error? The left hand side "^${cdat_SOURCE_DIR}/" produces a regex that will check that the binary directory does not start with the same string as the source directory, just like your strlen/substring comparison. I'm not seeing the advantage...?

[aashish24]

Sorry here it is:

bs1mbpro:build bs1$ /usr/local/CMake.app/Contents/bin/cmake ../source -DGIT_PROTOCOL=http://
-- The C compiler identification is AppleClang 5.1.0.5030040
-- The CXX compiler identification is AppleClang 5.1.0.5030040
-- Check for working C compiler: /usr/bin/cc
-- Check for working C compiler: /usr/bin/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++
-- Check for working CXX compiler: /usr/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
CMake Error at CMakeLists.txt:13 (STRING):
string end index: 31 is out of range -1 - 30

[INFO] We reset your path to: /Users/bs1/uvcdat-devel/build/install/Externals/bin:/Users/bs1/uvcdat-devel/build/install/bin:/Users/bs1/uvcdat-devel/build/install/Externals/bin:/Users/bs1/uvcdat-devel/build/install/Library/Frameworks/Python.framework/Versions/2.7/bin:/opt/local/bin:/opt/local/sbin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/bin:/opt/X11/bin:/usr/local/git/bin:/usr/local/munki:/usr/texbin
-- Looking for a Fortran compiler
-- Looking for a Fortran compiler - /usr/local/bin/gfortran
-- Found Git: /usr/bin/git (found version "1.8.5.2 (Apple Git-48)")
-- The Fortran compiler identification is GNU
-- Check for working Fortran compiler: /usr/local/bin/gfortran
-- Check for working Fortran compiler: /usr/local/bin/gfortran -- works
-- Detecting Fortran compiler ABI info
-- Detecting Fortran compiler ABI info - done

[remram44]

The left hand side "^${cdat_SOURCE_DIR}/" produces a regex that will check that the binary directory does not start with the same string as the source directory

That's assuming nothing in ${cdat_SOURCE_DIR} is any kind of special character. I don't see how taking a filename for a regex is a good idea.

I have no idea how SUBSTRING counts characters, the CMake documentation (and language...) is not exactly workable. I'll test this some more when I get the chance.

[remram44]

Alright got it, tests are over there: https://gist.github.com/remram44/615b7e19f1ff2a724c1a (CMake has no support for unit-testing!?)

[aashish24]

nice. cmake it self is not a replacement for junit or cppunit or things like that. It is at a higher level than the unit testing or regression testing framework. It collects information from these testing frameworks and post them on the web-server.

[remram44]

I mean testing CMake code itself.

[aashish24]

@dlonie can you look into this?

[alliepiper]

Into testing CMake code?

http://www.cmake.org/cmake/help/v2.8.12/cmake.html#opt:-Pfile

add_test([test name] "${CMAKE_COMMAND}" -P "/path/to/test_script.cmake")

Add the cmake code you want to test to test_script.cmake.

[aashish24]

@remram44 can you add test for it based on what @dlonie 's suggestion?

[remram44]

Probably not... it's not worth it as failures would be easy to spot (and work around).

[aashish24]

@remram44 I would suggest we add test even as for some it may not be as easy plus in the future if somone make any changes, then atleast we can verify that we have not broken the code.

[remram44]

You are welcome to add this, or at least to set up a test suite infrastructure in the CMake code. I won't pursue this myself.

[alliepiper]

Just an FYI: See my last comment for how to unit test cmake code. The infrastructure exists.

[remram44]

The CTest suite tests the build tree, not the source. There would need to be a system to copy the CMake test files over, with the files they test, and then register them with CTest. I'm not writing that for a single piece of code that can only fail in harmless ways.

[alliepiper]

Not sure I see where the difficulty is...All it would take is one line, file(COPY ...), followed by a call to add_test(...) as mentioned above.

But I'm not going to twist your arm on this. If it's not worth it to you as the author of the patch, it's really not worth it to me. shrugs

[aashish24]

I agree and disagree. Obviously the old code failed that bothered a developer enough to push a patch like this. Yes, it was a harmless error but it could annoy someone right? I will look at the code morefully before I am comfortable merging it (in case you don't want to add testing for it). Now that will add extra work for me because now I have to think of all possible scenarios which otherwise could be tested by a test. But I understand that adding a test is some work and developers often don't like that.

[remram44]

I will look at the code morefully before I am comfortable merging it (in case you don't want to add testing for it). Now that will add extra work for me because now I have to think of all possible scenarios which otherwise could be tested by a test.

I posted a test case 16 days ago, here it is again: https://gist.github.com/remram44/615b7e19f1ff2a724c1a

Again, you're welcome to add it to CTest if you feel this is useful.

[remram44]

This is not in.

[aashish24]

Unless we have some tests for this, we won't be able to merge this in. A previous change broke the build for few folks and now we have to make sure that we do some testing for it.

[remram44]

Reopening as discussed; removed "gatekeeper" label since CMake tests are to be added before merging.

[doutriaux1]

@aashish24 where is kitware at with this?

[alliepiper]

It's on my list. I'm out next week for the holiday, should have something afterwards.

[doutriaux1]

doh!

[aashish24]

@dlonie can we add a simple test and get this one merged as well?

[alliepiper]

This is next on my list. Should get to it soon.

[alliepiper]

Issued new PR (can't update this one as it is in a fork, so I pushed a new branch to the UVCDAT repo). New PR is #973.