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

Add tests requirements #260

Merged
merged 1 commit into from
Mar 18, 2019
Merged

Conversation

Kojoley
Copy link
Contributor

@Kojoley Kojoley commented Mar 18, 2019

Not a full list of used features, but it should be enough. Will wash away failures on unsupported compilers from the regression board.

References

@Kojoley Kojoley force-pushed the add-tests-requirements branch from b84f1a2 to ebaae6b Compare March 18, 2019 02:28
Will wash away failures on unsupported compilers from the regression board
@Kojoley Kojoley force-pushed the add-tests-requirements branch from ebaae6b to 09c4f96 Compare March 18, 2019 02:34
@mloskot mloskot self-requested a review March 18, 2019 09:57
@mloskot mloskot added the config/boost-build Any issues about Boost.Build configuration label Mar 18, 2019
Copy link
Member

@mloskot mloskot left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@mloskot mloskot merged commit 4db69da into boostorg:develop Mar 18, 2019
@mloskot
Copy link
Member

mloskot commented Mar 18, 2019

@stefanseefeld Shall we target this for our Boost 1.70 milestone?

@Kojoley Kojoley deleted the add-tests-requirements branch March 18, 2019 18:13
@Kojoley
Copy link
Contributor Author

Kojoley commented Mar 18, 2019

Sidenote: Your jamfiles have <cxxflags>-std=c++11, I strongly suggest to remove it.

@stefanseefeld
Copy link
Member

@Kojoley how would you suggest we express requirements ?
Coincidentally, I just ran into an issue with the Boost.uBLAS project, where part of the library requires C++17. So we ended up adding requirements to the concerned tests, examples, and benchmarks, using <cxxstd>17. It turned out that that isn't quite working, as some testers set compiler flags using <cxxflags>, so <cxxstd> wasn't used in those cases.
So I'm genuinely curious about what the right way to express language version requirements is.

@stefanseefeld
Copy link
Member

@mloskot even though I would like to add this (as well as some / cleanup) into master, I'm afraid that this could trigger some failures, and thus affect the release schedule. It doesn't have any effect on the (user-visible) code, so I don't think it's urgent.

@mloskot
Copy link
Member

mloskot commented Mar 18, 2019

@Kojoley Yes, I'm aware and @pdimov was pointing it out that our Jamfile needs clean-up (e.g. #149)

@stefanseefeld The thing is, AFAIU, this PR actually fixes a known issue for the Boost regression matrix (#149)

@Kojoley
Copy link
Contributor Author

Kojoley commented Mar 18, 2019

You should not force compiler standard flags from the tests, it will prevent you from testing higher standard versions, just skip the test via requirements or do nothing based on Boost.Config macros.

The config.requires returns <build>no when feature check fails, so you can use it everywhere where you could supply <build>no. Per test example https://github.com/boostorg/fusion/blob/4c51811315af8257f420ada0f195a89d479c684e/test/Jamfile#L231-L237

@mloskot
Copy link
Member

mloskot commented Mar 18, 2019

You should not force compiler standard flags from the tests, it will prevent you from testing higher standard versions

Yes, the idea is clear and I also try to achieve it in the CMake config, https://github.com/boostorg/gil/blob/develop/CMakeLists.txt#L26 and I planned to explore it further to get clear about exactly the same question as @stefanseefeld is asking earlier.

Here is what I've learned so far:

  1. use the requires to filter unsupported compilers
  2. do not add <cxxflags>-std=c++11 to allow newer versions
  3. call b2 with explicit cxxstd=V where V lists one or more from 11 1y 14 1z 17 2a latest

But, is 1. and 2. enough to express minimum required version?

@Kojoley
Copy link
Contributor Author

Kojoley commented Mar 18, 2019

But, is 1. and 2. enough to express minimum required version?

In most cases the things Config and Predef provide should be enough. You can always write a check that will satisfy your particular requirements.

I also try to achieve it in the CMake config, /CMakeLists.txt@develop#L26

I am not an expert in CMake, but seems you are doing it wrong. It is the same thing as just setting -std manually. The minimum standard requirement for the target seems should be expressed like this. CMake also allows you to make future checks, and have build-in standard future checks.

@mloskot
Copy link
Member

mloskot commented Mar 18, 2019

In most cases the things Config and Predef provide should be enough.

Good.

You can always write a check that will satisfy your particular requirements.

I do not wish to write any custom checks, if that is what you suggest.

The goal of this build configuration is as simple as this:

  1. Force any pre C++11 compiler to fail, earlier than during actual compilation.
  2. Behave nicely when we are being tested as part of Boost super-project tree.

All other aspects, if any, are low priority. If, however, anyone feels anything else should be improved, great, then PRs are welcome.

I am not an expert in CMake, but seems you are doing it wrong.

I disagree. I would have been doing it wrong if there were public targets defined for use by GIL users.

It is the same thing as just setting -std manually.

Yes, it is and there is nothing wrong with that there.
One can build tests with CMAKE_CXX_STANDARD=11 as the default or request CMAKE_CXX_STANDARD=17 or later.

GIL's CMake config is not a pure modern CMake, yet. Regardless if it is modern or not, it is dedicated to those who wish build GIL tests and examples, and this config is not supposed to export anything to public or be well-behave interacting with any external CMake config. Shortly, it is purely for internal development purposes. Setting CXX_STANDARD globally is a convenient shortcut.

@Kojoley
Copy link
Contributor Author

Kojoley commented Mar 18, 2019

I do not wish to write any custom checks, if that is what you suggest.

Actually I am not even sure what was your question, so the answer was broad.

It is the same thing as just setting -std manually.

Yes, it is and there is nothing wrong with that there.

Just remember that by not respecting platform defaults you are making troubles for package maintainers.

@mloskot
Copy link
Member

mloskot commented Mar 19, 2019

Your points are clear and valid, in general. They just do not apply to GIL (not to its current setup; until we've got Boost official CMake, etc.)
There's no Jamfile or CMakeLists.txt here to package.

@mloskot
Copy link
Member

mloskot commented Mar 20, 2019

@Kojoley I should have clarified my point better. The current build configurations in GIL are for development purposes, for convenience of contributors. They are not free from hacks and they could follow best practices more. It's just that 1) it has not been my priority to improve it; 2) I have failed myself to see where or how to improve it (I know the CMake deserves modernisation though). I do not claim the current build configurations are optimal.

If you know where to improve the build configs and if you are willing to help out, awesome, please do!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
config/boost-build Any issues about Boost.Build configuration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants