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

[>=2.0] Requires CPAN JSON in practice, even with all of JSON::XS, Cpanel::JSON::XS, JSON::PP installed? #372

Closed
hartwork opened this issue Jan 6, 2025 · 4 comments

Comments

@hartwork
Copy link
Contributor

hartwork commented Jan 6, 2025

Hi!

While playing with Gentoo bug report https://bugs.gentoo.org/922908 I found that with all of JSON::XS, Cpanel::JSON::XS, JSON::PP installed, the tests still fail with…

# make -j5 -j1 check 
Error: Missing Perl module 'JSON' required by /var/tmp/portage/dev-util/lcov-2.0-r2/work/lcov-2.0/tests//../bin/criteria
make[1]: *** [common.mak:69: checkdeps] Error 1

…which is in (some?) contradiction with statement…

lcov/README

Lines 135 to 140 in 3cd2210

- at least one flavor of JSON module.
In order of performance/preference:
- JSON::XS
- Cpanel::JSON::XS
- JSON::PP
- JSON

…but maybe it depends on whether one is reading that as being about (a) runtime or (b) runtime-and-test dependencies.

I guess for Gentoo, the fix is to insist on plain JSON for when tests are enabled with LCOV >=2.0.
With regard to upstream, what do you envision as the best fix or improvement?

Best, Sebastian

@hartwork
Copy link
Contributor Author

hartwork commented Jan 6, 2025

PS: Here's the related fix that I applied to Gentoo packaging now: gentoo/gentoo@be52a5f

@hartwork hartwork changed the title [>=2.0] Requires CPAN JSON in practice, even with JSON::XS, Cpanel::JSON::XS, JSON::PP installed? [>=2.0] Requires CPAN JSON in practice, even with all of JSON::XS, Cpanel::JSON::XS, JSON::PP installed? Jan 6, 2025
@henry2cox
Copy link
Collaborator

My bad :-(
(I never noticed - partly because we have all those modules available.)

I changed the implementation slightly, to follow the doc (and not explicitly load JSON).

@hartwork
Copy link
Contributor Author

hartwork commented Jan 6, 2025

I changed the implementation slightly, to follow the doc (and not explicitly load JSON).

@henry2cox cool!

I checked Git master after pulling…

# git --no-pager grep "use JSON"
scripts/criteria:use JSON;
scripts/criteria.pm:use JSON;
scripts/threshold.pm:use JSON;

…and the list of open pull requests — did you push this anywhere?

henry2cox added a commit that referenced this issue Jan 6, 2025
@hartwork
Copy link
Contributor Author

hartwork commented Jan 6, 2025

@henry2cox closing as "assumed fixed" by pull request #374 — thanks!

@hartwork hartwork closed this as completed Jan 6, 2025
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

No branches or pull requests

2 participants