ensure that we have an import_std configuration in the test-set

[burnpanck]

also, update flake8

[mpusz]

Ahh, I forgot to say that it also requires C++23+ to work 😢

Also, I do not know why a freestanding build failed.

FYI, I am providing a 3-day training from today, so I may be less responsive than usual.

[mpusz]

I've just changed the CI scripts (but not the generator) to account for C++23. They should also check for something like import_std_support when provided by the matrix.

[burnpanck]

the freestanding build failed because of an issue you seemed to have previously identified: In Debug builds, the CMakeCheckCompiler test fails due to a missing _main symbol. you had an "exclude" in the matrix, but "include" comes after, so we have to account for that in the generator. I forgot about that when I added that special "cxx_modules" subsample.

[burnpanck]

I might try to merge freestanding with the hosted conan, making this just another dimension of the matrix. I'll also extract the cxx_modules into its own dimension, and ensure all the unsupported combinations are skipped.

[burnpanck]

finally, I'll experiment with a truly random sample of configurations, together with a GitHub Actions "manual_trigger" with a user input to choose the random seed. I belive that should be possible.

[mpusz]

WOW! You are a true GitHub Actions Wizard! 😄 I would never come up with those solutions by myself. It is great to learn those things from you. Thanks!

[mpusz]

Also, it seems that there is some issue with contracts setting as it resolves to an empty string instead of "none" here: https://github.com/mpusz/mp-units/actions/runs/11826393072/job/32952538814#step:15:3

[mpusz]

Hi @burnpanck, will you have some time to look into it soon?

[burnpanck]

I'l try to do it today. I would probably remove all logic affecting the settings of std_format and import_std from the CI, and instead include that into the generator; this has a few advantages: The logic determining which elements to run from the matrix properly respects what is actually being run in CI, and the logic determining what configurations are compatible is next to where the combinations are being configured. Would that be ok for you @mpusz? From my experience that would also be safer in the sense that a wrong configuration of the matrix would cause a loud failure rather than silently skipping an important test.

[mpusz]

Sure, it sounds great. However, please be careful with the import_std as this requires a very specific configuration and it is enforced through the conanfile.py. If import_std is set to True but other options are invalid, Conan will raise an error.

[burnpanck]

If import_std is set to True but other options are invalid, Conan will raise an erro

That is a good thing. It means that we won't accidentally avoid testing import_std=True. If the matrix generator script thinks that a particular configuration with import_std=True is valid and schedules it for testing, it should either indeed test import_std=True or it should fail. Note also that the logic dealing with mutually incompatible configurations is very localised, so it should be fairly easy to maintain https://github.com/mpusz/mp-units/blob/bc440cc15f159b7ad8e017f8c0d9db6badb76fa7/.github/generate-job-matrix.py#L55-L72

Finally, as promised, I added changed the random seed to be truly random. It chosen seed is printed at the start of the "generate-matrix" job. I also added a workflow_dispatch trigger to allow manual starts of each of the modified workflows, including an input to select a specific seed, perhaps to re-run a previous failed test.

[burnpanck]

I forgot to mention; the workflow_dispatch is only effective if it is on the default branch, so we unfortunately won't be able to test that until after the merge.

[burnpanck]

Oh, and the failed GCC-13 C++23 fmtlib none Release CMake test doesn't appear to be related to my changes here; either it has been pulled in with the recent merge, or it has always been there and has been discovered here through a lucky random choice.

[mpusz]

I also added a workflow_dispatch trigger to allow

Thanks a lot! You are a true GitHub Actions wizard 🧙🏻 I wouldn't know how to do at least half of the things you contributed in the last few days.

[mpusz]

As you have introduced "features" maybe we should also explicitly name no_crtp? The requirements for it are also provided here:

https://github.com/mpusz/mp-units/blob/c1d323a91a44fc19f36e237ed91fc242140759ce/conanfile.py#L114-L122

[mpusz]

Thinking a bit more about workflow_dispatch, do you think it could be a good idea to be able to manually run the tests for all of the valid possible combinations? Being able to run it by hand could allow us to ensure that everything is OK before doing the library release.

[mpusz]

@burnpanck, you have 5 open PRs. Do you plan to work on those soon? I would like to provide the next mp-units release soon.

[burnpanck]

@mpusz: Unfortunately, I can't guarantee anything right now. I think most of my PRs were generally mergeable, but of course, now that time has passed, neither you nor I myself remember exactly what the last status was. This one here was ready except for your latest change requests, which do not appear too difficult to do. Maybe this weekend?

[mpusz]

The freestanding configuration should have contracts=none and not use std_format option at all.

[mpusz]

@burnpanck, it would be great if you could finish your PRs soon. I plan to release the final V2 version soon and then start working on V3. Will you be able to work on those PRs soon?

[burnpanck]

Picking things up again... It looks like we got some new conan options that weren't implemented as features previously (no_crtp was already mentioned in this thread, then we have natural_units). The version of the GHA workflow in this branch compared to current master already has a few improvements to better separate "features" (conan options) from other build configurations; this should help both for these two features and future changes. I'll try to finish this and also implement that workflow dispatch with the option to run a denser or even full matrix.

[burnpanck]

With the matrix now including no_crtp and natural_units, I think running a full dense matrix might be out of question: Even without -ffreestanding, that is about 1500 combinations. Freestanding brings another 1000. But what we could do is add a workflow_dispatch option that triggers a higher-density matrix, or a full dense matrix in a certain subspace of configuration options. Just let me know which :-). In the meantime I'll try to figure out if the current CI failures are a) bugs caused by my changes, b) unsupported configurations or c) unrelated upstream bugs.

[mpusz]

Thanks @burnpanck! If we enable random configurations at CI it should already provide a nice coverage. If I would need to select a most important configuration than it would be:

• cxx_modules: true (this tests headers as well)
• std_format: true
• no_crtp: true
• freestanding: false
• natural_units: true

This is the most common and forward looking use case.

We should test it on all supporting compilers with Debug and Release with import_std on and off.

Additionally, we should consider setting a new value for contracts. I hope that we will be able to add standard option to it as well :wink:

[burnpanck]

I implemented the workflow_dispatch option to select release-tests (in the "Conan CI" and "CMake Test Package CI" workflows). workflow_dispatch is only respected on the default branch, so you will only see it once merged (I did a test run in my fork by temporarily switching the default branch to this one). The configuration I have chosen for the release-tests is as follows:

1. All the tests that would be run normally (whatever is picked by that random sample; now around 27 configurations)
2. All toolchains and valid combinations restricted as follows:
   • std_format: true
   • freestanding: false
   • natural_units: true
   • contracts: gsl-lite unless import_std is set, then none
   • no_crtp: true when using C++23, and false under C++20 (because no_crtp requires C++23)
   • cxx_modules: both combinations are tested (because only Clang supports true according to our tables)

With that, the full release-tests configuration ends up running about ~ 85 configurations.

[burnpanck]

@mpusz I may need help to fix the remaining failing tests. I believe these aren't failures due to my changes, but simply issues being uncovered by the better test coverage. Specifically, I see two types of failures:

• natural_units: false: Tests fail with fatal error: 'mp-units/framework/system_reference.h' file not found. Somehow, the cmake install correctly picks up the MP_UNITS_API_NATURAL_UNITS and doesn't install that header, but during the build, its missing.
• import_std: true: Tests fail with Experimental "import std" support not enabled when detecting toolchain; it must be set before "CXX" is enabled (usually a "project()" call). According to this thread on the CMake discourse, it may be related to specific CMake versions requiring different values for CMAKE_EXPERIMENTAL_CXX_IMPORT_STD.