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

battery-embed: add recipe #22698

Merged
merged 23 commits into from
Aug 21, 2024
Merged

Conversation

toge
Copy link
Contributor

@toge toge commented Feb 7, 2024

Specify library name and version: battery-embed/*

Is cmake module only package acceptable?

Let me first check if it is OK to create a recipe with only a cmake module.

Since ignition-cmake also provides a cmake module, I would like to achieve the same thing with battery-embed.
https://github.com/conan-io/conan-center-index/tree/master/recipes/ignition-cmake

Is this recipe adequate in cci policy?

patch description

battery-embed currently has the following two problems:

  1. example program is always built
  2. compile error with gcc13 due to lack of cstdint

Patch files try to solve these.

reason two versions are provided

1.2.x requires std::jthread and std::stop_token.
Those are not provided on apple-clang (yet).


@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@uilianries
Copy link
Member

compile error with gcc13 due to lack of cstdint

@toge Do you have a log or an issue/PR in the upstream to confirm about this behavior? I don't have GCC13 installed here right now to validate it 😞

example program is always built

It could be suggested to the upstream to use your patch instead, so users are free to decide about it.

@toge
Copy link
Contributor Author

toge commented Feb 9, 2024

@uilianries

It could be suggested to the upstream to use your patch instead, so users are free to decide about it.

The issue about it has been created on upstream.
batterycenter/embed#2

I'm watching it.

Do you have a log or an issue/PR in the upstream to confirm about this behavior? I don't have GCC13 installed here right now to validate it 😞

Off course!
Here are:

======== Exporting recipe to the cache ======== WARN: Name containing special chars is discouraged 'battery-embed' battery-embed/1.2.8: Exporting package recipe: /home/toge/src/conan-center-index/recipes/battery-embed/all/conanfile.py battery-embed/1.2.8: exports: File 'conandata.yml' found. Exporting it... battery-embed/1.2.8: Calling export_sources() battery-embed/1.2.8: Copied 1 '.yml' file: conandata.yml battery-embed/1.2.8: Copied 1 '.py' file: conanfile.py battery-embed/1.2.8: Copied 2 '.patch' files: 0001-disable-examples.patch, 0002-include-cstdint.patch battery-embed/1.2.8: Exported to cache folder: /home/toge/.conan2/p/battebcc91ef1dfa80/e battery-embed/1.2.8: Exported: battery-embed/1.2.8#dc406be40a1fbe9305318a815f8cd372 (2024-02-09 09:48:05 UTC)

======== Input profiles ========
Profile host:
[settings]
arch=x86_64
build_type=Release
compiler=gcc
compiler.cppstd=20
compiler.libcxx=libstdc++11
compiler.version=13
os=Linux

Profile build:
[settings]
arch=x86_64
build_type=Release
compiler=gcc
compiler.cppstd=20
compiler.libcxx=libstdc++11
compiler.version=13
os=Linux

======== Computing dependency graph ========
Graph root
cli
Build requirements
battery-embed/1.2.8#dc406be40a1fbe9305318a815f8cd372 - Cache
cmake/3.27.4#a7e78418b024dccacccc887f049f47ed - Cache
Resolved version ranges
cmake/[>=3.21 <4]: cmake/3.27.4

======== Computing necessary packages ========
battery-embed/1.2.8: Compatible package ID 9a4eb3c8701508aa9458b1a73d0633783ecc2270 equal to the default package ID: Skipping it.
Build requirements
battery-embed/1.2.8#dc406be40a1fbe9305318a815f8cd372:9a4eb3c8701508aa9458b1a73d0633783ecc2270 - Build
cmake/3.27.4#a7e78418b024dccacccc887f049f47ed:63fead0844576fc02943e16909f08fcdddd6f44b#fac7109ddc535a744445b6e1c095129c - Cache

======== Installing packages ========
cmake/3.27.4: Already installed! (1 of 2)
cmake/3.27.4: Appending PATH environment variable: /home/toge/.conan2/p/cmake9d32db761075c/p/bin
battery-embed/1.2.8: Calling source() in /home/toge/.conan2/p/battebcc91ef1dfa80/s

-------- Installing package battery-embed/1.2.8 (2 of 2) --------
battery-embed/1.2.8: Building from source
battery-embed/1.2.8: Package battery-embed/1.2.8:9a4eb3c8701508aa9458b1a73d0633783ecc2270
battery-embed/1.2.8: Copying sources to build folder
battery-embed/1.2.8: Building your package in /home/toge/.conan2/p/b/battefe07bce641e42/b
battery-embed/1.2.8: Generating aggregated env files
battery-embed/1.2.8: Generated aggregated env files: ['conanbuild.sh', 'conanrun.sh']
battery-embed/1.2.8: Calling build()
battery-embed/1.2.8: Apply patch (conan): disable building examples
battery-embed/1.2.8: Apply patch (portability): include cstdint for gcc 13 later
battery-embed/1.2.8: Package '9a4eb3c8701508aa9458b1a73d0633783ecc2270' built
battery-embed/1.2.8: Build folder /home/toge/.conan2/p/b/battefe07bce641e42/b
battery-embed/1.2.8: Generating the package
battery-embed/1.2.8: Packaging in folder /home/toge/.conan2/p/b/battefe07bce641e42/p
battery-embed/1.2.8: Calling package()
battery-embed/1.2.8: package(): Packaged 1 file: LICENSE
battery-embed/1.2.8: package(): Packaged 1 '.txt' file: CMakeLists.txt
battery-embed/1.2.8: Created package revision 383d669dd11332c6b6339d753c0fb9eb
battery-embed/1.2.8: Package '9a4eb3c8701508aa9458b1a73d0633783ecc2270' created
battery-embed/1.2.8: Full package reference: battery-embed/1.2.8#dc406be40a1fbe9305318a815f8cd372:9a4eb3c8701508aa9458b1a73d0633783ecc2270#383d669dd11332c6b6339d753c0fb9eb
battery-embed/1.2.8: Package folder /home/toge/.conan2/p/b/battefe07bce641e42/p
WARN: deprecated: Usage of deprecated Conan 1.X features that will be removed in Conan 2.X:
WARN: deprecated: 'env_info' used in: cmake/3.27.4

======== Launching test_package ========

======== Computing dependency graph ========
Graph root
battery-embed/1.2.8 (test package): /home/toge/src/conan-center-index/recipes/battery-embed/all/test_package/conanfile.py
Requirements
battery-embed/1.2.8#dc406be40a1fbe9305318a815f8cd372 - Cache
Build requirements
cmake/3.27.4#a7e78418b024dccacccc887f049f47ed - Cache

======== Computing necessary packages ========
Requirements
battery-embed/1.2.8#dc406be40a1fbe9305318a815f8cd372:9a4eb3c8701508aa9458b1a73d0633783ecc2270#383d669dd11332c6b6339d753c0fb9eb - Cache
Build requirements
Skipped binaries
cmake/3.27.4

======== Installing packages ========
battery-embed/1.2.8: Already installed! (1 of 1)

======== Testing the package ========
Removing previously existing 'test_package' build folder: /home/toge/src/conan-center-index/recipes/battery-embed/all/test_package/build/gcc-13-x86_64-20-release
battery-embed/1.2.8 (test package): Test package build: build/gcc-13-x86_64-20-release
battery-embed/1.2.8 (test package): Test package build folder: /home/toge/src/conan-center-index/recipes/battery-embed/all/test_package/build/gcc-13-x86_64-20-release
battery-embed/1.2.8 (test package): Writing generators to /home/toge/src/conan-center-index/recipes/battery-embed/all/test_package/build/gcc-13-x86_64-20-release/generators
battery-embed/1.2.8 (test package): Generator 'VirtualRunEnv' calling 'generate()'
battery-embed/1.2.8 (test package): Generator 'CMakeToolchain' calling 'generate()'
battery-embed/1.2.8 (test package): CMakeToolchain generated: conan_toolchain.cmake
battery-embed/1.2.8 (test package): CMakeToolchain generated: CMakePresets.json
battery-embed/1.2.8 (test package): CMakeToolchain generated: ../../../CMakeUserPresets.json
battery-embed/1.2.8 (test package): Generator 'CMakeDeps' calling 'generate()'
battery-embed/1.2.8 (test package): CMakeDeps necessary find_package() and targets for your CMakeLists.txt
find_package(battery-embed)
target_link_libraries(... battery-embed::battery-embed)
battery-embed/1.2.8 (test package): Generating aggregated env files
battery-embed/1.2.8 (test package): Generated aggregated env files: ['conanrun.sh', 'conanbuild.sh']

======== Testing the package: Building ========
battery-embed/1.2.8 (test package): Calling build()
battery-embed/1.2.8 (test package): Running CMake.configure()
battery-embed/1.2.8 (test package): RUN: cmake -G "Unix Makefiles" -DCMAKE_TOOLCHAIN_FILE="/home/toge/src/conan-center-index/recipes/battery-embed/all/test_package/build/gcc-13-x86_64-20-release/generators/conan_toolchain.cmake" -DCMAKE_INSTALL_PREFIX="/home/toge/src/conan-center-index/recipes/battery-embed/all/test_package" -DCMAKE_POLICY_DEFAULT_CMP0091="NEW" -DCMAKE_BUILD_TYPE="Release" "/home/toge/src/conan-center-index/recipes/battery-embed/all/test_package"
-- Using Conan toolchain: /home/toge/src/conan-center-index/recipes/battery-embed/all/test_package/build/gcc-13-x86_64-20-release/generators/conan_toolchain.cmake
-- Conan toolchain: C++ Standard 20 with extensions OFF
-- The CXX compiler identification is GNU 13.2.1
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Conan: Target declared 'battery-embed::battery-embed'
-- Conan: Including build module from '/home/toge/.conan2/p/b/battefe07bce641e42/p/lib/cmake/battery-embed/CMakeLists.txt'
-- Configuring done (0.2s)
-- Generating done (0.0s)
-- Build files have been written to: /home/toge/src/conan-center-index/recipes/battery-embed/all/test_package/build/gcc-13-x86_64-20-release

battery-embed/1.2.8 (test package): Running CMake.build()
battery-embed/1.2.8 (test package): RUN: cmake --build "/home/toge/src/conan-center-index/recipes/battery-embed/all/test_package/build/gcc-13-x86_64-20-release" -- -j12
[ 20%] Generating embed/autogen/test_package_cpp.cpp
[ 40%] Building CXX object CMakeFiles/test_package.dir/test_package.cpp.o
[ 60%] Building CXX object CMakeFiles/test_package.dir/embed/autogen/test_package_cpp.cpp.o
[ 80%] Building CXX object CMakeFiles/test_package.dir/embed/embed_impl.cpp.o
[100%] Linking CXX executable test_package
[100%] Built target test_package

======== Testing the package: Executing test ========
battery-embed/1.2.8 (test package): Running test()
battery-embed/1.2.8 (test package): RUN: ./test_package
#include
#include
#include

#include "battery/embed.hpp"

int main(void) {
std::cout << b::embed<"test_package.cpp">() << std::endl;
return EXIT_SUCCESS;
}

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

Copy link
Contributor

@valgur valgur left a comment

Choose a reason for hiding this comment

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

Looks like a perfectly reasonable solution to me. Thanks!

@HerrNamenlos123
Copy link
Contributor

Hi! I am the author of battery-embed.

Interesting that people tried to get my library into Conan, without me even knowing!
I apprechiate the effort, however. I would have been more than willing to make upstream changes if they are for the greater good.

Are there still any issues remaining? I added <cstdint> upstream, no issue with that. Also, examples are only built if battery-embed is the top level CMake module AND BUILD_EXAMPLES is set (which it is, if it's the top-level module).

Is there still an issue with std::jthread? It's only used for convenience. If necessary, it can easily be changed to a normal std::thread and std::atomic<bool> instead of std::stop_token...

@AbrilRBS
Copy link
Member

AbrilRBS commented Apr 5, 2024

Hi @HerrNamenlos123 I'll get back to you with the status of the PR later today, just wanted to drop by to thank you for offering to help, really appreciated :)

@conan-center-bot

This comment has been minimized.

@HerrNamenlos123
Copy link
Contributor

HerrNamenlos123 commented Apr 5, 2024

@toge I really apprechiate your efforts. Could you explain to me why you want to add 1.2.12 AND 1.1.1? I don't see any reason to use 1.1.1? I see that 1.0.0 gives you C++14, but does 1.1.1 give you any advantage over the latest version? If so, can we re-implement it in the latest version?

Also, I don't quite understand why you need to patch the CMakeLists to disable the examples. Can't you define -DBUILD_EXAMPLES=OFF in the conanfile? Isn't that the whole point?

(In case you didn't know, the option() you patched is just defaulted with PROJECT_IS_TOP_LEVEL, meaning the default is based on how you build it. The point of the option is that you can still define it manually and override the default)

I think there shouldn't be any reason to patch anything and if so, i can fix it upstream to keep everything clean...

EDIT: I meant 1.1.1

@toge
Copy link
Contributor Author

toge commented Apr 6, 2024

@HerrNamenlos123
Thank you for your comment!
I am glad to discuss the way to package your product.

Could you explain to me why you want to add 1.2.12 AND 1.1.1? I don't see any reason to use 1.1.1?

The reason to provide 1.1.1 is supporting apple-clang.
As you know, 1.2.x uses std::jthread.
It seems that std::jthread is not supported by apple-clang, and I get the following error message.

embed/embed_impl.cpp:51:10: error: no type named 'jthread' in namespace 'std'; did you mean 'thread'?
    std::jthread embeddedFileWatcherThread = std::jthread([](std::stop_token stopToken) {
    ~~~~~^~~~~~~
         thread
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.4.sdk/usr/include/c++/v1/__thread/thread.h:164:33: note: 'thread' declared here
class _LIBCPP_EXPORTED_FROM_ABI thread
                                ^
embed/embed_impl.cpp:51:67: error: no type named 'stop_token' in namespace 'std'
    std::jthread embeddedFileWatcherThread = std::jthread([](std::stop_token stopToken) {
                                                             ~~~~~^
embed/embed_impl.cpp:51:51: error: no member named 'jthread' in namespace 'std'
    std::jthread embeddedFileWatcherThread = std::jthread([](std::stop_token stopToken) {
                                             ~~~~~^
embed/embed_impl.cpp:55:40: error: variable '__begin2' cannot be implicitly captured in a lambda with no capture-default specified
            for (auto& [key, filedata] : embeddedFiles) {
                                       ^

Also, I don't quite understand why you need to patch the CMakeLists to disable the examples. Can't you define -DBUILD_EXAMPLES=OFF in the conanfile? Isn't that the whole point?

The reason for patching instead of defining BUILD_EXAMPLES is portability.
I think BUILD_EXAMPLES is a popular name and I am afraid of conflicts.

I think there shouldn't be any reason to patch anything and if so, i can fix it upstream to keep everything clean...

I agree with you.
To solve these problems, I am planning to split CMakeLists.txt into two files.

  1. CMakeLists.txt : project, BUILD_EXAMPLES, add_subdirectory.
  2. battery_embed.cmake : functions and dependent variables.

Does this fit your policy?

toge and others added 3 commits August 6, 2024 19:20
Co-authored-by: Martin Valgur <[email protected]>
Co-authored-by: Martin Valgur <[email protected]>
@toge
Copy link
Contributor Author

toge commented Aug 6, 2024

@valgur
Thanks!

@conan-center-bot

This comment has been minimized.

@HerrNamenlos123
Copy link
Contributor

@toge I took a look at the current package and I propose 2 more changes:

  1. Update to the latest Version of 1.2.19, as some important bug fixes were made.

  2. In the Test package, do not include cstdint and cstdlib, because both should be included in embed.hpp itself. Their absence is a bug and the test package should point out the bug and not hide it.

If any other things pop up in the latest version, feel free to immediately create an issue or a PR.

@toge
Copy link
Contributor Author

toge commented Aug 12, 2024

@HerrNamenlos123
Thank you for your comment!
I fixed.

@conan-center-bot

This comment has been minimized.

danimtb
danimtb previously approved these changes Aug 20, 2024
@danimtb danimtb self-assigned this Aug 20, 2024
Copy link
Member

@AbrilRBS AbrilRBS left a comment

Choose a reason for hiding this comment

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

Thanks! I have some questions that are confusiong me a bit, otherwise looks good

recipes/battery-embed/all/conanfile.py Show resolved Hide resolved
recipes/battery-embed/all/conanfile.py Show resolved Hide resolved
Copy link
Member

@AbrilRBS AbrilRBS left a comment

Choose a reason for hiding this comment

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

Thanks! I have some questions that are confusiong me a bit, otherwise looks good

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

All green in build 17 (fe43ce39c386c9165ad9de09d62a9193f97b2dd5):

  • battery-embed/1.2.19:
    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 17 (fe43ce39c386c9165ad9de09d62a9193f97b2dd5):

  • battery-embed/1.2.19:
    All packages built successfully! (All logs)

@danimtb danimtb requested a review from AbrilRBS August 21, 2024 10:46
Copy link
Member

@AbrilRBS AbrilRBS left a comment

Choose a reason for hiding this comment

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

Thanks!

@conan-center-bot conan-center-bot merged commit f4ac88d into conan-io:master Aug 21, 2024
12 checks passed
@HerrNamenlos123
Copy link
Contributor

Thank you everyone for the effort, was not expecting my library to end up in Conan Center xD

@HerrNamenlos123
Copy link
Contributor

I am terribly sorry for only noticing now, but unfortunately I found another mistake. The Test package links to battery::embed, but this is not supposed to happen. Like with the include paths, this is meant to happen automatically via b_embed().

The package itself works fine, so it's absolutely not critical, but if we want to be pedantic and ensure correct testing, it should be fixed at some point. Sorry for not noticing earlier, I must have skipped over it.

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.

7 participants