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

mp-units/.github/generate-job-matrix.py

Lines 55 to 72 in bc440cc

	@property
	def is_supported(self) -> bool:
	# check if selected features are supported by the platform
	s = self.platform.feature_support
	for field in dataclasses.fields(Features):
	if getattr(self, field.name) and not getattr(s, field.name):
	return False
	# additional checks for import_std
	if self.import_std:
	if self.std < 23:
	return False
	if not self.cxx_modules:
	return False
	if not self.std_format:
	return False
	if self.contracts != "none":
	return False
	return True

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:

mp-units/conanfile.py

Lines 114 to 122 in c1d323a

	"explicit_this": {
	"min_cppstd": "23",
	"compiler": {
	"gcc": "14",
	"clang": "18",
	"apple-clang": "",
	"msvc": "",
	},
	},

[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]

Thanks! Please let me know if you plan to work a bit more on this or if it should be merged?

[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.