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

added recipe for kwidgetsaddons #3289

Closed

Conversation

boussaffawalid
Copy link
Contributor

@boussaffawalid boussaffawalid commented Oct 21, 2020

Specify library name and version: lib/1.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
Copy link
Collaborator

Failure in build 1 (bdf396e32bff671ce96c732200ed0dd626165e66):

  • Error processing recipe (ref 'kwidgetsaddons/5.75.0'): Linux x86_64, Release, gcc 4.9, libstdc++ . Options: kwidgetsaddons:shared-False
    You are depending on 'qt/5.15.1' but it is not in the repository

Comment on lines +30 to +32
del self.options.fPIC

def build_requirements(self):
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
del self.options.fPIC
def build_requirements(self):
del self.options.fPIC
def configure(self):
if self.options.shared:
del self.options.fPIC
def build_requirements(self):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

according to https://docs.conan.io/en/latest/mastering/conditional.html#constrain-settings-and-options del self.options.fPIC should be done in the config_options method

Copy link
Contributor

@madebr madebr Oct 22, 2020

Choose a reason for hiding this comment

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

Yes indeed, but there are 2 conditions on which fPIC is a don't care:

  • on Windows: fPIC is not needed
  • on Linux, when building a shared library. All code should be built as PIC, so shared=True and fPIC=False is an invalid configuration. Therefore, we remove fPIC when shared=True. Because the user-supplied shared value is only visible in configure, this check should be done in configure.

self._cmake.definitions['CONAN_KDE_INSTALL_DATADIR_KF5'] = os.path.join(self.package_folder, "res")
self._cmake.definitions['CMAKE_INSTALL_LOCALEDIR'] = os.path.join(self.package_folder, "res", "locale")
# a hack to avoid installing CMake find modules andconfig files (KB-H016)
self._cmake.definitions['KDE_INSTALL_CMAKEPACKAGEDIR'] = os.path.join(self.build_folder, "dummy")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you check the name of the generated cmake files + targets and recreate them in package_info?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried this:

        self.cpp_info.name = "KF5"
        self.cpp_info.components["WidgetsAddons"].libs = ["KF5WidgetsAddons"]
        self.cpp_info.components["WidgetsAddons"].includedirs = ["include/KF5", "include/KF5/KWidgetsAddons"]
        self.cpp_info.components["WidgetsAddons"].names["cmake_find_package"] = "WidgetsAddons"
        self.cpp_info.components["WidgetsAddons"].names["cmake_find_package_multi"] = "WidgetsAddons"

Which can be linked in cmake using

find_package(KF5 5.75.0 REQUIRED COMPONENTS WidgetsAddons)
target_link_libraries(example KF5::kwidgetsaddons)

However this break conan TARGETS mode as I cannot do this anymore

target_link_libraries(example CONAN_PKG::kwidgetsaddons)

because the name is now KF5 instead of kwidgetsaddons.

Not sure how to fix this. Ultimately we should have

  • CONAN_PKG::kwidgetsaddons in conan target mode
  • KF5::kwidgetsaddons in cmake find_package mode

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it this what you want?

self.cpp_info.names["cmake_find_package"] = "KF5"
self.cpp_info.names["cmake_find_package_multi"] = "KF5"
self.cpp_info.components["libkwidgetsaddons"].libs = ["KF5WidgetsAddons"]
self.cpp_info.components["libkwidgetsaddons"].includedirs = ["include/KF5", "include/KF5/KWidgetsAddons"]
self.cpp_info.components["libkwidgetsaddons"].names["cmake_find_package"] = "WidgetsAddons"
self.cpp_info.components["libkwidgetsaddons"].names["cmake_find_package_multi"] = "WidgetsAddons"
self.cpp_info.components["libkwidgetsaddons"].requirse = ["qt5::qt5"]  # FIXME: should probably only require "qt5::widgets"
self.cpp_info.components["_lib2"].names["cmake_find_package"] = "kwidgetsaddons"
self.cpp_info.components["_lib2"].names["cmake_find_package_multi"] = "kwidgetsaddons"

This enables:

find_package(KF5 REQUIRED COMPONENTS WidgetsAddons)
target_link_libraries(example KF5::kwidgetsaddons) 
# target_link_libraries(example KF5::WidgetsAddons) is also possible, but that is a limitation of the imperfect conan modelling of the components.

# This still works
target_link_libraries(example CONAN_PKG::kwidgetsaddons)

