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

diligent-core: migrate to Conan v2 #23347

Merged
merged 9 commits into from
Jun 15, 2024

Conversation

MikeLankamp
Copy link
Contributor

@MikeLankamp MikeLankamp commented Apr 2, 2024

Specify library name and version: diligent-core/*

Upgrading diligent-core to Conan 2.

The main problems in using diligent-core with Conan 2 as-is are:

  • a conflicting requirement of vulkan-headers (1.3.224.0 from vulkan-validationlayers/1.3.224.1, but 1.3.224.1 from diligent-core itself and volk/1.3.224.1). This has been fixed by setting the conflicting dependencies to 1.3.224.0.
  • The cmake/3.24.2 tool requirement is not Conan2-enabled. The dependency has been generalized to [>=3.24 <4], which allows Conan to pick a Conan2-enabled version.

This PR is similar to PR #21522, but doesn't attempt to bump dependencies, which comes with its own set of problems apparently. This PR just tries to get this recipe Conan2-compatible.

Some additional fixes in the conanfile are required to support Conan2. The largest of which is the change that the source is no longer downloaded into "source_subfolder". This was originally to provide a custom top-level CMakeLists.txt which defines library aliases for conan dependency. Without these, Diligent's own CMake files attempts to build empty folders in "ThirdParty". This was changed into modifying the original top-level CMakeLists.txt to include the file with target aliases instead (similar to PR #21522).


@CLAassistant
Copy link

CLAassistant commented Apr 2, 2024

CLA assistant check
All committers have signed the CLA.

@conan-center-bot

This comment has been minimized.

@conan-center-bot conan-center-bot removed the Version conflict There is a version conflict when solving the dependencies graph label Apr 2, 2024
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

Copy link
Contributor

github-actions bot commented Apr 2, 2024

Hooks produced the following warnings for commit ab5b43d
diligent-core/api.252005@#c5e43fba2d251db74b30e90dc23e7c4d
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\GraphicsEngineD3D11_64r.dll' links to system library 'd3d11' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\GraphicsEngineD3D11_64d.dll' links to system library 'd3d11' but it is not in cpp_info.system_libs.
diligent-core/api.252004@#25e5d7ab6414e84ba1bd52e898cd2da9
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\GraphicsEngineD3D11_64d.dll' links to system library 'd3d11' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\GraphicsEngineD3D11_64r.dll' links to system library 'd3d11' but it is not in cpp_info.system_libs.
diligent-core/api.250014@#f688142527956ffcb3b8d5d0853ccc5f
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\GraphicsEngineD3D11_64d.dll' links to system library 'd3d11' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\GraphicsEngineD3D11_64r.dll' links to system library 'd3d11' but it is not in cpp_info.system_libs.
diligent-core/2.5.2@#882ed6a310496183bac52ea5f407fe8f
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\GraphicsEngineD3D11_64d.dll' links to system library 'd3d11' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\GraphicsEngineD3D11_64r.dll' links to system library 'd3d11' but it is not in cpp_info.system_libs.
diligent-core/api.252003@#f1becd85a6d342f6ff4c627148a6c626
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\GraphicsEngineD3D11_64r.dll' links to system library 'd3d11' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\GraphicsEngineD3D11_64d.dll' links to system library 'd3d11' but it is not in cpp_info.system_libs.
diligent-core/2.5.1@#7c9c2ca2e0b5f2d0827210ee1269ac6b
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\GraphicsEngineD3D11_64r.dll' links to system library 'd3d11' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\GraphicsEngineD3D11_64d.dll' links to system library 'd3d11' but it is not in cpp_info.system_libs.
diligent-core/api.252009@#88cf924aeaa270e277383d0e88661513
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\GraphicsEngineD3D11_64d.dll' links to system library 'd3d11' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\GraphicsEngineD3D11_64r.dll' links to system library 'd3d11' but it is not in cpp_info.system_libs.

@AbrilRBS AbrilRBS self-assigned this Apr 3, 2024
@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.

@MikeLankamp
Copy link
Contributor Author

Hi @valgur, how could this PR get the required approvals? @RubenRBS, can you provide a required review?

@jwillikers
Copy link
Contributor

@MikeLankamp Could you review MikeLankamp#1? That change will use the libglvnd on Linux and FreeBSD. I figured it would be best to get that in your branch.

@MikeLankamp
Copy link
Contributor Author

MikeLankamp commented Apr 10, 2024

I figured it would be best to get that in your branch.

@jwillikers, may I ask why? Your change seems unrelated with my Conan 2.0 support and aims to get libglvnd used. Adding it to my branch would only pollute my PR to master with unrelated stuff (potentially dragging out the review).

Additionally, I'm not knowledgable enough about libglvnd and the Linux/FreeBSD ecosystem to judge whether its inclusion here makes sense, so it seems better for you to create a separate PR to master once my PR is in.

@AbrilRBS
Copy link
Member

Hi! We have moved the discussion on libglvnd to #23649, so no need to apply those changes for now

As for the review of the PR itself, this is something I plan on doing next monday, sorry for the delay! From what I've seen in a quick glance, it looks mostly good to go! :)

@MikeLankamp
Copy link
Contributor Author

As for the review of the PR itself, this is something I plan on doing next monday

I suppose that didn't happen, but I hope you'll find time next week :)

@MikeLankamp
Copy link
Contributor Author

Hi, @danimtb, @RubenRBS, I don't want to sound pushy, but this PR has been awaiting a review for almost two months now. I'd like to remind you that without this change, this package is unusable with Conan 2.

I hope you can find the time soon to have a look at this change.

@danimtb
Copy link
Member

danimtb commented Jun 13, 2024

Hi @MikeLankamp,

I'm sorry for not getting back to you sooner. It is somehow hard to keep pace with the reviews. I have noticed you added files for test_v1_package, which are no longer needed. I don't see anything else to prevent this PR from getting merged. But let's wait for @RubenRBS review.

Thanks a lot for your patience 🙏

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

All green in build 9 (8754d20ae3caa107370e187bc31db626db98add0):

  • diligent-core/api.252009:
    All packages built successfully! (All logs)

  • diligent-core/api.252004:
    All packages built successfully! (All logs)

  • diligent-core/api.250014:
    All packages built successfully! (All logs)

  • diligent-core/api.252005:
    All packages built successfully! (All logs)

  • diligent-core/api.252003:
    All packages built successfully! (All logs)

  • diligent-core/2.5.2:
    All packages built successfully! (All logs)

  • diligent-core/2.5.1:
    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 9 (8754d20ae3caa107370e187bc31db626db98add0):

  • diligent-core/api.252005:
    All packages built successfully! (All logs)

  • diligent-core/api.252004:
    All packages built successfully! (All logs)

  • diligent-core/api.252009:
    All packages built successfully! (All logs)

  • diligent-core/api.252003:
    All packages built successfully! (All logs)

  • diligent-core/2.5.1:
    All packages built successfully! (All logs)

  • diligent-core/2.5.2:
    All packages built successfully! (All logs)

  • diligent-core/api.250014:
    All packages built successfully! (All logs)

@conan-center-bot conan-center-bot merged commit ad38e25 into conan-io:master Jun 15, 2024
43 checks passed
@MikeLankamp
Copy link
Contributor Author

MikeLankamp commented Jun 15, 2024

Hi @valgur, @danimtb, @RubenRBS, sorry to bother you after this is merged (thanks for approving this PR!), but when I try to use this package, Conan 2 complains as follows:

diligent-core/2.5.2: Compatible package ID 0b6ce91bc61db9be4c90e8138b36d3ae166dc8be equal to the default package ID: Skipping it.
diligent-core/2.5.2: Checking 3 compatible configurations
diligent-core/2.5.2: Compatible configurations not found in cache, checking servers
diligent-core/2.5.2: 'f9de1b468d6419cb25174c1f3ce2c2b9866a247f': compiler.cppstd=14
diligent-core/2.5.2: Main binary package '0b6ce91bc61db9be4c90e8138b36d3ae166dc8be' missing. Using compatible package 'f9de1b468d6419cb25174c1f3ce2c2b9866a247f': compiler.cppstd=14
ERROR: Invalid setting 'MD/MDd' is not a valid 'settings.compiler.runtime' value.
Possible values are ['static', 'dynamic']
Read "http://docs.conan.io/2/knowledge/faq.html#error-invalid-setting"

I'm using MSVC and my local profile has compiler.cppstd = 17, so it can't find an exact prebuilt match. It uses cppstd_compat.py to find the prebuilt cppstd=14 package, but in processing that one, it seems to re-validate the settings from the package_id. And "MD/MDd" is not valid.

Is this is a bug in Conan, or a bug in this package?

On the one hand, documentation seems to indicate you can put any string in self.info as it's just for package_id creation, so in that case, why does Conan re-validate it? On the other hand, documentation also says this is practice discouraged.

That said, I also don't quite get why the runtime property is set to both release/debug. I suppose this reuses the same prebuilt package for both configurations, but I don't know why. It's been this way since this recipe got added in #7804. @AndreyMlashkin, do you perhaps remember why?

Update: fixed in conan-io/conan#16575

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