coin-lemon: allow to consume coin-lemon with C++20 or higher

[SpaceIm]

closes #15515

see #15516 for previous attempt

Here is the kind of error you can get if it's not fixed and compiler.cppstd=20 (or 23): #16356 (comment)

---

• I've read the contributing guidelines.
• I've used a recent Conan client version close to the currently deployed.
• I've tried at least one configuration locally with the conan-center hook activated.

[jcar87]

is this a problem that exists purely in the Conan Center recipe, or is this also an issue with the upstream library?

If the upstream library does not work with C++20, then perhaps the recipe should reflect that. In similar cases the patches are pulled from upstream for issues that have already been fixed in newer versions - but we do not want the recipes in Conan Center to deviate from the upstream code any more than necessary, and we cannot speculate that the authors will follow the same approach when this is actually fixed, so we need to minimise any potential changes in behaviour.

Is there an issue that has been opened against the issue tracker at https://lemon.cs.elte.hu/trac/lemon/report/1 ?

[SpaceIm]

There is https://lemon.cs.elte.hu/trac/lemon/ticket/631, but LEMON is barely maintained.

[jcar87]

There is https://lemon.cs.elte.hu/trac/lemon/ticket/631, but LEMON is barely maintained.

Thanks for providing a link.

If this library is is barely maintained, I think the more reason we have to prevent Conan Center contributors and maintainers from becoming maintainers of this library by proxy. Consumers of libraries (regardless of which package manager or repository they choose to use), should take this into account when making the decision to use, or continue to use, libraries: this library is barely maintained and does not support C++20.

Conan Center should be very conservative with patching upstream code: users should be able to trust us that we build the code that the authors ship, but most importantly, that we build from the code that the authors have developed and tested. New code introduced in patches (even if it works well, which I'm assuming it does) - do not fall in this category. In my opinion, this is an obvious case of making a configuration invalid in the validate() method.

Enterprise users have the freedom (and are encouraged) to maintain their own versions of recipes and libraries where they need them to deviate from upstream in ways that we cannot support. And all users in general are greatly encouraged to raise issues of this kind with the library authors themselves. If this library was not building with C++20 due to an issue with Conan Center or the Conan client, that would be a different story.

[SpaceIm]

It's a portability patch, that's all, maybe a little bit more intrusive than patching CML, but it doesn't change API, ABI, nor any algorithm of LEMON. It's the straightforward replacement of std::allocator<T>::construct & std::allocator<T>::destroy (deprecated in C++11 and removed in C++20).
If it's not accepted, it will fail soon or later with new compilers if they change their default C++ standard, and this library will become unusable in conancenter (you can raise or let the recipe fails, it's the same result for users, library can't be used), as well as downstream recipes (these libraries are not aware of conan, we just package them).
If there was a release with a fix, well I could understand, but the latest version is 8 years old...

[prince-chrismc]

We've never blocked these patches, and they conform to the patch policy... https://github.com/conan-io/conan-center-index/blob/master/docs/adding_packages/sources_and_patches.md#rules

ConanCenter DOES accept working software patches, these patches are needed to generate the binaries for architectures not considered by library maintainers, or to use some compilers or configurations. These patches make it possible to generate binaries that cannot be generated otherwise

Going back on this is too late, we did not (and barely now) track these patches so we could not remove them all.

[uilianries]

LGTM

[SpaceIm]

Why is it still blocked?

[prince-chrismc]

We added it to our Shepards meeting to discuss, I've always felt the patch policy was unfinished and we the changes in the team we have new ideas (and changing expectations). So we are going to re-open this internal and make decision :)

[prince-chrismc]

Quick update on this, we started the conversation. Not done (disclaimer subject to change) but just a heads up.

Our theme for this year is simplification and in combinations with the changing expectations around ConanCenter - we are most likely going to change the patch policy (keep an eye out for that). The idea is to reduce the number of patches and reduce the footprint that ConanCenter is stretched into. Both are simplifications :)

We also have new tools with validate_build being supported so I suspect our decision will be to take advantage of that so we do not support configurations that we are enabled by upstream. It's likely we will ask for changes here to be clear :)

Bare with us, as we sort this out. There will be a little bit of room for feedback of course; this is important topic with strong opinions so it might be difficult to satisfy the 100k+ voices but as maintainers we will be setting the new expectations with everyone in mind.

[CLAassistant]

All committers have signed the CLA.

[SSE4]

@jcar87 can you unblock?

[SSE4]

@prince-chrismc can we move it forward? C++23 is already finalized and C++26 is on its way. given the fact, new C++ standards are removing and deprecating old features (e.g. auto_ptr, bind2nd, random_shuffle, mem_fun, register keyword, etc.), many old and unmaintained libraries will fail to compile with newer C++ standards and will require various patches. some will not compile due to other changes in standards (e.g. comparison operator lookup changes).
there should be a strong message on how is it going to be handled going forward - do we just accept that patches, or make new cci. releases for such changes, or make forks and accept as a new package, or reject and only provide libraries as is.
people will continue to upgrade to C++23 and further, so it will be more and more hot topic.

[SSE4]

@prince-chrismc any update?

[SpaceIm]

I close/open this PR to fix CI status since nobody cares.

[conan-center-bot]

Conan v1 pipeline ✔️

All green in build 3 (d687a9ab1bc2ce85f20e681ff6944069cb035fd8):

• coin-lemon/1.3.1@:
  All packages built successfully! (All logs)

---

Conan v2 pipeline ✔️

Note: Conan v2 builds may be required once they are on the v2 ready list

All green in build 3 (d687a9ab1bc2ce85f20e681ff6944069cb035fd8):

• coin-lemon/1.3.1@:
  All packages built successfully! (All logs)

[SpaceIm]

@jcar87 how can we move it forward?

