Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

coin-lemon: fixed C++17 and C++20 compilation warnings. #15516

Closed
wants to merge 6 commits into from

Conversation

rturrado
Copy link
Contributor

Specify library name and version: coin-lemon/1.3.1

Fixes issue #15515.

I've recently been playing with a project that is using C++11 and coin-lemon/1.3.1, which is the latest official release of this library. Changing the project's C++ standard to C++23 raises a few compiler warnings, some of them coming from coin-lemon. This commit adds two patches to the coin-lemon recipe that remove the compilation warnings. I have only tried compiling with gcc 12.2.0 on Linux.


@CLAassistant
Copy link

CLAassistant commented Jan 28, 2023

CLA assistant check
All committers have signed the CLA.

@conan-center-bot

This comment has been minimized.

@uilianries
Copy link
Member

@rturrado Thank you for your patch, could you please send it directly to the upstream? It seems be a portability patch which makes more sense there than here, otherwise, we would need to update it for each new version.

@rturrado
Copy link
Contributor Author

rturrado commented Jan 31, 2023

@uilianries Thanks for the comment! May I ask what do you mean by 'send it directly to the upstream'? Do you mean to send the patch to whoever is maintaining coin-lemon?

I initially thought that's what I should do so, yes. And it should be the library owners who updated the library, gathering the latest patches into a new version, and then created a new conan package with that version.

I tried to create a conan package myself because I saw they hadn't created a new version of the library since 2014 (even though they have been publishing patches until 2021).

Please confirm if I understood you correctly and I will proceed as you suggest.

Thanks again.

@uilianries
Copy link
Member

@rturrado Correct, sending to the authors.

I saw they hadn't created a new version of the library since 2014 (even though they have been publishing patches until 2021).

Sorry, I didn't check it. In that case, let's add here, but please, don't forget to send to authors too. I also commented some updates to be added to conandata.yml

Thank you for your contribution!

@conan-center-bot

This comment has been minimized.

…20_compilation_errors.patch, and updated conandata.yml as suggested in the pull request review.
@rturrado
Copy link
Contributor Author

rturrado commented Feb 2, 2023

I've seen that my changes seem to have broken a compilation with gcc 5.

How can I know all the different combinations a package is built for? And how can I test that locally before uploading?

Thanks, and sorry for the inconveniences I may be causing!

@conan-center-bot

This comment has been minimized.

@uilianries
Copy link
Member

How can I know all the different combinations a package is built for?

https://github.com/conan-io/conan-center-index/blob/master/docs/supported_platforms_and_configurations.md

And how can I test that locally before uploading?

https://github.com/conan-io/conan-center-index/blob/master/docs/developing_recipes_locally.md#testing-more-environments

Indeed there are many environments, but you don't need to test all them before pushing if you feel safe.

@conan-center-bot

This comment has been minimized.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 5, 2023

Hooks produced the following warnings for commit 788b028
coin-lemon/1.3.1
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library './lib/libemon.so' links to system library 'm' but it is not in cpp_info.system_libs.

@rturrado rturrado requested a review from uilianries February 8, 2023 23:03
uilianries
uilianries previously approved these changes Feb 9, 2023
@ghost
Copy link

ghost commented Feb 9, 2023

I detected other pull requests that are modifying coin-lemon/all recipe:

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

@@ -6,3 +6,9 @@ patches:
"1.3.1":
- patch_file: "patches/cmake-add-runtime-destination_1.3.1.patch"
- patch_file: "patches/0001-remove-register-keyword.patch"
- patch_file: "patches/fix_cpp_17_compilation_warnings.patch"
patch_type: "portability"
patch_description: "Fix C++17 compilation warnings regarding the use of the deprecated std::iterator"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/conan-io/conan-center-index/blob/master/docs/adding_packages/sources_and_patches.md#rules

I am not sure the warnings deserve a patch. These are costly to maintain and we really want ones to make "working software"

Does this fail to build with later cppstd? I would prefer less patches otherwise

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @prince-chrismc ,

I'm more in favour of a 'treatiing warnings as errors' policy. Anyway, yes, some of these warnings cause failures when compiling with C++17 or newer (see #15515 ).

Best regards,
Roberto.

patch_description: "Fix C++17 compilation warnings regarding the use of the deprecated std::iterator"
- patch_file: "patches/fix_cpp_20_compilation_errors.patch"
patch_type: "portability"
patch_description: "Fix C++20 compilation errors regarding the use of allocator.construct and allocator.destroy"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the patch_source too with the PR to upstream were you submitted this 🙏 so others can benefit from it too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @prince-chrismc ,

I tried to get in touch with the maintainers of the Lemon Graph library via their website and their user mailing list, without luck. The last version of the library is from 2014.

