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

xkbcommon: Use PkgConfigDeps build context for wayland #13612

Merged
merged 5 commits into from
Oct 21, 2022

Conversation

jwillikers
Copy link
Contributor

Specify library name and version: xkbcommon/*

Now that PkgConfigDeps can generate pkg-config files for the build context as of Conan 1.52.0, the xkbcommon recipe should make use of it for the wayland and wayland-protocols dependencies which are used as tool_requires.


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

@ghost
Copy link

ghost commented Oct 19, 2022

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

@ghost ghost mentioned this pull request Oct 19, 2022
4 tasks
@conan-center-bot

This comment has been minimized.

if self._has_build_profile:
replace_in_file(self, meson_build_file,
"wayland_scanner_dep = dependency('wayland-scanner', required: false, native: true)",
"wayland_scanner_dep = dependency('wayland-scanner_BUILD', required: false, native: true)")
Copy link
Contributor

Choose a reason for hiding this comment

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

if you dont ass the context suffix could this be removed?

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'll get an error if I don't set the build_context_suffix for the wayland package because this library requires the wayland-client library from the wayland package as a regular requires while simultaneously requiring the wayland-scanner tool from the wayland package as a tool_requires.

If we were able to place the build-context pkg-config files in a separate directory from the others, this would avoid naming conflicts and the necessary patching to find the tool_requires dependencies. Meson would be able to handle this gracefully just by setting the standard project option build.pkg_config_path to a value including the directory for the build-context pkg-config files.

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've gone ahead and re-worked this PR to use a separate directory for the pkg-config files in the build context. This doesn't require using replace_in_file when a build profile is present.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sometimes I ask questions for myself to learn. Don't feel obligated to make changes the community (aka you) know more than me and that's a good thing.

Reading and looking qt the code I feel like it's a limitation on the client, like there's nothing in the layout for separation... seems like a useful feature

Maybe we can open an issue again the main repo and see what the team thinks?

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 think that'd be a good idea, even if it's only useful for Meson. Your question hit on something that'd been bothering me, too. I'm glad you brought it up. It really seems more maintainable if we don't have to patch the build system in order to use the pkg-config files in the build context. I think the changes here will be a great demonstration of the desired functionality. I'll take care of opening the issue tomorrow unless you get to it first.

Copy link
Contributor

Choose a reason for hiding this comment

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

I created conan-io/conan#12342 but feel free to correct me if I misunderstood or left something important out :)

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! I just realized my workaround doesn't actually work... I'd have to not only generate pkg-config files for the wayland dependency, but also for everything listed as a requires for the wayland::wayland-scanner component. Without the pkg-config files available for the dependencies, pkg-config reports that the dependency was not found.

@jwillikers
Copy link
Contributor Author

@ericLemanissier I've cherry-picked your commit in #13604 now that your changes to the X11 packages have been merged.

@conan-center-bot

This comment has been minimized.

@conan-center-bot
Copy link
Collaborator

All green in build 4 (05bab6bebd90212f425c0653376a1069b737cf8b):

  • xkbcommon/1.4.1@:
    All packages built successfully! (All logs)

  • xkbcommon/1.3.1@:
    All packages built successfully! (All logs)

  • xkbcommon/1.4.0@:
    All packages built successfully! (All logs)

  • xkbcommon/1.3.0@:
    All packages built successfully! (All logs)

  • xkbcommon/1.0.1@:
    All packages built successfully! (All logs)

  • xkbcommon/1.2.1@:
    All packages built successfully! (All logs)

  • xkbcommon/1.0.3@:
    All packages built successfully! (All logs)

  • xkbcommon/1.1.0@:
    All packages built successfully! (All logs)

  • xkbcommon/0.10.0@:
    All packages built successfully! (All logs)

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.

5 participants