-
Notifications
You must be signed in to change notification settings - Fork 31
Feature/test installed package #173
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
Feature/test installed package #173
Conversation
wusatosi
left a comment
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.
I'm very much for testing installation.
This clashes with #122 on functionality, but that PR has not been updated in a while.
@nickelpro can you check if this approach is good?
6d2cffa to
9b3db9d
Compare
55f0336 to
b35342d
Compare
|
see #172 |
|
Yep, you beat me to it, version file name is wrong |
cc3f595 to
01a5be1
Compare
45f6a25 to
5b8132b
Compare
|
@camio @dietmarkuehl @ednolan Is it possible to review and merge this MR? It was ready since weeks, I do not want to rebase it again! |
dietmarkuehl
left a comment
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.
I'm not cmake expert at all! Mostly the changes look OK to me. I believe such a test is present on beman.execution and it takes more time than all other tests together, significantly slowing down my development: I build often and something which takes multiple seconds is annoying.
I don't see a way to disabe this test without modifying the CMakeLists.txt (I haven't tried it, though).
Good point! |
|
FYI, Claus: You can test cookiecutter locally by running |
|
@ednolan after rebase ci fails, what dit you do? |
|
I merged a refactor of the CI system. It made various changes, but I think the change that appears to have broken things here is that, previously, CI would build with Ninja Multi-Config, and it would always build Debug before building Release. Now, it still uses Ninja Multi-Config, but it only builds either just Debug or just Release. I don't know why that would make a difference, but I think that's the culprit because it looks like all the CI failures here are in one of two categories:
I'm continuing to look into this. |
I got it. We need on CI set! |
|
I've managed to reproduce thie issue locally by launching one of the new images with the following command: And running the following commands: This produced the following output: Note specifically in: It's looking for a directory called I'll push an update to CI that enables CTEST_OUTPUT_ON_FAILURE. |
| "hidden": true, | ||
| "generator": "Ninja", | ||
| "binaryDir": "${sourceDir}/build/${presetName}", | ||
| "installDir": "${sourceDir}/stagedir", |
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.
Now that the tests don't assume an installDir being set by the preset, we should not be mandating the installDir here
| { | ||
| "name": "_root-build", | ||
| "hidden": true, | ||
| "targets": [ |
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.
Ditto, mandating targets is no longer required so we shouldn't
| VERSION 2.1.1 | ||
| ) | ||
|
|
||
| if(PROJECT_IS_TOP_LEVEL) |
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 example code should not be used for testing. This code serves no purpose for a user who is copy-pasting the example into their own source directory. It serves only to confuse them.
The test should be a five line project that calls cmake_minimum_required() -> project() -> find_package(REQUIRED) -> if(NOT TARGET). It should not be an example. It should exist solely for the purpose of testing the imports.
| /compile_commands.json | ||
| /build | ||
| /CMakeFiles | ||
| /stagedir |
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.
These shouldn't exist outside the build directory anyway, no need to add them here
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.
see #217
I am the point to stop my support of this project. It takes to much time and to much discussions for me! |
Description
Use ctest --build-and-test to check the installed version
Related Issues
Motivation and Context
Every
exported cmake config packagemust be checked for its usability.Testing
Meta
see https://discourse.cmake.org/t/is-beman-exemplar-beman-exemplar-version-cmake-a-vallid-package-name/14188