I've issued a new ticket, and attached the first of the patches (C++17), to: https://lemon.cs.elte.hu/trac/lemon/attachment/ticket/680/fix_cpp_17_compilation_warnings.patch
The second patch (C++20) is actually part of this other attachment to a previous ticket (I only took the patch section for array_map.h because the patch section for path.h wasn't patching correctly): http://lemon.cs.elte.hu/trac/lemon/attachment/ticket/631/93c983122692.patch

I think I should update this second patch with the fixes for path.h as well.

If you reckon, I can add those two links mentioned above as patch_source.

Thanks,
Roberto.

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ❌

Failure in build 6 (a94d7b1fe8cb38927eab847915cc2113710bde1d):

  • coin-lemon/1.3.1@:
    CI failed to create some packages (All logs)

    Logs for packageID 42a10e89ce278d2cf0fa1c09abdd40b137b6e79b:
    [settings]
    arch=armv8
    build_type=Debug
    compiler=apple-clang
    compiler.libcxx=libc++
    compiler.version=13.0
    os=Macos
    [options]
    coin-lemon:shared=False
    
    [...]
    Configuration (profile_host):
    [settings]
    arch=armv8
    build_type=Debug
    compiler=apple-clang
    compiler.libcxx=libc++
    compiler.version=13.0
    os=Macos
    [options]
    coin-lemon:shared=False
    [build_requires]
    [env]
    [conf]
    tools.system.package_manager:mode=install
    tools.system.package_manager:sudo=True
    tools.apple:sdk_path=/Applications/conan/xcode/13.0/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.3.sdk
    
    Configuration (profile_build):
    [settings]
    arch=x86_64
    build_type=Debug
    compiler=apple-clang
    compiler.libcxx=libc++
    compiler.version=13.0
    os=Macos
    [options]
    coin-lemon:shared=False
    [build_requires]
    [env]
    [conf]
    tools.system.package_manager:mode=install
    tools.system.package_manager:sudo=True
    tools.apple:sdk_path=/Applications/conan/xcode/13.0/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.3.sdk
    
    coin-lemon/1.3.1: Forced build from source
    Installing package: coin-lemon/1.3.1
    Requirements
        coin-lemon/1.3.1 from local cache - Cache
    Packages
        coin-lemon/1.3.1:42a10e89ce278d2cf0fa1c09abdd40b137b6e79b - Build
    
    Cross-build from 'Macos:x86_64' to 'Macos:armv8'
    Installing (downloading, building) binaries...
    [HOOK - conan-center.py] pre_source(): [IMMUTABLE SOURCES (KB-H010)] OK
    coin-lemon/1.3.1: Configuring sources in /Users/jenkins/w/prod/BuildSingleReference@4/.conan/data/coin-lemon/1.3.1/_/_/source/src
    coin-lemon/1.3.1: Waiting 5 seconds to retry...
    coin-lemon/1.3.1: ERROR: Error downloading file http://lemon.cs.elte.hu/pub/sources/lemon-1.3.1.tar.gz: 'HTTPConnectionPool(host='lemon.cs.elte.hu', port=80): Max retries exceeded with url: /pub/sources/lemon-1.3.1.tar.gz (Caused by NewConnectionError('<urllib3.connection.HTTPConnection object at 0x104fe7650>: Failed to establish a new connection: [Errno 8] nodename nor servname provided, or not known'))'
    ERROR: coin-lemon/1.3.1: Error in source() method, line 44
    	destination=self.source_folder, strip_root=True)
    	ConanException: Error downloading file http://lemon.cs.elte.hu/pub/sources/lemon-1.3.1.tar.gz: 'HTTPConnectionPool(host='lemon.cs.elte.hu', port=80): Max retries exceeded with url: /pub/sources/lemon-1.3.1.tar.gz (Caused by NewConnectionError('<urllib3.connection.HTTPConnection object at 0x104f968d0>: Failed to establish a new connection: [Errno 8] nodename nor servname provided, or not known'))'
    

Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability.


Conan v2 pipeline (informative, not required for merge) ❌

Note: Conan v2 builds are informative and they are not required for the PR to be merged.

The v2 pipeline failed. Please, review the errors and note this will be required for pull requests to be merged in the near future.

See details:

Failure in build 5 (a94d7b1fe8cb38927eab847915cc2113710bde1d):

  • coin-lemon/1.3.1@:
    Error running command conan graph info --requires coin-lemon/1.3.1@#163a480a1ec7e12c4949a9a556f9beec -f json -pr:h /home/conan/w/prod-v2_cci_PR-15516/5/c779839f-b427-402f-badb-3b5bef3bbab8/profile_windows_192_release_dynamic_msvc_release_64.-shared-True.txt -pr:b /home/conan/w/prod-v2_cci_PR-15516/5/c779839f-b427-402f-badb-3b5bef3bbab8/profile_windows_192_release_dynamic_msvc_release_64.-shared-True.txt:
    -------- Input profiles --------
    Profile host:
    [settings]
    arch=x86_64
    build_type=Release
    compiler=msvc
    compiler.runtime=dynamic
    compiler.runtime_type=Release
    compiler.version=192
    os=Windows
    [options]
    */*:shared=True
    
    Profile build:
    [settings]
    arch=x86_64
    build_type=Release
    compiler=msvc
    compiler.runtime=dynamic
    compiler.runtime_type=Release
    compiler.version=192
    os=Windows
    [options]
    */*:shared=True
    
    
    -------- Computing dependency graph --------
    ERROR: coin-lemon/1.3.1: Error in configure() method, line 37
    	del self.options.fPIC
    	ConanException: option 'fPIC' doesn't exist
    Possible options are ['shared']
    

Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability.

@stale
Copy link

stale bot commented Mar 12, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale
Copy link

stale bot commented Apr 25, 2023

This pull request has been automatically closed because it has not had recent activity. Thank you for your contributions.

@stale stale bot closed this Apr 25, 2023
@SpaceIm SpaceIm mentioned this pull request May 1, 2023
3 tasks
@SpaceIm
Copy link
Contributor

SpaceIm commented May 1, 2023

@rturrado do you still work on this one? I've hit this issue in coin-lemon while trying to compile OpenMVG with C++20 (#16356 (comment)).

EDIT: I've opened #17338

@rturrado
Copy link
Contributor Author

rturrado commented Jun 5, 2023

Sorry for the late reply, @SpaceIm. I had abandoned this Conan package for a while. I'm glad you're trying to fix it in a new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants