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

Improve zug #2

Merged
merged 2 commits into from
Apr 25, 2022
Merged

Improve zug #2

merged 2 commits into from
Apr 25, 2022

Conversation

uilianries
Copy link

@uilianries uilianries commented Apr 22, 2022

Some improvements to accept your recipe in Conan Center Index

Signed-off-by: Uilian Ries <[email protected]>
@BlueSolei
Copy link
Owner

BlueSolei commented Apr 22, 2022

Thanks a lot for all the great improvements :-)
Two things I see:

  1. by changing the setting to settings = ("compiler", "arch", "os", "build_type"), you made the recipe depend on arch, os and build_type, which this library doesn't depend on them at all (you didn't use these flags in the recipe).
    I see you use:
    def package_id(self):
        self.info.header_only()

but maybe it's best to be explicit in that this package only depends on the compiler and std?
which brings me to the next issue,
2. the only dependency is in the compiler, its version, and the c++ std.
the code you added is great, but why did you remove the dependency over Zc:externConstexpr when using VS?
This flag is needed when we use VS in C++14.
So I can't just use the self.info.header_only() trick as I need different version for VS compiler (with the flag)

Signed-off-by: Uilian Ries <[email protected]>
@uilianries
Copy link
Author

  1. Use FULL settings ever. It will be the only way for Conan V2 which is closer to be released. You should manipulate the package ID on package_id method, so does not matter if you are using full settings.

  2. Usually we take compiler flags as patch, or even from upstream. I see this is not the case, Zug has no more updates and that flag is too small for a patch. I just reverted to the previous cxxflags.

@BlueSolei
Copy link
Owner

  1. cool
  2. how exactly do you take the compiler flags as a patch? from where?
    Usually, they are part of the build system scripts (here CMake target) but how do you extract them from there? and to where?
    I fill I miss a big part of Conan here :-| do you have an example? a link to docs?

@BlueSolei BlueSolei merged commit 57ef619 into BlueSolei:zug Apr 25, 2022
@uilianries
Copy link
Author

  1. Create a patch to change directly the upstream's cmake. For this specific case, externConstexpr is known: Adding a "zug_" prefix to target names and some other enhancements arximboldi/zug#26 (comment)

About patch example, there are tons in Conan Center Index, just search for "patches". Anyway, it's documented here and here

@BlueSolei
Copy link
Owner

I understand patching the library code if we can't change it, but AFAIK this is not the issue here.
The issue here is with compiler flags when you use the library.
I need a way to get these flags from the library to the project which uses the Zug's Conan package.
How do I do that?
My understanding is that I need to put these flags in the recipe, as we did here:

    def package_info(self):
        if is_msvc(self):
            self.cpp_info.cxxflags = ["/Zc:externConstexpr"]

What other way does the library have which can be taken by the recipe and put in the package so a client can import the package?
What am I missing?

The link you put, tells the programmer to add this flag manually to his build script which uses Zug.
Or, the flag can be put in the CMakeLists.txt file, as a compiler option to the Zug target, but that means I will need to package that file too and add it as a CMake module, but that will only work for someone who imports the package through CMake.
Again, I fill I miss a fundamental thing here.

@uilianries
Copy link
Author

btw, about settings: https://github.com/conan-io/conan-center-index/blob/154c9edca773d52a790d917bf2c0b95796a7ce7b/docs/packaging_policy.md#settings

What other way does the library have which can be taken by the recipe and put in the package so a client can import the package?

https://docs.conan.io/en/latest/reference/conanfile/methods.html#cpp-info

You already did. When passing cxxflags, any Conan generator will inject those flags in the generated file.

BlueSolei pushed a commit that referenced this pull request Oct 25, 2022
* [Catch2] adapt recipe for Conan v2

* fixes

* fixes

* fixes

* fixes

* fixes 4

* fixes 5

* fixes 6

* Update recipes/catch2/3.x.x/conanfile.py

Co-authored-by: Uilian Ries <[email protected]>

* Update recipes/catch2/3.x.x/conanfile.py

Co-authored-by: Uilian Ries <[email protected]>

* Update recipes/catch2/3.x.x/conanfile.py

Co-authored-by: Uilian Ries <[email protected]>

* Update recipes/catch2/3.x.x/conanfile.py

* fix patches

* fix

* fix

* remove comments

* Update recipes/catch2/3.x.x/conanfile.py

Co-authored-by: Chris Mc <[email protected]>

* Update recipes/catch2/3.x.x/conanfile.py

Co-authored-by: Chris Mc <[email protected]>

* test package changes

* Move test package files (#2)

* move gtest

Signed-off-by: Uilian Ries <[email protected]>

* Delete test_all.sh

* Use cmake targets

* Do not use settings

Signed-off-by: Uilian Ries <[email protected]>

Signed-off-by: Uilian Ries <[email protected]>
Co-authored-by: Uilian Ries <[email protected]>
Co-authored-by: Chris Mc <[email protected]>
BlueSolei pushed a commit that referenced this pull request Mar 14, 2024
* sqlcipher: Add 4.5.6

* sqlcipher: Add 4.5.6

* sqlcipher: Fix topic case and add missing endline

* sqlcipher: Fix license for version >4.5.6

* do not enforce cmake in test package

Signed-off-by: Uilian Ries <[email protected]>

* sqlcipher: relax license filename to copy

Co-authored-by: Uilian Ries <[email protected]>

* sqlcipher: Add option to enable column metadata

* sqlcipher: do not enforce cmake in test package #2

---------

Signed-off-by: Uilian Ries <[email protected]>
Co-authored-by: Uilian Ries <[email protected]>
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.

2 participants