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

Adds find_package for fmtlib, range-v3, nlohmann_json #15878

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

clonker
Copy link
Member

@clonker clonker commented Feb 19, 2025

Adds find_package for fmtlib, range-v3, nlohmann_json if IGNORE_VENDORED_DEPENDENCIES was set to ON.

Fixes #15872

@aarlt
Copy link
Member

aarlt commented Feb 19, 2025

hmm.. I just saw this - the question is basically how the stuff is used.. the use-case that I had in mind when I was doing this, was to just being able to easily embed solidity into another project - where the "root" cmake file will find needed packages.. I'm not sure right now why I exactly not just added the find_packages there - but maybe we should at least have a mechanism that will not call find the needed packages automatically - so that the "root" cmake file is still under control

@aarlt
Copy link
Member

aarlt commented Feb 19, 2025

However, I don't see the reason right now why I didn't just call find_package there. I think it should be totally fine, even without that special flag. I thought there was a good reason not to call it there, but apparently I don't remember that right now.

@aarlt
Copy link
Member

aarlt commented Feb 19, 2025

Ok, I just remembered the issue, why I decided not to use find_package in this case. The issue was that we had internally for jsoncpp a different target name as normally defined by the official one (see aarlt/cmake_vcpkg_solidity@2679ec0#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20aR16) so I thought it would be the best, if a "root" cmake file is dealing with the aliases. However, because we don't use that anymore we can just do it as you implemented it.

@aarlt
Copy link
Member

aarlt commented Feb 19, 2025

The only thing that could make some sense for some users is to allow them to ignore specific versions of the libraries. E.g. by default we use exactly what you have defined, but if e.g. something like IGNORE_SPECIFIC_DEPENDENCY_VERSIONS was set, it will just use any version.

@Spixmaster
Copy link

Indeed, having specific versions will break packages like https://aur.archlinux.org/packages/solidity.

@clonker
Copy link
Member Author

clonker commented Feb 20, 2025

Indeed, having specific versions will break packages like https://aur.archlinux.org/packages/solidity.

For this particular package I don't see how this change would lead to breakage. It has IGNORE_VENDORED_DEPENDENCIES set to OFF, therefore the three dependencies are pulled in as submodules. But I agree, the version should probably only be restricted to a major version - if at all.

@Spixmaster
Copy link

Spixmaster commented Feb 20, 2025

The reason that -D IGNORE_VENDORED_DEPENDENCIES=OFF is set is due to this bug #15872. Otherwise, the option would be set to ON.

I am the package maintainer.

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.

-D IGNORE_VENDORED_DEPENDENCIES=ON: "fmt::fmt-header-only" not found
3 participants