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

Add geographiclib/1.51 #4291

Merged
merged 2 commits into from
Jan 20, 2021

Conversation

hdhauk
Copy link
Contributor

@hdhauk hdhauk commented Jan 18, 2021

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

@conan-center-bot
Copy link
Collaborator

Failure in build 1 (36b80c68c372acd1934de7e8caada498ebd6bb0e):

  • geographiclib/1.50.1
    • Hooks errors detected:
      • [HOOK - conan-center.py] pre_export(): ERROR: [CONFIG.YML HAS NEW VERSION (KB-H052)] The version "1.51" exists in "conandata.yml" but not in "../config.yml", so it will not be built. Please update "../config.yml" to include newly added version "1.51". (https://github.com/conan-io/conan-center-index/blob/master/docs/error_knowledge_base.md#KB-H052)

@conan-center-bot
Copy link
Collaborator

Some configurations of 'geographiclib/1.51' failed in build 2 (ff5e8d81197509f5d57c6785af8698c268e5e2a7):

@hdhauk hdhauk force-pushed the update-geographiclib-1.51 branch from ff5e8d8 to 990252a Compare January 18, 2021 09:29
@conan-center-bot conan-center-bot removed the Bump version PR bumping version without recipe modifications label Jan 18, 2021
@SpaceIm
Copy link
Contributor

SpaceIm commented Jan 18, 2021

You have also to raise ConanInvalidConfiguration if version >= 1.51 and clang < 4 I guess.

@conan-center-bot
Copy link
Collaborator

All green in build 3 (990252a07256c43f4ad56c0ed7704398cf8d0c06)! 😊

@hdhauk
Copy link
Contributor Author

hdhauk commented Jan 18, 2021

You have also to raise ConanInvalidConfiguration if version >= 1.51 and clang < 4 I guess.

I've added a tools.check_min_cppstd(self, 11), wouldn't that suffice?

@SpaceIm
Copy link
Contributor

SpaceIm commented Jan 18, 2021

No, because it prevents to build with compilers versions for which default C++ standard is not 11 or higher (and cppstd 11+ not set in profile), like gcc 4.9 or 5.

cppstd check should be:

        if tools.Version(self.version) >= "1.51" and self.settings.compiler.get_safe("cppstd"):
            tools.check_min_cppstd(self, 11)

Then raise for specific compilers versions.

@dmn-star
Copy link
Contributor

dmn-star commented Jan 18, 2021

You can adopt _min_compiler_version_default_cxx11 from the boost recipe
https://github.com/conan-io/conan-center-index/pull/3977/files/15e9b1be20a61892d5958ed494ab74a9d7b7b333

@hdhauk hdhauk force-pushed the update-geographiclib-1.51 branch from 990252a to e38bec0 Compare January 18, 2021 12:27
@conan-center-bot
Copy link
Collaborator

Failure in build 4 (e38bec0174cef4096ecd3d69b86ab1fd19e17a93):

  • Error processing recipe (ref 'geographiclib/1.51'): Linux x86_64, Release, gcc 4.9, libstdc++ . Options: geographiclib:shared-False
    Unexpected error happened:
ERROR: geographiclib/1.51: Error in configure() method, line 43
	raise ConanException("C++11 support needed for version >= 1.51")
	ConanException: C++11 support needed for version >= 1.51

@hdhauk hdhauk force-pushed the update-geographiclib-1.51 branch from e38bec0 to 81df2f5 Compare January 18, 2021 12:39
@conan-center-bot
Copy link
Collaborator

Failure in build 5 (81df2f5f54899b4dc7173a199ecc1b3af166fb2b):

  • Error processing recipe (ref 'geographiclib/1.51'): Macos x86_64, Release, apple-clang 9.1, libc++ . Options: geographiclib:shared-False
    Unexpected error happened:
ERROR: geographiclib/1.51: Error in configure() method, line 42
	elif tools.Version(self.settings.compiler.version) < self._min_compiler_version_default_cxx11:
	ConanException: Invalid version 'None'

Comment on lines 31 to 33
"gcc": 6,
"clang": 6,
"Visual Studio": 14, # guess
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"gcc": 6,
"clang": 6,
"Visual Studio": 14, # guess
"gcc": 4.9,
"clang": 6,
"Visual Studio": 14, # guess

I'm able to build with gcc 4.9 and 5

Copy link
Contributor

Choose a reason for hiding this comment

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

"gcc": 4.9 -> "gcc": 6.1
The default mode is C++98 for GCC versions prior to 6.1, and C++14 for GCC 6.1 and above.

Copy link
Contributor

Choose a reason for hiding this comment

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

It supports C++11 even if it's not the default.

Copy link
Contributor

@dmn-star dmn-star Jan 19, 2021

Choose a reason for hiding this comment

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

it doesn't matter, the important thing is that it has only c++03 default mode.

Also c++11 support in gcc 4.9 is quite buggy. And anyway, what's the point of using almost EOL compiler?

  1. Use compiler.cppstd if set ( On CCI cppstd is not set anywhere)
  2. Use C++ standard compiler default mode, if cppstd not set

Upvote conan-io/conan#8304 :-)

Copy link
Contributor

@SpaceIm SpaceIm Jan 19, 2021

Choose a reason for hiding this comment

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

Most underlying build files of open source libraries don't follow default standard, they force any standard allowing to properly build if user didn't ask for a specific one.
So if cppstd is not set, I think that it should not be default standard of the compiler, but any standard supported by the compiler and allowing to build the library. It's the behaviour currently implemented in all CCI recipes.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a different issue. Like most libraries, geographiclib doesn't conditionnally disable build of components depending on C++ standard, boost is an edge case.

Copy link
Contributor

@dmn-star dmn-star Jan 19, 2021

Choose a reason for hiding this comment

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

It's a different issue.

for the same reason. apple-clang (with c++17 support) was used in C++03 default mode to build the boost.

conditionnally disable build of components depending on C++ standard, boost is an edge case.

but what about the conditional compilation depending on C++ standard or other c++ funny things?

Why not follow the official Conan guide ,don't modify the c++ standard in recipe, bump gcc to 6.1 and profit. :-)

Copy link
Contributor

@SpaceIm SpaceIm Jan 19, 2021

Choose a reason for hiding this comment

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

There is no official Conan guide about cppstd AFAIK, and cppstd in profile is experimental.

Visual Studio 2019 default standard is C++14 for example. Do you want to not provide or test in CCI binaries for Visual Studio if library requires C++17?

Copy link
Contributor

@dmn-star dmn-star Jan 19, 2021

Choose a reason for hiding this comment

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

VS2019 IDE pass the c++14 flag to cl.exe by default. :-) Xcode 12 does for apple-clang too but CCI uses apple-clang in C++03 mode on the command line.

Do you want to not provide or test in CCI binaries for Visual Studio if library requires C++17?

Why not, if the underlying build system do right thing ( cmake for example target_compile_features(xxxx PUBLIC cxx_std_17)? But not by force cppstd in the recipe.

There is no official Conan guide about cppstd AFAIK, and cppstd in profile is experimental.

IMHO https://docs.conan.io/en/latest/howtos/manage_cpp_standard.html

No matter simply never use the binaries generated by Conan in the production.

Copy link
Contributor

@SpaceIm SpaceIm Jan 19, 2021

Choose a reason for hiding this comment

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

Do you want to not provide or test in CCI binaries for Visual Studio if library requires C++17?

Why not, if the underlying build system do right thing ( cmake for example target_compile_features(xxxx PUBLIC cxx_std_17)? But not by force cppstd in the recipe.

Yes and it's what most recipes do (including this one, recipe itself doesn't inject C++11 at all) , they rely on underlying build system to set a minimum standard if default one is not sufficient, so they don't honor default standard of compiler.

Copy link
Contributor

@SpaceIm SpaceIm left a comment

Choose a reason for hiding this comment

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

Don't raise for compilers you have not listed in _min_compiler_version_default_cxx11()

"gcc": 6,
"clang": 6,
"Visual Studio": 14, # guess
}.get(str(self.settings.compiler))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
}.get(str(self.settings.compiler))
}.get(str(self.settings.compiler), False)

Comment on lines 42 to 43
elif tools.Version(self.settings.compiler.version) < self._min_compiler_version_default_cxx11:
raise ConanInvalidConfiguration("C++11 support needed for version >= 1.51")
Copy link
Contributor

@SpaceIm SpaceIm Jan 18, 2021

Choose a reason for hiding this comment

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

Suggested change
elif tools.Version(self.settings.compiler.version) < self._min_compiler_version_default_cxx11:
raise ConanInvalidConfiguration("C++11 support needed for version >= 1.51")
def lazy_lt_semver(v1, v2):
lv1 = [int(v) for v in v1.split(".")]
lv2 = [int(v) for v in v2.split(".")]
min_length = min(len(lv1), len(lv2))
return lv1[:min_length] < lv2[:min_length]
minimum_version = self._min_compiler_version_default_cxx11
if not minimum_version:
self.output.warn("geographiclib {} requires C++11 math functions. Your compiler is unknown. Assuming it supports this feature.".format(self.version))
elif lazy_lt_semver(str(self.settings.compiler.version), minimum_version):
raise ConanInvalidConfiguration("geographiclib {} requires C++11 math functions, which your compiler does not support.".format(self.version))

@conan-center-bot
Copy link
Collaborator

Some configurations of 'geographiclib/1.51' failed in build 6 (3a5488d0a3ff4950cbd456b605a4a5726ff08836):

@ghost
Copy link

ghost commented Jan 18, 2021

I detected other pull requests that are modifying geographiclib/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.

@hdhauk hdhauk force-pushed the update-geographiclib-1.51 branch from 3a5488d to 239e31b Compare January 18, 2021 18:10
@conan-center-bot
Copy link
Collaborator

Failure in build 7 (239e31bcfe803d6d6a51c0ef523e352dfeb236f4):

  • Error processing recipe (ref 'geographiclib/1.50.1'): Linux x86_64, Release, gcc 4.9, libstdc++ . Options: geographiclib:shared-True
    Unexpected error happened:
ERROR: 400: Unsupported Conan v2 repository request for 'c3i_PR-4291_239e31bcfe803d6d6a51c0ef523e352dfeb236f4'. [Remote: c3i_PR-4291_239e31bcfe803d6d6a51c0ef523e352dfeb236f4]

@hdhauk
Copy link
Contributor Author

hdhauk commented Jan 19, 2021

Failure in build 7 (239e31bcfe803d6d6a51c0ef523e352dfeb236f4):

* **Error processing recipe (ref 'geographiclib/1.50.1'): Linux x86_64, Release, gcc 4.9, libstdc++ . Options:  geographiclib:shared-True**
  Unexpected error happened:
ERROR: 400: Unsupported Conan v2 repository request for 'c3i_PR-4291_239e31bcfe803d6d6a51c0ef523e352dfeb236f4'. [Remote: c3i_PR-4291_239e31bcfe803d6d6a51c0ef523e352dfeb236f4]

Does anyone know what might have gone wrong here? Clicking the details link gives me a 403 error. @SpaceIm @dmn-star

@dmn-star
Copy link
Contributor

try to close the PR and reopen it after ~20--30 sec.

Comment on lines 57 to 66
if tools.Version(self.version) >= "1.51":
if self.settings.compiler.get_safe("cppstd"):
tools.check_min_cppstd(self, 11)
elif self._compiler_supports_cxx11:
self._cmake.definitions["CONAN_CMAKE_CXX_STANDARD"] = 11
else:
raise ConanInvalidConfiguration("C++11 support needed for version >= 1.51")

self._cmake.definitions["GEOGRAPHICLIB_LIB_TYPE"] = "SHARED" if self.options.shared else "STATIC"
self._cmake.configure(build_folder=self._build_subfolder)
Copy link
Contributor

Choose a reason for hiding this comment

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

useless ;) (and the last 2 lines have lost one level of indentation)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops! I'll fix that indentation error. Am I missing something tho? Wouldn't this achieve what we want? The first case will make sure we get a ConanInvalidConfiguration exception if settings.compiler.cppstd is set too low, and the second will make sure we use C++11 if it isn't set and the compiler supports it.

Copy link
Contributor

@SpaceIm SpaceIm Jan 19, 2021

Choose a reason for hiding this comment

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

It's already handled in underlying build files. Your errors come from test_package, you need to set CXX_STANDARD 11.

I've proposed modifications far above, handling all this thing in a "graceful" way (and used in most others recipes with this kind of issue), you wan still use them ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright! Thanks :) I'll try to make those changes then

@hdhauk hdhauk force-pushed the update-geographiclib-1.51 branch from 239e31b to 5a44f47 Compare January 19, 2021 15:30
@@ -8,6 +8,7 @@ class TestPackageConan(ConanFile):
def build(self):
cmake = CMake(self)
cmake.configure()
cmake.definitions["CONAN_CMAKE_CXX_STANDARD"] = 11
Copy link
Contributor

@SpaceIm SpaceIm Jan 19, 2021

Choose a reason for hiding this comment

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

Suggested change
cmake.definitions["CONAN_CMAKE_CXX_STANDARD"] = 11

and add those lines at the end of test_package's CMakeLists.txt:

if(geographiclib_VERSION VERSION_GREATER_EQUAL "1.51")
  set_property(TARGET ${PROJECT_NAME} PROPERTY CXX_STANDARD 11)
endif()

self._cmake.definitions["GEOGRAPHICLIB_LIB_TYPE"] = "SHARED" if self.options.shared else "STATIC"
self._cmake.configure(build_folder=self._build_subfolder)
return self._cmake
return self._cmake
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return self._cmake
return self._cmake

undo modifications

@conan-center-bot
Copy link
Collaborator

Some configurations of 'geographiclib/1.50.1' failed in build 8 (5a44f4754cb3cf31594dbbec33bfd8ae543118d1):

@hdhauk hdhauk force-pushed the update-geographiclib-1.51 branch from 5a44f47 to f6b2586 Compare January 19, 2021 16:02
@conan-center-bot
Copy link
Collaborator

All green in build 9 (f6b25866f901d2d7dbc5ad0e0e797e2598864505)! 😊

if self.settings.compiler.cppstd:
tools.min_cppstd(self, 11)

def lazy_lt_semver(v1, v2):
Copy link
Contributor

Choose a reason for hiding this comment

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

Another variant conan-io/conan#8002 (just to keep track of all of them)

@SSE4 SSE4 requested a review from uilianries January 20, 2021 06:30
@conan-center-bot conan-center-bot merged commit eb27840 into conan-io:master Jan 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants