libkml: add a patch for C++17 compatibility

[valgur]

libkml/libkml#275

[ghost]

I detected other pull requests that are modifying libkml/all recipe:

• libkml: fall back to C++14 #23755
• libkml: use version range for expat, limit cpp standard to c++14 #23665

This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there.

[conan-center-bot]

Conan v1 pipeline ✔️

All green in build 2 (c2ccc777e9c6f19233bb47c1d54cf7dbec0713f0):

• libkml/1.3.0:
  All packages built successfully! (All logs)

---

Conan v2 pipeline ✔️

Note: Conan v2 builds are now mandatory. Please read our discussion about it.

All green in build 2 (c2ccc777e9c6f19233bb47c1d54cf7dbec0713f0):

• libkml/1.3.0:
  All packages built successfully! (All logs)

[AbrilRBS]

Thanks! Feel free to ping me once upstream ahs approved this :)

[valgur]

@RubenRBS I'm doubtful the project/fork is actually being maintained. The last release was in 2015.

reason: we don't accept patches for unmaintained libraries

[jcar87]

@RubenRBS I'm doubtful the project/fork is actually being maintained. The last release was in 2015.

Thanks @valgur for looking into this.
As per our policy, we do not accept patches to C or C++ where upstream might not. If this is an unmaintained library that does not support C++17, then it should raise an the InvalidConfiguration exception when cppstd=17. We provide libraries built from their sources as-is. By plugging fixes that the upstream maintainers won't, this essentially makes the Conan team responsible for the ongoing maintenance of a library fork.

In general, if the change truly is trivial, please provide more context in the PR description to support this claim. Opening a PR upstream is unfortunately not enough, especially if the authors are known to not respond. We do review upstream PRs/issues and require that the upstream maintainers acknowledge, comment, and confirm whether the proposed solution is what they would include in future versions.

Changing the base class of a C++ class or struct has side effects in the ABI. Conan will not cause a rebuild of consumers_ of libkml/1.3.0 unless the major or minor components have changed, or other settings have. So the change proposed in this PR could cause link errors in consumers, provided some conditions were met. I think in this case this would typically be that an public function accepts an object (by value) of the GetElement struct. It's unclear to me whether that is the case, but we are not familiar with the codebase to make a call on this.

[valgur]

@jcar87 Fair enough.

I would indeed argue that this change is trivial and should not have any effects on the ABI, API or the behavior of the library. The GetElement functor is used only locally in an std::for_each() and is not declared in any headers, so it won't be linked against even in the same project let alone external libs.

Here's the single usage:
https://github.com/libkml/libkml/blob/916a801ed3143ab82c07ec108bad271aa441da16/src/kml/xsd/xsd_file.cc#L51-L69

[jcar87]

On which compiler is this a problem?
The last time this recipe built on CI, it passed with C++17 on both Linux and macOS (with gcc and apple-clang).
https://c3i.jfrog.io/c3i/misc-v2/summary.html?json=https://c3i.jfrog.io/c3i/misc-v2/logs/pr/20193/1/libkml/1.3.0//summary.json

[ericLemanissier]

on msvc : (building current master recipe) https://github.com/eirikb/proof-of-conan/actions/runs/8830426628/job/24243544036#step:11:521

That said this PR is not sufficient to make it pass : https://github.com/eirikb/proof-of-conan/actions/runs/8830578071/job/24244034540#step:11:506 convenience\feature_list.cc(125,17): error C2039: 'binary_function': is not a member of 'std'

[jcar87]

Thanks Eric.

It's a similar situation with coin-lemon - where the PR claiming support for C++20 compatibility was not actually complete.
That is the risk with patches like this - it may fix "something" but we cannot foresee all uses. We don't expect contributors to test all configurations, but it would be greatly helpful for contributors to tell us in the PR:

• which configuration this was failing on (that motivated the PR in the first place)
• where this was tested
  We will verify these claims ourselves, whenever it is feasible, especially to cover any configuration that is not currently covered by CI.

On the other hand:

• when referencing an issue that is reported upstream, we will verify that the library maintainers have replied back - if they haven't, we are very unlikely to accepted a backport
• the information about how this may be low risk due to this being only an issue when creating the package, but not when consuming it, would have been useful in the description as well - we are not familiar with the codebases of all recipes in Conan Center, so anything to support the PR is expected in the description.

So that we can manually verify any configuration that is not currently covered by CI.

For libraries that fall under "cannot be built with higher C++ standard, but can be consumed with any", the recommended approach is to simply reflect this in validate_build():

def validate_build(self):
     check_max_cppstd(self, "14")

When consuming with compiler.cppstd=17 or higher, compatibility.py will do the rest - and consider the binary built with C++14 - this will work fine. I've opened an issue in the Conan client conan-io/conan#16148 - to see if we can have a similar fallback without user intervention.

In the meantime, after checking with the team, we are proposing #23755 to supersede this PR.

[valgur]

would have been useful in the description as well

I'll start actually updating descriptions when you fix #18780.

[jcar87]

would have been useful in the description as well

I'll start actually updating descriptions when you fix #18780.

Please keep in mind that the team may not review, prioritise and should certainly not merge PRs with incomplete information.

Even if updating the description results in a new run, the packages are not rebuilt if the commit hasn't changed - it reruns the tests, which should be faster.

[valgur]

@jcar87 The need for more context info in the description is usually proportional to the size of the PR, which in turn usually depends on the complexity of the recipe and the project. And the more complex PRs usually need incremental updates as more bugs and edge cases get uncovered. I guess I'll just re-trigger a multi-hour CI run each time in these cases then?

Even if updating the description results in a new run, the packages are not rebuilt if the commit hasn't changed - it reruns the tests, which should be faster.

Is that really the case? Based on my experience it re-runs both the build and test_package steps.

[jcar87]

@jcar87 The need for more context info in the description is usually proportional to the size of the PR, which in turn usually depends on the complexity of the recipe and the project.

I would say this PR is a good example of that not being the case. That's why we found it confusing knowing that there are binaries published where C++17 was actually used - we don't know where this fails, why, and if the fixes proposed actually fix what the title is claiming to fix, which turned out not to be the case. Having the information at hand will help the review team get to this quicker.

[jcar87]

However this is a distraction. We are asking to provide this information when opening the PR.