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 package cpplogging #14043

Closed
wants to merge 6 commits into from
Closed

Conversation

EmilienBINET
Copy link
Contributor

Specify library name and version: cpplogging/1.0.3.0
requires cppcommon/1.0.3.0
the previous versions (cpplogging/1.0.0.0 and cpplogging/1.0.1.0) require cppcommon/1.0.2.0

  • 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

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@EmilienBINET EmilienBINET marked this pull request as draft November 11, 2022 17:21
@conan-center-bot

This comment has been minimized.

@EmilienBINET EmilienBINET marked this pull request as ready for review November 13, 2022 09:03
@EmilienBINET
Copy link
Contributor Author

Build currently fails because version 1.0.3.0 requires cppcommon/1.0.3.0 to be merged (PR #14038)

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

Copy link
Contributor

@prince-chrismc prince-chrismc left a comment

Choose a reason for hiding this comment

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

This is amazing work but it's a touch behind our best practices and current recommendations -- i'd like to encourage you to look at the cmake template we have to maybe update this so it's not using deprecated features

Also we typically dont add older point releases unless they are specific required.

Id suggest keeping on the latest -- they are very similar so I would not imagine you need an older point release

@@ -0,0 +1,100 @@
from conans import CMake
Copy link
Contributor

Choose a reason for hiding this comment

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

Given this is a new recipe I would be really appreciated if you used the newer build helpers

This is deprecated and will be removed soon... you can see the template for a good example

from conan.tools.cmake import CMake, CMakeDeps, CMakeToolchain, cmake_layout

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the template, I'll look at it.
I left that one line because I struggled a bit when I tried to remove it and could not make the recipe to work.

recipes/cpplogging/all/conanfile.py Show resolved Hide resolved
del self.options.fPIC

if self.settings.compiler.get_safe("cppstd"):
build.check_min_cppstd(self, "17")
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking over the project is does not say this and the GitHub actions build without it so I am surprsed to see this here 🤔

Can you share why you are adding this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The project uses std::string_view which was added to the stl in C++17.

Copy link
Member

Choose a reason for hiding this comment

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

fair enough, but it should be pointed by the upstream, there is no reference about C++ standard there.

Copy link
Member

Choose a reason for hiding this comment

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

I just created the PR chronoxor/CppLogging#5 to enforce the standard as requirement.

Co-authored-by: Chris Mc <prince.chrismc@gmail.com>
@EmilienBINET
Copy link
Contributor Author

Thanks for the compliment, but the credit goes to the one that made the cppcommon and cppserver recipes.
I used them as template as the creator of those 3 libraries uses the same way to code his projects.

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline

All green in build 7 (6395f842bbf5dd96803a79fd2f2f64cddfed2285):

  • cpplogging/1.0.0.0@:
    All packages built successfully! (All logs)

    🔸 Informative: This recipe is not ready for Conan v2

    We have started the migration process to Conan v2 and exporting recipes successfully will be required in the future.
    This is just an informative note to gain awareness about the process, no need to take any action. The plan is to enforce smaller steps that are easier to fix and, eventually, this conan export step will work.
    See the recipe migration guide to know more about the changes required.

    ERROR: Error loading conanfile at '/home/conan/w/prod_cci_PR-14043/recipes/cpplogging/all/conanfile.py': Unable to load conanfile in /home/conan/w/prod_cci_PR-14043/recipes/cpplogging/all/conanfile.py
      File "<frozen importlib._bootstrap_external>", line 728, in exec_module
      File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
      File "/home/conan/w/prod_cci_PR-14043/recipes/cpplogging/all/conanfile.py", line 1, in <module>
        from conans import CMake
    ImportError: cannot import name 'CMake' from 'conans' (/opt/pyenv/versions/3.7.13/lib/python3.7/site-packages/conans/__init__.py)
    
  • cpplogging/1.0.1.0@:
    All packages built successfully! (All logs)

    🔸 Informative: This recipe is not ready for Conan v2

    We have started the migration process to Conan v2 and exporting recipes successfully will be required in the future.
    This is just an informative note to gain awareness about the process, no need to take any action. The plan is to enforce smaller steps that are easier to fix and, eventually, this conan export step will work.
    See the recipe migration guide to know more about the changes required.

    ERROR: Error loading conanfile at '/home/conan/w/prod_cci_PR-14043/recipes/cpplogging/all/conanfile.py': Unable to load conanfile in /home/conan/w/prod_cci_PR-14043/recipes/cpplogging/all/conanfile.py
      File "<frozen importlib._bootstrap_external>", line 728, in exec_module
      File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
      File "/home/conan/w/prod_cci_PR-14043/recipes/cpplogging/all/conanfile.py", line 1, in <module>
        from conans import CMake
    ImportError: cannot import name 'CMake' from 'conans' (/opt/pyenv/versions/3.7.13/lib/python3.7/site-packages/conans/__init__.py)
    
  • cpplogging/1.0.3.0@:
    All packages built successfully! (All logs)

    🔸 Informative: This recipe is not ready for Conan v2

    We have started the migration process to Conan v2 and exporting recipes successfully will be required in the future.
    This is just an informative note to gain awareness about the process, no need to take any action. The plan is to enforce smaller steps that are easier to fix and, eventually, this conan export step will work.
    See the recipe migration guide to know more about the changes required.

    ERROR: Error loading conanfile at '/home/conan/w/prod_cci_PR-14043/recipes/cpplogging/all/conanfile.py': Unable to load conanfile in /home/conan/w/prod_cci_PR-14043/recipes/cpplogging/all/conanfile.py
      File "<frozen importlib._bootstrap_external>", line 728, in exec_module
      File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
      File "/home/conan/w/prod_cci_PR-14043/recipes/cpplogging/all/conanfile.py", line 1, in <module>
        from conans import CMake
    ImportError: cannot import name 'CMake' from 'conans' (/opt/pyenv/versions/3.7.13/lib/python3.7/site-packages/conans/__init__.py)
    

Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

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

Thank you for your contributions, please, consider ConanCenterIndex templates in the future, they are more aligned with new features and will save your time when writing a new recipe from scratch.

"1.0.1.0":
url: "https://github.com/chronoxor/CppLogging/archive/refs/tags/1.0.1.0.tar.gz"
sha256: "46247873f8137ff1511222a85f34a8d54d5f804a93e3f04c6476bf61d8db7113"
"1.0.3.0":
Copy link
Member

Choose a reason for hiding this comment

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

Do you really need all these three versions? Otherwise, please, keep only the latest. We want to avoid more and more versions which are outdated and not downloaded. They cost CI time, resources and storage.

del self.options.fPIC

if self.settings.compiler.get_safe("cppstd"):
build.check_min_cppstd(self, "17")
Copy link
Member

Choose a reason for hiding this comment

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

fair enough, but it should be pointed by the upstream, there is no reference about C++ standard there.

def _configure_cmake(self):
if not self._cmake:
self._cmake = CMake(self)
self._cmake.definitions["CPPLOGGING_MODULE"] = "OFF"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self._cmake.definitions["CPPLOGGING_MODULE"] = "OFF"
self._cmake.definitions["CPPLOGGING_MODULE"] = False
self._cmake.definitions["BUILD_TESTING"] = False

Enforce disabling tests

del self.options.fPIC

if self.settings.compiler.get_safe("cppstd"):
build.check_min_cppstd(self, "17")
Copy link
Member

Choose a reason for hiding this comment

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

I just created the PR chronoxor/CppLogging#5 to enforce the standard as requirement.

Comment on lines +76 to +78
files.get(self, **self.conan_data["sources"][self.version])
extracted_dir = glob.glob("CppLogging-*")[0]
os.rename(extracted_dir, self._source_subfolder)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
files.get(self, **self.conan_data["sources"][self.version])
extracted_dir = glob.glob("CppLogging-*")[0]
os.rename(extracted_dir, self._source_subfolder)
files.get(self, **self.conan_data["sources"][self.version], destination=self._source_subfolder, strip_root=True)

Comment on lines +10 to +13
# VirtualBuildEnv and VirtualRunEnv can be avoided if "tools.env.virtualenv:auto_use" is defined
# (it will be defined in Conan 2.0)
generators = "CMakeDeps", "CMakeToolchain", "VirtualBuildEnv", "VirtualRunEnv"
apply_env = False
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# VirtualBuildEnv and VirtualRunEnv can be avoided if "tools.env.virtualenv:auto_use" is defined
# (it will be defined in Conan 2.0)
generators = "CMakeDeps", "CMakeToolchain", "VirtualBuildEnv", "VirtualRunEnv"
apply_env = False
generators = "CMakeDeps", "CMakeToolchain", "VirtualRunEnv"

You are not testing a build tool

cmake_layout(self)

def test(self):
if not cross_building(self):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if not cross_building(self):
if can_run(self):

Comment on lines +2 to +30

from conan import ConanFile
from conan.tools.cmake import CMake, cmake_layout
from conan.tools.build import cross_building


class CpploggingTestConan(ConanFile):
settings = "os", "compiler", "build_type", "arch"
# VirtualBuildEnv and VirtualRunEnv can be avoided if "tools.env.virtualenv:auto_use" is defined
# (it will be defined in Conan 2.0)
generators = "CMakeDeps", "CMakeToolchain", "VirtualBuildEnv", "VirtualRunEnv"
apply_env = False
test_type = "explicit"

def requirements(self):
self.requires(self.tested_reference_str)

def build(self):
cmake = CMake(self)
cmake.configure()
cmake.build()

def layout(self):
cmake_layout(self)

def test(self):
if not cross_building(self):
cmd = os.path.join(self.cpp.build.bindirs[0], "console")
self.run(cmd, env="conanrun")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
from conan import ConanFile
from conan.tools.cmake import CMake, cmake_layout
from conan.tools.build import cross_building
class CpploggingTestConan(ConanFile):
settings = "os", "compiler", "build_type", "arch"
# VirtualBuildEnv and VirtualRunEnv can be avoided if "tools.env.virtualenv:auto_use" is defined
# (it will be defined in Conan 2.0)
generators = "CMakeDeps", "CMakeToolchain", "VirtualBuildEnv", "VirtualRunEnv"
apply_env = False
test_type = "explicit"
def requirements(self):
self.requires(self.tested_reference_str)
def build(self):
cmake = CMake(self)
cmake.configure()
cmake.build()
def layout(self):
cmake_layout(self)
def test(self):
if not cross_building(self):
cmd = os.path.join(self.cpp.build.bindirs[0], "console")
self.run(cmd, env="conanrun")
from conans import ConanFile, CMake
from conan.tools.build import cross_building
class TestPackageV1Conan(ConanFile):
settings = "os", "arch", "compiler", "build_type"
generators = "cmake", "cmake_find_package_multi"
def build(self):
cmake = CMake(self)
cmake.configure()
cmake.build()
def test(self):
if not cross_building(self):
bin_path = os.path.join("bin", "console")
self.run(bin_path, run_environment=True)

Keep test v1 only with legacy tools, so we can prove that the recipe is cross-compatible.

@@ -0,0 +1,42 @@
/*!
Copy link
Member

Choose a reason for hiding this comment

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

Delete this file, you can re-use directly from test_package.

Comment on lines +1 to +9
cmake_minimum_required(VERSION 3.15)
project(PackageTest CXX)

find_package(cpplogging CONFIG REQUIRED)

add_executable(console src/console.cpp)
target_link_libraries(console cpplogging::cpplogging)

target_compile_features(console PRIVATE cxx_std_17)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cmake_minimum_required(VERSION 3.15)
project(PackageTest CXX)
find_package(cpplogging CONFIG REQUIRED)
add_executable(console src/console.cpp)
target_link_libraries(console cpplogging::cpplogging)
target_compile_features(console PRIVATE cxx_std_17)
cmake_minimum_required(VERSION 3.1)
project(test_package)
include(${CMAKE_BINARY_DIR}/conanbuildinfo.cmake)
conan_basic_setup(TARGETS)
add_subdirectory(${CMAKE_CURRENT_SOURCE_DIR}/../test_package/
${CMAKE_CURRENT_BINARY_DIR}/test_package/)

Re-use the implementation from test_package/CMakeLists.txt, but conserve conan_basic_setup() It's deprecated due CMakeDeps, so it's only useful here.

@stale
Copy link

stale bot commented Jan 16, 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 stale bot added the stale label Jan 16, 2023
@EmilienBINET
Copy link
Contributor Author

A little message to say I have not forsaken the subject. I just could not find time to do it.
I hope I will be able to get back into it from the and of the month.

@stale
Copy link

stale bot commented Mar 12, 2023

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

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

Successfully merging this pull request may close these issues.

4 participants