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

Find cjson on Fedora, where cmake config is missing #2289

Merged
merged 3 commits into from May 11, 2024

Conversation

pemensik
Copy link
Contributor

Current fedora packages do not ship cmake config on cjson-devel package in any release. That should get fixed, but allow locating of cjson via pkg-config in that case.

@pemensik
Copy link
Contributor Author

A drawback of this change is it prefers pkg-config information over cJSON cmake module, even when it is present. I am not sure how that could be avoided and FindcJSON would use it only when normal config is missing.

@jackeri
Copy link
Member

jackeri commented Mar 12, 2023

Wonder if this custom findcjson actually breaks the build on some distros (arch for instance). It would be nice to get some feedback from users of other distros on this. Also, just static linking the cjson from the bundled libs is not such a big deal as it is just a single file…

If BUNDLED_LIBS is disabled and cjson library does not ship its cmake
config for some reason, attempt to find it the other way. That is the
case on current Fedora cjson package.

Provide CJSON_LIBRARY in addition to CJSON_LIBRARIES variable.
Use just CJSON_LIBRARIES similar to other modules.
Use config by default even when FindcJSON module is preferred. Try
CONFIG only and if that works, not not attempt other methods. Should
behave exactly the same way as if FindcJSON module were not present.
@pemensik
Copy link
Contributor Author

Number of files does not really matter. Fedora guidelines are very clear about this and cjson has its own package. I do not see a good reason for bundling it. But I have found a way to have it maximally compatible and prefer CONFIG mode, if it works. It can still use pkg-config or plain library search

@runlevel5
Copy link
Contributor

@pemensik is this only impacting F38?

@pemensik
Copy link
Contributor Author

It affects Fedora 38 and older. cjson-devel on Fedora 39 already contains cmake module, starting with version 1.7.15.

It can of course affect also other distributions, but Debian had it present also on previous oldstable. I have made no extensive search across distributions how they are built. If they are built with cmake, cmake module is made. Depends what package maintainer has chosen.

@pemensik
Copy link
Contributor Author

@jackeri It is not because the size of additional code. But using static library requires all dependent packages rebuilt if serious bug or even security vulnerability were found. With shared library just the library is rebuilt and unless it modifies ABI, that's it. I think that is primary reason why Fedora requires all libraries in shared form if possible.

@pemensik
Copy link
Contributor Author

Of course EPEL builds are affected as well. Not sure if anyone is compiling etlegacy on RHEL or their clones.

Copy link

sonarcloud bot commented Feb 13, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
48 Security Hotspots
3.1% Duplication on New Code (required ≤ 3%)
E Reliability Rating on New Code (required ≥ A)
D Security Rating on New Code (required ≥ A)

See analysis details on SonarCloud

idea Catch issues before they fail your Quality Gate with our IDE extension SonarLint SonarLint

@Aciz
Copy link
Member

Aciz commented May 11, 2024

Wonder if this custom findcjson actually breaks the build on some distros (arch for instance). It would be nice to get some feedback from users of other distros on this. Also, just static linking the cjson from the bundled libs is not such a big deal as it is just a single file…

Not sure what the status of this is and if this is still being considered or what, but FWIW it doesn't seem to break anything on Arch.

@Aranud Aranud merged commit 6ec07db into etlegacy:master May 11, 2024
1 check failed
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.

None yet

5 participants