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

Added an option for range-v3 to build tests #1535

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

adembudak
Copy link

Added range_v3_tests build option and set it off by default.

Copy link
Member

@eli-schwartz eli-schwartz left a comment

Choose a reason for hiding this comment

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

What's the advantage of the option? e.g. there aren't any external dependencies and you can restrict which tests to run by using the subproject name as a suite.

@@ -0,0 +1 @@
option('range_v3_tests', type : 'boolean', value : false)
Copy link
Member

Choose a reason for hiding this comment

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

  • defaulting tests off means they aren't run in the wrapdb CI
  • meson options are namespaces already, this option would get used as -Drange-v3:range_v3_tests=true. Don't include project names inside option names. Yes, I know you need it in cmake...

Copy link
Author

@adembudak adembudak May 27, 2024

Choose a reason for hiding this comment

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

i wanted to keep the variable name the same with the original repo (in upstream repo CMake configuration), but i agree having it just tests would have a better ergonomics and also following the current conventions of the Wrap files.

Copy link
Member

Choose a reason for hiding this comment

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

Still defaults to false?

Copy link
Author

Choose a reason for hiding this comment

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

Umm, yes. Don't build the tests by default.

Copy link
Member

Choose a reason for hiding this comment

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

Again, you need to make it build in CI. Previously it did so because it was enabled by default always.

See README.md.

@adembudak
Copy link
Author

What's the advantage of the option? e.g. there aren't any external dependencies and you can restrict which tests to run by using the subproject name as a suite.

The library files are header only which doesn't need to compile, enabled tests (400+ of them) need to compiled, waiting less and spending less CPU cycles would be some of the reason i guess :)

Copy link
Member

@eli-schwartz eli-schwartz left a comment

Choose a reason for hiding this comment

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

When fixing a review request in one commit, please use git commit --amend && git push --force to update. :) It is one logical commit, not two.

Per README.md, the releases.json file contains information about the release, and should have the revision incremented when making changes to the meson.build files.

if get_option('tests')
subdir('test')
endif

Copy link
Member

Choose a reason for hiding this comment

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

There's stray whitespace at the end of the files you edited?

Copy link
Author

@adembudak adembudak May 27, 2024

Choose a reason for hiding this comment

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

Fixed :)

@@ -0,0 +1 @@
option('range_v3_tests', type : 'boolean', value : false)
Copy link
Member

Choose a reason for hiding this comment

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

Still defaults to false?

@eli-schwartz
Copy link
Member

The library files are header only which doesn't need to compile, enabled tests (400+ of them) need to compiled, waiting less and spending less CPU cycles would be some of the reason i guess :)

Ah right, those are still compiled by default even if the tests themselves don't run...

@adembudak
Copy link
Author

Changed range_v3_tests to tests. Not sure whether it should be tests or test though. I chose the former but any feedback is greatly appreciated.

@benoit-pierre
Copy link
Contributor

tests appears to be the more common choice:

▹ grep -wl "'test'" subprojects/packagefiles/*/meson_options.txt | wc -l
4
▹ grep -wl "'tests'" subprojects/packagefiles/*/meson_options.txt | wc -l
45

@adembudak adembudak force-pushed the master branch 2 times, most recently from 585929d to 9156bb4 Compare May 27, 2024 14:31
@jpakkane
Copy link
Member

jpakkane commented Jun 9, 2024

Please do not have merge commits in your branch, keep your branch up to date using rebase instead.

@adembudak
Copy link
Author

adembudak commented Jun 9, 2024

Please do not have merge commits in your branch, keep your branch up to date using rebase instead.

Thanks, will keep it in mind next time.

@eli-schwartz
Copy link
Member

There are two problems:

  • the issue with merge commits isn't "please remember next time", but rather "we can't merge the PR until the merge commit is removed"
  • the merge commit also contains significant logic changes to the range-v3 wrap, which is one of the reasons the policy exists to not merge a merge commit

P.S. Sorry for not noticing the request to review.

@eli-schwartz
Copy link
Member