[SSE4]

@jcar87 @prince-chrismc @uilianries @RubenRBS @franramirez688 @memsharded ping, it's being blocked for a long time already without any clear guidance on how to proceed.
many people are upgrading towards newer C++ standard like C++20, C++23 or even C++26. but there is still no clear indication on how does conan should handle it. the radio silence isn't good, let's work together to make C++ community happy.

[prince-chrismc]

Thanks for the ping! We have answered this in other channels but I'll post here as well for continuity :)

We are moving away from accept patches with changes C++ code. Please see the updated patch policy if you have questions. #19185 which also had suggestion for this to be approved.

If you require changes like this I would strongly recommend the new DevOps section in the docs. In particular, https://docs.conan.io/2/devops/using_conancenter.html#control-and-customization is how you can make changes like this available for yourself!

[pgrossomoreira]

If this patch goes against conan's policy, then fine, let coin-lemon/1.3.1 rot. Eventually nobody will be able to use this package as most people will be using -std=c++20 or later. But can we at least have another package that has the patch for people who aren't stuck on C++11?

It's unreasonable to expect every user that needs coin-lemon to make their own fork of conan-center-index, figure out how to patch this library, create a new conan package and maintain their own instance of artifactory... all because of one small portability patch.

What do you think @prince-chrismc? Would it be okay to create another package in conan-center-index that is almost identical to the existing coin-lemon except it compiles with modern C++? If so, I could try my luck doing so in spite of having near zero prior experience with conan.

Thank you.

[SpaceIm]

Eventually nobody will be able to use this package as most people will be using -std=c++20 or later

The problem is that people may not even uses this library directly but indirectly through another recipe, so they will be stuck (coin-lemon is a dependency of openmvg for example).

I don't think it's a good idea to create another recipe, just allow to merge this PR please, it's not against conancenter policy since portability patches are allowed.

[maksim-petukhov]

@pgrossomoreira

But can we at least have another package that has the patch for people who aren't stuck on C++11?

There is also another way. Conan could add a feature which would allow to conditionally apply patches. For example, patches which allow C++XX compilation of some package would use some special patch_type and they would not be applied by default. But user can override this behavior through command line or conanfile and force to apply patches with this specific special patch_type.

@prince-chrismc

[prince-chrismc]

I am no longer on the Conan team 🙊 but I doubt the answer has changed knowing what I know.

@jcar87 @RubenRBS perhaps you can update this thread? ❤️

[jcar87]

As we continue reviewing our policies, we have taken the decision of raising the bar of entry for any PR that patches C or C++ library code in any way. Conan Center’s intention has always been to provide binary packages built from the source code as provided by the authors, and this is already mentioned in our documentation.


We can and do regularly consider patches for build scripts, as do other package repositories - but patching actual library code is a different can of worms, and something we take really seriously. We have users in enterprise environments that would not find it acceptable if recipes patch upstream code in a way that completely bypasses the validation and scrutiny that those changes would otherwise be subjected to when contributed directly to the library authors.

it doesn't change API, ABI, nor any algorithm of LEMON. It's the straightforward replacement of std::allocator::construct & std::allocator::destroy (deprecated in C++11 and removed in C++20).


I hope you can understand that we need to validate this assertion before approving and merging this. Especially taking into account that the changes proposed in this pull request are NOT being checked by our CI in any way, nor any additional evidence has been provided to ensure that the the behaviour of the affected code is as intended by the library authors. This is an unusual pull request and anything that attempts to modify C++ code in non-trivial ways will be held up by the team until we get a chance to validate it and make a decision.

Eventually nobody will be able to use this package as most people will be using -std=c++20

This is a consequence of the fact that this library is no longer maintained by its authors, and it is not compatible with C++20 and there’s no expectation of when it might be. This is not a case of “this works if I use a different package manager, but not with Conan” - as far as we can see, we are being asked to fixed something that never worked in the first place, and is thus not a regression.

As a user of a library, you need to make a decision as to whether you want C++20 across all translation units, or you want to continue using coin-lemon, and if you want to do both, whether you are comfortable of absorbing the cost of maintaining that possibility. We do not have the resources to maintain libraries where the original authors are no longer doing so. Conan Center has always made every effort to provide libraries “as is” . We do encourage, where possible, for our users to contribute things to the original libraries - it’s a disservice to the community if we are able to fix some things but this never makes it upstream. However, there’s only so much that can be done when a library is abandoned.


We have been unable to identify any other public package repository that adds C++20 compatibility to this library - that is, if you use coin-lemon, or liblemon as it may be known elsewhere, with any other package repository, it will a also not work with C++ 20.


Bear in mind that you can always force specific dependencies to be compiled in a different mode, e.g. you add coin-lemon:compiler.cppstd=14 to the [settings] section of your profile, for example, or passs this via command line options to Conan. You would have to ensure that calling code that includes the affected headers are also using a compatible version of the C++ standard. If you are using CMake you can set the C++ standard of specific source files or targets, and it is still likely to work when the rest of your code and dependencies are in C++ 20 mode. If you have sample code, please let us know if you need assistance in setting this up.


We are approving this PR after evaluating the proposed changes - however, and for avoidance of doubt, we hugely discourage further contributions that go in the same direction, even if such contributions have been considered in the past. Conan Center itself cannot be used as a proxy to maintain forks of libraries that are no longer maintained by their authors. We will make exceptions whenever we feel there’s enough evidence that the proposed changes have sufficient scrutiny and validation.

.

[SpaceIm]

We can and do regularly consider patches for build scripts, as do other package repositories - but patching actual library code is a different can of worms, and something we take really seriously.

There are still many recent PRs with patches of source code itself without any blocker, so why this PR specifically?

[SpaceIm]

Thanks

[pgrossomoreira]

Thank you!