self._cmake.definitions['CONAN_KDE_INSTALL_DATADIR_KF5'] = os.path.join(self.package_folder, "res")
self._cmake.definitions['CMAKE_INSTALL_LOCALEDIR'] = os.path.join(self.package_folder, "res", "locale")
# a hack to avoid installing CMake find modules andconfig files (KB-H016)
self._cmake.definitions['KDE_INSTALL_CMAKEPACKAGEDIR'] = os.path.join(self.build_folder, "dummy")
Copy link
Contributor

@madebr madebr Oct 22, 2020

Choose a reason for hiding this comment

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

I have another question about the KF5 cmake module.
When doing find_package(KF5 REQUIRED COMPONENTS WidgetsAddons),
is the name KF5 owned by kwidgetsaddons, or is this something shared with other kde packages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is my concern as well, KF5 is the namespace for all kde packages.
In the cmake version one should be able todo

find_package(KF5 REQUIRED COMPONENTS WidgetsAddons karchive)

or

find_package(KF5 REQUIRED COMPONENTS karchive)
find_package(KF5 REQUIRED COMPONENTS WidgetsAddons)

So I'm afraid that the approach you mentioned above wont work. I didn't try it thought.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this deserves an issue on https://github.com/conan-io/conan

Copy link
Contributor

Choose a reason for hiding this comment

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

@SpaceIm @uilianries @danimtb @jgsogo

Have you encountered something like this before?
Is conan able to handle this?

Copy link
Contributor

@SpaceIm SpaceIm Oct 27, 2020

Choose a reason for hiding this comment

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

I guess the only way to handle this case is to add a master recipe requiring kde recipes. This master recipe would be the "owner" of KF5 module/config file.
Otherwise, a PR could allow to conan client to merge multiple module/config file with the same name when generators are called.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be a regular package requiring other packages... the cmake_find_package generator will take care of transitively include all the requirements. Not sure what you mean.

Copy link
Contributor

Choose a reason for hiding this comment

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

If what you're proposing is to let a master recipe selectively add requirements using options, that is imho unmaintainable.
For adding a new KDE recipe, you should then make changes to 2 recipes: the recipe itself + the master recipe.
That's way too complicated imho.

Copy link
Contributor

Choose a reason for hiding this comment

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

If qt and boost were modularized, it would require the same feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with @madebr having a master recipe just so that conan will be able to generate cmake_find_package does not sounds very convenient.
Also, how would the master recipe handle multiple libraries? Someone may require only kwidgetsaddons, someone else may require kwidgetsaddons and kpropert
If the master recipe will depends on both libraries, this will force users to depend on both even if only one is needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can think about a Conan feature that allows to gather different requirements into the same file generated by cmake_find_package (or any other generator) if the file happens to use the same. It would require some Conan magic to compose this file... does it works always or it is somehow declared in the consumer recipe? can it work for transitive requirements? As it depends on generator filenames/names then it cannot be resolved in the graph,... I see some questions here that don't have an obvious answer, but maybe we can open an issue in the Conan repo and move this feature-request there.


On the other hand, a master/parent recipe is something that can be added right now. I'm not saying it is the best approach, just one alternative. That recipe would need options to define the requirements to use:

class KF5Recipe(ConanFile):
   options = {
      'kwidgetsaddons': [True, False],
      'kpropert': [True, False],
   }

   def requirements(self):
      if self.options.kwidgetsaddons:
         self.requires('kwidgetsaddons/{}'.format(self.version))
      if self.options.kpropert:
         self.requires('kpropert/{}'.format(self.version))

   def package_info(self):
      if self.options.kwidgetsaddons:
         self.cpp_info.components['kwidgetsaddons'].libs = self.deps_cpp_info['kwidgetsaddons'].libs
         self.cpp_info.components['kwidgetsaddons'].includes = self.deps_cpp_info['kwidgetsaddons'].includes
         ...
      if self.options.kpropert:
         self.cpp_info.components['kpropert']...

And use it from consumers:

class Consumer(ConanFile):
   requires = "kf5/version"
   default_options = {'kf5:kwidgetsaddons': True, 'kf5:kpropert': False}

Only required components will be created and only required libraries will be retrieved and available in the kf5:: namespace.

@stale
Copy link

stale bot commented Dec 1, 2020

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 Dec 1, 2020
@stale
Copy link

stale bot commented Dec 31, 2020

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants