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

Change travis config to test both cmake and autoconf #27

Merged
merged 4 commits into from
Mar 30, 2016

Conversation

jd-boyd
Copy link
Contributor

@jd-boyd jd-boyd commented Mar 23, 2016

Certainly #25 should go in first, but I'm starting this pull request so that I can see the result of the travis run.

@jd-boyd
Copy link
Contributor Author

jd-boyd commented Mar 23, 2016

OK, this works for cmake and autotools on linux, but the cmake builds on OSX fail. I see that #26 was already opened for this. I guess working on that may be where I go next.

@jd-boyd
Copy link
Contributor Author

jd-boyd commented Mar 23, 2016

If #29 is accepted, and this is rebased on top, then it should have completely passing tests. I'll rebase it on master if/once #29 is merged.

@brarcher
Copy link
Contributor

If you do not have an entry in the AUTHORS file yet, can you include a change to this pull request adding yourself?

- ./configure
- mkdir build
- cd build
- ../scripts/run_config.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

A small thing, but as the number of lines necessary to build is small, could it be included directly in the .travis.yml file instead of as a separate script?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can do that.

At:
https://groups.google.com/forum/#!topic/travis-ci/uaAP9zEdiCg
one of the travisci founders says:

The recommended way to do this is to pull out the commands into a shell script and have that be run from your .travis.yml instead.

However, it wasn't quite as ugly to inline as I expected, lets see if it passes.

I'm still planning to rebase/squash after #29 is merged. If it isn't merged, then more changes will be required to omit cmake testing on osx.

@jd-boyd jd-boyd force-pushed the test_cmake branch 2 times, most recently from f673f00 to 785bb5c Compare March 24, 2016 03:04
@jd-boyd
Copy link
Contributor Author

jd-boyd commented Mar 24, 2016

If you do not have an entry in the AUTHORS file yet, can you include a change to this pull request adding yourself?

Will do.

Contributors:
Cesar Ballardini (signals)
Anthony G. Basile (fix configure.ac and strsignal())
Friedrich Beckmann (mingw and msvc port #1)
Frank Bergmann (WIN32 tmpfile workaround)
Joshua Boyd (travis testing with both build systems, cmake fixes)
Copy link
Contributor

Choose a reason for hiding this comment

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

It is a real nit-pick, but could the () be lined up with the others?

@brarcher
Copy link
Contributor

The CMake change for OSX has been merged in. Feel free to update this request.

@brarcher
Copy link
Contributor

Jenkins: ok to test

@brarcher
Copy link
Contributor

Looking at the build results on Travis, CMake now does work for OSX. The configuration for Travis, however, does not run the unit tests when using CMake. It turns out that for CMake the test target is currently 'test' instead of 'check'. I think that this is a convention of CMake.

If you could, would you mind in your change running 'make check' for autotools and 'make test' for CMake?

@jd-boyd jd-boyd force-pushed the test_cmake branch 2 times, most recently from 597f46f to 38b34d8 Compare March 27, 2016 07:26
@jd-boyd
Copy link
Contributor Author

jd-boyd commented Mar 27, 2016

If you could, would you mind in your change running 'make check' for autotools and 'make test' for CMake?

I've pushed a commit with that change and alas it looks like there is more work to do to get CMake full up to speed.

@brarcher
Copy link
Contributor

The AppVeyor failures are my fault, as I am working on getting it working in another branch. They can be ignored.

The Travis-CI failures for CMake show that CMake does not let unit tests run on Linux or OSX. However, Jenkins did show CMake was successful on Linux. The difference is that the Travis-CI build attempted to build in another directory instead of directly in the source tree. Further evidence is the following from the Travis-CI build, showing that the unit testing script cannot find a binary which was built:

test_output.sh: 1: test_output.sh: ./ex_output: not found

The autotools setup does allow building out of tree, but it appears that CMake does not quite yet. If the CMake changes did not build in a separate build directory, they would probably be successful. If that is sufficient for now, you could make that adjustment a separate defect can be created for letting CMake build out of tree, if it is important.

@jd-boyd
Copy link
Contributor Author

jd-boyd commented Mar 30, 2016

The autotools setup does allow building out of tree, but it appears that CMake does not quite yet.

CMake strongly recommends building out of tree. I think the problem is in how CMake is invoking the test shell scripts and building test_vars, but I haven't completely gotten it worked out yet.

Also, I noticed that the check_check_export test was never added to CMake, so one more todo item.

Keep make check in autoconf builds.
test_vars was being built in tree, and while the test programs were built out of tree, the CMakeLists.txt led to trying to run them in tree.
@jd-boyd
Copy link
Contributor Author

jd-boyd commented Mar 30, 2016

The tests (minus the still not added check_check_export test) pass locally, so lets see what Travis makes of them.

@brarcher brarcher merged commit 82cdb29 into libcheck:master Mar 30, 2016
@brarcher
Copy link
Contributor

Nice! Thanks for helping dig into this.

@jd-boyd jd-boyd deleted the test_cmake branch March 31, 2016 03:29
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