Add approvaltests.cpp 10.12.0 #7213

isidore · 2021-09-08T16:51:38Z

Specify library name and version: approvaltests.cpp/10.12.0

new version

I've read the guidelines for contributing.
I've followed the PEP8 style guides for Python code in the recipes.
I've used the latest Conan client version.
I've tried at least one configuration locally with the
conan-center hook activated.

claremacrae · 2021-09-10T06:50:09Z

This build is failing, but not due to anything in the PR… I think it’s likely to be due to a change in the Conan CI config?

boost 1.72.0 is missing for one config: https://c3i.jfrog.io/c3i/misc/logs/pr/7213/1-configs/macos-m1-clang/approvaltests.cpp/10.12.0//5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9-test.txt … we didn’t change anything in that area….

I’ve asked about it in the Conan channel on #include <C++> Discord.

SSE4 · 2021-09-10T06:54:33Z

This build is failing, but not due to anything in the PR… I think it’s likely to be due to a change in the Conan CI config?

boost 1.72.0 is missing for one config: https://c3i.jfrog.io/c3i/misc/logs/pr/7213/1-configs/macos-m1-clang/approvaltests.cpp/10.12.0//5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9-test.txt … we didn’t change anything in that area….

I’ve asked about it in the Conan channel on #include <C++> Discord.

yes, there is an issue that right now boost doesn't build for ARM due to the bug in CMake version. PR #7146 addresses that, but it's not merged yet (still needs some approvals).
there is no problem with approvaltests, for now the best you can do is to skip ARM builds for a while (until boost PR is merged):

if self.settings.arch == "armv8":
    raise ConanInvalidConfiguration("see #7146")

SSE4 · 2021-09-10T14:51:10Z

okay, #7146 has been merged, so packages should be there. I've re-triggered the CI.

SSE4 · 2021-09-10T15:16:31Z

it seems to be using some x86_64 ASM even if compiling for ARM?

#define CATCH_TRAP() __asm__("int $3\n" : : ) /* NOLINT */

if so, please disable armv8 builds as suggested in the comment above: #7213 (comment)

Co-Authored-By: Clare Macrae <github@cfmacrae.fastmail.co.uk>

madebr · 2021-09-10T17:33:42Z

Looks like cancelled builds are interpreted by conan-center-bot as successfully.

SSE4 · 2021-09-10T17:34:49Z

Looks like cancelled builds are interpreted by conan-center-bot as successfully.

right, seems to be another bug! thanks for spotting!

claremacrae · 2021-09-10T18:41:11Z

Is there anything we need to do?

madebr · 2021-09-10T18:42:31Z

You need to add "arch" to settings.

claremacrae · 2021-09-10T19:20:58Z

You need to add "arch" to settings.

Thanks... Our release process tests that the PR builds... we didn't think to do that with the earlier change today - but I did then test the most recent commit, so hopefully it will all be good now.

SSE4 · 2021-09-10T19:35:48Z

You need to add "arch" to settings.

Thanks... Our release process tests that the PR builds... we didn't think to do that with the earlier change today - but I did then test the most recent commit, so hopefully it will all be good now.

we recently added some changes which enhanced test coverage for header only packages.
previously, header-only packages were built only on Linux.
now, they at least built once on Windows/macOS/Linux.
it may cause some recipes which previously were green to fail now.

madebr · 2021-09-10T20:47:10Z

This is a fluke.
boost/1.77.0 was just merged; #6929

2 things you can do:
bump the boost version used by approvaltests.cpp,
or close and re-open this pr.

claremacrae · 2021-09-10T20:50:49Z

This is a fluke.
boost/1.77.0 was just merged; #6929

2 things you can do:
bump the boost version used by approvaltests.cpp,
or close and re-open this pr.

Thanks @madebr - I cannot close and re-open the PR as it was created by the other co-author of approval tests, and we have finished pairing for today...

So please could someone on the Conan team close and re-open this?

claremacrae · 2021-09-10T20:51:53Z

I'm curious... Why would the merging of boost 1.77.0 cause builds here to fail?

madebr · 2021-09-10T20:57:30Z

#6929 caused boost packages of all versions to be rebuilt.
I assume pushing a new recipe + package to conan central is not an atomic action.

claremacrae · 2021-09-12T11:14:52Z

