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

Using a common Catch main object for development. #276

Merged
merged 7 commits into from
Jun 1, 2019

Conversation

arcuru
Copy link
Contributor

@arcuru arcuru commented Apr 21, 2019

This needed a few small changes from my hacked together test. Mostly I hit an issue where I couldn't create a catchlib object in every exercise during a full CMake build, so I had to switch how I did the individual exercise CMakeLists.txt files.

I added some comments into the students' CMakeLists files, but they may need to be phrased better to be clearer to them. I wanted to explain what it was instead of just confusing the student if they looked at it.

Closes #272

@arcuru arcuru requested a review from KevinWMatthews April 21, 2019 15:17
@KevinWMatthews
Copy link
Contributor

Ah, yes - you would need to name each library uniquely. Normally that's no issue, but in our case that is much harder to maintain.

I think that we're correct to omit special compiler flags or definitions from catchlib: -Wall -Wextra -Wpedantic -Werror or _SCL_SECURE_NO_WARNINGS. Those are useful in student code but not for this.

Looks pretty good at a read. Hope to try this in detail this evening.

@KevinWMatthews
Copy link
Contributor

KevinWMatthews commented Apr 24, 2019

@patricksjackson I'm getting failures when I build individual exercises in isolation (using only the exercise-level CMakeLists.txt, not using the root-level CMakeLists.txt).

Can you reproduce this:

CMake Error at CMakeLists.txt:26 (add_executable):
  Error evaluating generator expression:

    $<TARGET_OBJECTS:catchlib>

  Objects of target "catchlib" referenced but no such target exists.

using something like:

$ cd exercises/beer-song/
$ cmake .

@KevinWMatthews
Copy link
Contributor

KevinWMatthews commented Apr 24, 2019

I think it's a quirk of CMake conditionals. When building from the root level, EXERCISM_COMMON_CATCH exists and is Boolean. When building at the exercise level, EXERCISM_COMMON_CATCH does not exist at all.

Reading through CMake's documentation on if() and variable references but haven't gotten to the bottom of it yet.

@KevinWMatthews
Copy link
Contributor

Short story - got off in the weeds above.

I think we need to reverse the order of the conditional for isolated exercises to build properly:

# Use the common Catch library?
if(${EXERCISM_COMMON_CATCH})
    # For Exercism track development only
    add_executable(${exercise} ${file}_test.cpp ${exercise_cpp} ${file}.h $<TARGET_OBJECTS:catchlib>)
else()
    # Build executable from sources and headers
    add_executable(${exercise} ${file}_test.cpp ${exercise_cpp} ${file}.h test/tests-main.cpp)
endif()

because if (NOT <expr>) behaves differently that if (<expr>).

@KevinWMatthews
Copy link
Contributor

KevinWMatthews commented Apr 24, 2019

There are some CMake if semantics here. I see this succeed:

if(NOT EXERCISM_COMMON_CATCH)

this fail:

if(NOT ${EXERCISM_COMMON_CATCH})

and this succeed:

if(NOT "${EXERCISM_COMMON_CATCH}")

Do you see something similar? This is when building as a student would - only one specific CMakeLists.txt file.

@arcuru
Copy link
Contributor Author

arcuru commented Apr 27, 2019

I alwyas like to work with CMake occasionally to remind me how much I hate it. That test is identical to the one elsewhere in the file that tests the run all tests option, except it has a NOT in it.

I think the problem is CMake for evaluates the branch in some instances, even if it's not taken? No idea.

Regardless, I added a test to CI to run it from an individual directory, and updated the exercise CMakeLists to what you suggested actually works.

Also changing the title to WIP, I need to test this on Visual Studio because I now don't trust this will work everywhere, I'll change it back after confirming.

@arcuru arcuru changed the title Using a common Catch main object for development. [WIP] - Using a common Catch main object for development. Apr 27, 2019
@arcuru
Copy link
Contributor Author

arcuru commented Apr 27, 2019

Oh, I need to fix how I did the testing of an individual test anyways.

@arcuru
Copy link
Contributor Author

arcuru commented Apr 28, 2019

Works fine in Visual Studio too.

I added running the individual test in CI to #279, since it requires a compilable hello-world anyways.

@arcuru arcuru changed the title [WIP] - Using a common Catch main object for development. Using a common Catch main object for development. Apr 28, 2019
@KevinWMatthews
Copy link
Contributor

I alwyas like to work with CMake occasionally to remind me how much I hate it.

There's always Autotools ;)

That test is identical to the one elsewhere in the file that tests the run all tests option, except it has a NOT in it.

I don't know for certain, but I think that if (NOT) has different variable expansion rules than if (). Counterintuitive, I agree.

Copy link
Contributor

@KevinWMatthews KevinWMatthews left a comment

Choose a reason for hiding this comment

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

Good overall.

One possible change for consistency but it isn't required.

CMakeLists.txt Outdated Show resolved Hide resolved
.travis.yml Show resolved Hide resolved
@arcuru
Copy link
Contributor Author

arcuru commented Jun 1, 2019

I missed that this was actually all finished apparently. I've updated after #284 and am merging.

I alwyas like to work with CMake occasionally to remind me how much I hate it.

There's always Autotools ;)

Thankfully I've never had to touch Autotools, but it scares me anyways.

@arcuru arcuru merged commit 2d6b671 into exercism:master Jun 1, 2019
@arcuru arcuru deleted the common_dep branch June 1, 2019 15:59
smuroff pushed a commit to smuroff/cpp that referenced this pull request Jun 3, 2019
smuroff added a commit to smuroff/cpp that referenced this pull request Jun 3, 2019
arcuru pushed a commit that referenced this pull request Jun 4, 2019
* Implement armstrong numbers

* Increased difficulty a bit

* Made changes to the default CMakeLists.txt (based on PR #276)

* Corrected vertical spacing. Used REQUIRE_FALSE for clarity

* Corrected include guard

* Made example solution more explicitly
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.

Speed up CI builds
2 participants