[1/479] Compiling C++ object subprojects/range-v3-0.12.0/test/action/range.v3.test.act.concepts.exe.p/cont_concepts.cpp.obj
FAILED: subprojects/range-v3-0.12.0/test/action/range.v3.test.act.concepts.exe.p/cont_concepts.cpp.obj 
"clang-cl" "-Isubprojects\range-v3-0.12.0\test\action\range.v3.test.act.concepts.exe.p" "-Isubprojects\range-v3-0.12.0\test\action" "-I..\subprojects\range-v3-0.12.0\test\action" "-I..\subprojects\range-v3-0.12.0\include" "/MDd" "/nologo" "/showIncludes" "/utf-8" "/W2" "/EHsc" "/std:c++17" "/permissive-" "/Od" "/Zi" "/Fdsubprojects\range-v3-0.12.0\test\action\range.v3.test.act.concepts.exe.p\cont_concepts.cpp.pdb" /Fosubprojects/range-v3-0.12.0/test/action/range.v3.test.act.concepts.exe.p/cont_concepts.cpp.obj "/c" ../subprojects/range-v3-0.12.0/test/action/cont_concepts.cpp
In file included from ../subprojects/range-v3-0.12.0/test/action/cont_concepts.cpp:10:
In file included from C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.40.33807\include\array:8:
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.40.33807\include\yvals_core.h(898,1): error: static assertion failed: error STL1000: Unexpected compiler version, expected Clang 17.0.0 or newer.
_EMIT_STL_ERROR(STL1000, "Unexpected compiler version, expected Clang 17.0.0 or newer.");
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.40.33807\include\yvals_core.h(519,44): note: expanded from macro '_EMIT_STL_ERROR'
#define _EMIT_STL_ERROR(NUMBER, MESSAGE)   static_assert(false, "error " #NUMBER ": " MESSAGE)
                                           ^             ~~~~~
1 error generated.

What does this mean???

Added `tests` build option and set it off by default.
@adembudak
Copy link
Author

adembudak commented Jun 10, 2024

[1/479] Compiling C++ object subprojects/range-v3-0.12.0/test/action/range.v3.test.act.concepts.exe.p/cont_concepts.cpp.obj
FAILED: subprojects/range-v3-0.12.0/test/action/range.v3.test.act.concepts.exe.p/cont_concepts.cpp.obj 
"clang-cl" "-Isubprojects\range-v3-0.12.0\test\action\range.v3.test.act.concepts.exe.p" "-Isubprojects\range-v3-0.12.0\test\action" "-I..\subprojects\range-v3-0.12.0\test\action" "-I..\subprojects\range-v3-0.12.0\include" "/MDd" "/nologo" "/showIncludes" "/utf-8" "/W2" "/EHsc" "/std:c++17" "/permissive-" "/Od" "/Zi" "/Fdsubprojects\range-v3-0.12.0\test\action\range.v3.test.act.concepts.exe.p\cont_concepts.cpp.pdb" /Fosubprojects/range-v3-0.12.0/test/action/range.v3.test.act.concepts.exe.p/cont_concepts.cpp.obj "/c" ../subprojects/range-v3-0.12.0/test/action/cont_concepts.cpp
In file included from ../subprojects/range-v3-0.12.0/test/action/cont_concepts.cpp:10:
In file included from C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.40.33807\include\array:8:
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.40.33807\include\yvals_core.h(898,1): error: static assertion failed: error STL1000: Unexpected compiler version, expected Clang 17.0.0 or newer.
_EMIT_STL_ERROR(STL1000, "Unexpected compiler version, expected Clang 17.0.0 or newer.");
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.40.33807\include\yvals_core.h(519,44): note: expanded from macro '_EMIT_STL_ERROR'
#define _EMIT_STL_ERROR(NUMBER, MESSAGE)   static_assert(false, "error " #NUMBER ": " MESSAGE)
                                           ^             ~~~~~
1 error generated.

What does this mean???

Seems like something related with clang-cl version, the runner has version 16.0.6:

range-v3| Project name: range-v3
range-v3| Project version: 0.12.0
range-v3| C++ compiler for the host machine: clang-cl (clang-cl 16.0.6)

@eli-schwartz
Copy link
Member

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.

4 participants