seems like doctest suffers the same issue as catch (are they copy-pasting each other 👀 ?):
#define DOCTEST_BREAK_INTO_DEBUGGER() __asm__("int $3\n" : :)
need this commit: onqtam/doctest@3c5e0fa
(versions 2.4.1 - 2.4.6 have it, you're using 2.3.6 right now)

HUGE THANKS for this information - much appreciated! And I've done that...

SSE4 · 2021-09-12T11:32:02Z

/home/conan/w/BuildSingleReference/.conan/data/approvaltests.cpp/8.1.0/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/ApprovalTests.hpp:741:14: 
note:   no known conversion for argument 1 from ‘const doctest::String’ to ‘const string& {aka const std::__cxx11::basic_string<char>&}’

did doctest change their interfaces? 🤔

SSE4 · 2021-09-12T11:40:29Z

checking again, it's only old versions of approvaltests fails with up-to-date doctest.
perhaps, only approvaltests.cpp/8.2.0+ are compatible with new interfaces.

claremacrae · 2021-09-12T11:42:58Z

checking again, it's only old versions of approvaltests fails with up-to-date doctest.
perhaps, only approvaltests.cpp/8.2.0+ are compatible with new interfaces.

Thank you...

I am feeling a little bit unlucky with this PR - they aren't usually this hard!!! :-)

SSE4 · 2021-09-12T11:55:41Z

checking again, it's only old versions of approvaltests fails with up-to-date doctest.
perhaps, only approvaltests.cpp/8.2.0+ are compatible with new interfaces.

Thank you...

I am feeling a little bit unlucky with this PR - they aren't usually this hard!!! :-)

yeah, really a bad timing, we upgraded several things in infrastructure which caused previously green builds to fail.
we're trying to do our best to minimize breakage moving forward (adding more platforms)

claremacrae · 2021-09-12T12:39:49Z

yeah, really a bad timing, we upgraded several things in infrastructure which caused previously green builds to fail.
we're trying to do our best to minimize breakage moving forward (adding more platforms)

I would like to ask the conan team to see if a way can be found to de-couple the “contributor submitting a PR for an update” from “the contributor discovering the existing build got broken (or a pre-existing limitation was exposed) because of improvements to infrastructure”…

For example, perhaps have a weekly job that re-builds every recipe on the latest infra - and somehow capture which ones failed and notify maintainers…

That way, we could have had separate PRs for “get this library working on new platforms” and “update this library for a new release…”

By the current coupling together of “only find out your previously working recipe is broken when you try to add a new release”, I feel it becomes much harder to deal with…

prince-chrismc

Perhaps this is a candidate for remove older versions?

prince-chrismc · 2021-09-14T00:00:14Z

recipes/approvaltests.cpp/all/conanfile.py

        if self.options.with_gtest:
            self.requires("gtest/1.10.0")
        if self.options.with_doctest:
-            self.requires("doctest/2.3.6")
+            self.requires("doctest/2.4.6")


Suggested change

self.requires("doctest/2.4.6")

self.requires("doctest/2.4.6" if tools.Version(self.version) >= "8.2.0" else "doctest/2.3.6")

Thank you very much. I thought about a condition like that, but was assuming that the older versions would still fail the CI for armv8 (or whatever the problem platform was).

Do you mean to drop support for older versions of ApprovalTests.cpp @prince-chrismc - I didn't know it was possible to do that...

If so, it solves all these CI problems in stroke, without making the recipe more complex, so is a great idea...

claremacrae · 2021-09-15T17:40:54Z

Things I have found from experimenting on my Mac:

doctest/2.3.6

All versions of ApprovalTests.cpp in Conan build with doctest/2.3.6 (which is the version referenced in CCI master)
But that doctest does not build on archv8

doctest/2.4.3

ApprovalTests.cpp 8.0.0 to 8.8.0 do not build with this
8.8.1 and later do build
I suspect this may be the first doctest release that works on armv8

doctest/2.4.6

ApprovalTests.cpp 8.0.0 to 8.8.0 do not build with this
8.8.1 and later do build

claremacrae · 2021-09-15T17:45:34Z

So there are two orthogonal things going on here...

Which versions of doctest are compatible with which of ApprovalTests.cpp
Which versions of doctest are compatible with armv8 builds

I think the fundamental blockers to this PR are:

Conan added builds on CI platforms that are not compatible with current versions of ApprovalTests.cpp dependencies
doctest added a breaking change without bumping the major version number...

One of my options is this - I think it's my simplest option, and retains the previous behaviour on default...

Revert back to previous behaviour of selecting doctest/2.3.6
Reinstating the refusal to ever run on armv8

claremacrae · 2021-09-15T17:49:47Z

Another of my options is to remove the following versions from the ApprovalTests.cpp CCI recipe...

8.0.0
8.1.0
8.1.1
8.2.0
8.3.0
8.4.0
8.5.0
8.6.0
8.7.0
8.7.1
8.8.0

The following versions would remain:

8.8.1
8.9.0
8.9.1
8.9.2
9.0.0
10.0.0
10.0.1
10.1.0
10.1.1
10.1.2
10.2.0
10.2.1
10.3.0
10.4.0
10.5.0
10.5.1
10.6.0
10.7.0
10.7.1
10.8.0
10.9.1
10.10.0
10.11.0
10.12.0

Since ApprovalTests.cpp does try hard to honour semantic versioning, and Conan would still support several 8.x.y releases, this maybe wouldn't inconvenience too many users - but if they have pinned to a particular ApprovalTests.cpp release, I think it would still break their builds???

claremacrae · 2021-09-15T17:54:22Z

One of my options is this - I think it's my simplest option, and retains the previous behaviour on default...

Revert back to previous behaviour of selecting doctest/2.3.6

Reinstating the refusal to ever run on armv8

Given that ApprovalTests.cpp is used in Conan as a single-header, this should not even prevent uses from accessing the library via Conan on armv8, I presume...

madebr · 2021-09-15T17:56:39Z

You can conditionally add different versions of requirements + do tests.
Something like the following (untested):

def requirements(self):
    if tools.Version(self.version) >= "8.8.0":
        self.requires("doctest/2.4.6")
    else:
        self.requires("doctest/2.3.6")

def validate(self):
    if tools.Version(self.version) < "8.0.0":
        if tools.Version(self.deps_cpp_info["doctest"].version) >= "2.4.0":
            raise ConanInvalidConfiguration("The version of doctest is too new, use an older version (pre 2.4.0)")

Then, you can remove some old versions, while keeping some.
With the cmake recipe, we only keep the highest minversion. (e.g. remove 18.0, 18.1, 18.2 but keep 18.3)

claremacrae · 2021-09-15T17:58:10Z

Thank you for the suggestion. The problem with this is that doctest 2.3.6 will fail to build on CCI's armv8 CI builds...

prince-chrismc · 2021-09-15T18:00:11Z

Removing versions does not impact any user AFAIK, they just wont get updates anymore for improvements (or new platform support) assuming they are not using revisions.

I would like to suggest

pick the highest doctest version possible (my suggest above)
block arm for all versions less than 8.8.0 (with a nice comment)

I think that would be the best middle ground of the blockers you very nicely summarized

madebr · 2021-09-15T18:02:35Z

What ⬆️ said.
My suggestion would complicate the recipe and make future maintenance harder.

claremacrae · 2021-09-15T18:05:50Z

Removing versions does not impact any user AFAIK, they just wont get updates anymore for improvements (or new platform support) assuming they are not using revisions.

"assuming they are not using revisions." ... I don't know the consequences of this, if they were using revisions...

I would like to suggest

pick the highest doctest version possible (my suggest above)

block arm for all versions less than 8.8.0 (with a nice comment)

I think that would be the best middle ground of the blockers you very nicely summarized

Ok, so if I understand this correctly, it will then allow me to retain all the earlier versions...

Thank you very much indeed. I will go ahead and do this, but probably tomorrow...

@uilianries

All the older versions will persist on conan center: their recipes will just no longer be tested during CI builds, and they won't get new revisions. It was suggested by @uilianries that I retain the newest releases on the 8.x and 9.x series, in case their receipes ever need updating, and so long as they do not complicate the Python recipes. I've also kept a few of the 10.x series as well.

uilianries

Less is more

claremacrae · 2021-09-15T18:57:26Z

From @uilianries on #include Discord, I got a better understanding of the (lack of) consequences of removing old versions from the yaml, so I've kept the latest of each on the 8.x and 9.x series, and the most recent 4 on 10.x series...

As I understand it:

all the deleted releases will still appear in the web site and by conan search command (I was unaware that this would happen)
these should all just pass the CI with the current rules on this branch
and anyone using the deleted releases will get the previous recipes, which will give them the older doctest version, that works with all the ApprovalTests releases...

This massively simplifies our Python recipe...

Fingers crossed the CI just works now...

claremacrae · 2021-09-15T18:58:48Z

Fingers crossed the CI just works now...

And whatever happens, I'll be warming the planet just a little bit less, with fewer CI build steps in this and future PRs!!!

conan-center-bot · 2021-09-15T19:31:00Z

All green in build 10 (879ea58e3a90e1b885f1292518c16e4facb9f23c):

approvaltests.cpp/8.9.2@:
All packages built successfully! (All logs)
approvaltests.cpp/9.0.0@:
All packages built successfully! (All logs)
approvaltests.cpp/10.9.1@:
All packages built successfully! (All logs)
approvaltests.cpp/10.10.0@:
All packages built successfully! (All logs)
approvaltests.cpp/10.11.0@:
All packages built successfully! (All logs)
approvaltests.cpp/10.12.0@:
All packages built successfully! (All logs)

prince-chrismc

LGTM 🚀

claremacrae · 2021-09-15T20:17:12Z

Thank you everyone for all the support on this PR!

This was caused by my pruning out a lot of older releases from the Conan CI. See conan-io/conan-center-index#7213

Add approvaltests.cpp 10.12.0

14e6e99

conan-center-bot added the Bump version PR bumping version without recipe modifications label Sep 8, 2021

madebr previously approved these changes Sep 8, 2021

View reviewed changes