Fix various Perl runtime warnings #1082
Conversation
empty string if the parameter is not included in the request
- this has generated warnings since Perl 5.20
related to the illegally named function "shouldn't"
|
What kind of warnings would the outdated modules cause? My log files usually are pretty clean (macOS, Raspbian, pCP). |
|
Can't really remember what the old ones were. Can probably trigger them by undoing the patch I have in my package but the last one states:
|
|
I seem to be unable to reproduce what was there before. May have been a glitch specific to the Perl version I ran at the time. I can revert commit 1430108 if you prefer. |
There was a problem hiding this comment.
I'm sorry, I didn't have time to test this yet. I wouldn't be against updating some of the dependencies... Do you think the XML::Simple update might cause issues?
As for moving around files requiring a binary: I'm not sure that's a great problem. You and I know that the way we're handling dependencies isn't great. But you'd basically have to put dozens of copies of the same files all over the place, right?
| my $r = $_[0]->{'_params'}->{ $_[1] }; | ||
| return ($r ne '') ? $r : undef; |
There was a problem hiding this comment.
Why not return whatever was stored? Even if it was an empty string, a caller might actually expect a string. Changing this could cause a lot of issues. IMHO there's nothing wrong with the current implementation.
There was a problem hiding this comment.
I wish I had been more verbose when I created that patch. I've been using it for at least 8 years already.
Browsing the code I think the reason for changing it was probably because a construct like this:
my $sort = $request->getParam('sort') || 'title';
would never return 'title' unless that was what sort actually stated.
In hindsight a better version would probably be:
return defined($_[0]->{'_params'}->{ $_[1] })? $_[0]->{'_params'}->{ $_[1] } : undef;
There was a problem hiding this comment.
That $sort would very well become title if a string was returned. undef, '', or 0 are all falsy values in Perl, thus would fall back to title in the above case.
I see no reason to change this. It has been this way in almost two decades, does what I'd expect it to do. And nobody has complained so far (besides you, of course 😁).
|
The problem with files that are linked to a binary starts with systems for which no binary exists in the release, which is a given for the noCPAN tarball. Of course the safe way to fix that is to run the buildme.sh script from slimserver-vendor, which was not clear to me at all when I first started building LMS to run on Gentoo some 8 years back. Then again this does not lead to a guaranteed success because ModuleLoader will use the application specified search path to find the module but prefers the system library path to find the associated binary. Which leads to entirely new issues if you get e.g. XML::Parser from a system supplied package, or if on a non LMS dedicated machine some other app pulled that in from system. Ideally no such conflicts should rise when starting with noCPAN but it does because it contains modules that should have been in |
|
The real solution to the conflict situation you describe probably would be to not ship any CPAN modules at all with the noCPAN tarball. It's indeed a bit problematic to ship the Perl part only, and then expect the binaries to match. Would that simplify your use case? |
|
That would actually be a very bad idea, especially given the fact that slimserver has a very strict dependency on an older version of DBIx. It is also not a problem for multiple versions of a single module to exist on the same system, the version mismatch problem is strictly confined to those modules that do have a binary companion. Should note that above is not an issue for the Gentoo package that I am maintaining because I added a routine that recursively deletes all of the module instances that are duplicated. As such I can also easily fix the issue on this platform by adding an additional package dependency on Carp::Assert which will cause the offending version to be cleaned out as well. The Gentoo way however is to prefer issues to be fixed upstream. |
A couple of old nuisances.
Two of the issues addressed here have been generating warnings in Perl since at least version 5.20. The third one (Carp::Assert) is fairly recent but it is stated that this will be generating an error from Perl 5.40 onward instead.
I have another older warning same as the one in XML::Simple however this module is linked to an arch specific library and so shouldn't be in the root CPAN folder in the first place. I'll create another pull request for moving that incorrectly located module, as well as several others since these have proven to be the cause of numerous version conflicts on lesser supported systems.