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

Split tutorials, examples, and tests into separate targets #644

Merged
merged 3 commits into from
Mar 29, 2016

Conversation

mkoval
Copy link
Collaborator

@mkoval mkoval commented Mar 24, 2016

This pull request replaces the DART_BUILD_UNITTESTS, DART_BUILD_EXAMPLES and DART_BUILD_TUTORIALS CMake flags the tests, examples, and tutorials targets. These features are no longer built by default unless the user explicitly passes the corresponding target(s) to make.

The key advantage of this approach is that targets are specified at build time, whereas CMake flags are specified at configure time. These features are optional and take a long time to build, so I prefer having them disabled by default. It's currently awkward to do this because changing these settings requires re-running CMake with new -D flags and, possibly, deleting your CMakeCache.txt file.

With the new paradigm, this is as simple as running make during development and passing the tests, examples, or tutorials target when you want to try re-building that code. Note that you can still run make all tests examples tutorials to build everything at once.

I am not sure what @jslee02 and @mxgrey think of this change, so this pull request is mostly intended to start a discussion.

@jslee02
Copy link
Member

jslee02 commented Mar 24, 2016

I like this change.

One minor concern is the inconsistency of the name of apps and examples. We've used apps for historical reason, but I would prefer examples over apps. app (or application) sounds more like a complete software while our apps are for demonstration of the features of DART.

@@ -1,7 +1,7 @@
make
if [ $COVERALLS = ON ]; then make coveralls; fi
sudo ldconfig --verbose # So the test executeables can detect libtinyxml2
if [ $BUILD_CORE_ONLY = OFF ]; then make test; fi
if [ $BUILD_CORE_ONLY = OFF ]; then make all test tests; fi
Copy link
Member

Choose a reason for hiding this comment

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

question: I thought test target is to run the tests that are already built. So the order of targets shouldn't be like make all tests test? Moreover, I'm not sure why all needs to be here. The above make wouldn't run for all target?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right about the order: it should be make all test tests. This was a typo.

All does need to be here. make will invoke the first target in the Makefile, which is typically all, if no goals are specified on the command line:

By default, the goal is the first target in the makefile (not counting targets that start with a period). Therefore, makefiles are usually written so that the first target is for compiling the entire program or programs they describe. If the first rule in the makefile has several targets, only the first target in the rule becomes the default goal, not the whole list. You can manage the selection of the default goal from within your makefile using the .DEFAULT_GOAL variable (see Other Special Variables). (Source)

Since we are specifying a goal on the command line, we need to manually pass all. In practice, we may not need to do this because the tests target may indirectly depend on all of the targets included in all.

This raises another important question: should we also build tutorials and examples here, to verify that they build successfully?

Copy link
Member

Choose a reason for hiding this comment

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

This raises another important question: should we also build tutorials and examples here, to verify that they build successfully?

I would prefer to do complete build test in travis unless it exceeds the build time limit.

@mkoval
Copy link
Collaborator Author

mkoval commented Mar 24, 2016

I also like the name examples more than apps. I would prefer to make that change in a separate pull request, though, since it will be a relatively large diff.

@mkoval
Copy link
Collaborator Author

mkoval commented Mar 24, 2016

Looking at the Travis logs, it appears that I am not handling the BUILD_CORE_ONLY flag correctly. I'll try to fix that tomorrow.

@jslee02
Copy link
Member

jslee02 commented Mar 24, 2016

I would prefer to make that change in a separate pull request, though, since it will be a relatively large diff.

👍

@mkoval
Copy link
Collaborator Author

mkoval commented Mar 27, 2016

I fixed support for BUILD_CORE_ONLY and modified the osgDart tests and examples to follow the same paradigm. I don't plan to make any more changes to this pull request unless someone has a request.

I updated the Travis build script. It seems to be working correctly, except for the BUILD_CORE_ONLY=OFF builds on OS X are failing the testVskParser test. The executable is crashing, so I suspect that this is a compiler issue that is unrelated to my changes.

We also need to update the Appveyor build to build the all and tests targets. I'm not very familiar with CMake's MSVC++, so I'm not sure how to specify this in appveyor.yml. The build is also currently crashing with a "fatal error C1060: compiler is out of heap space" error.

@jslee02
Copy link
Member

jslee02 commented Mar 27, 2016

We also need to update the Appveyor build to build the all and tests targets.

We can specify target if use cmake command to build like this. I'll make the change once this is merged.

The build is also currently crashing with a "fatal error C1060: compiler is out of heap space" error.

I'll look into that on my Windows machine.

@jslee02 jslee02 added this to the DART 6.0.0 milestone Mar 27, 2016
@jslee02 jslee02 merged commit 11a9d9e into dartsim:master Mar 29, 2016
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