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

Fix issue #420 (mh_release.pl errors out on decode_json) #463

Merged
merged 4 commits into from
Sep 10, 2014

Conversation

hollie
Copy link
Owner

@hollie hollie commented Aug 18, 2014

This change fixes the failure that occurs when checking the MisterHouse version. Occurs on OS X 10.9.4 using Perl v 5.18.2.

Before merging this change, please test it out on other (non-OS X) systems.

@hollie hollie changed the title Fix issue #420 Fix issue #420 (mh_release.pl errors on decode_json) Aug 18, 2014
@hollie hollie changed the title Fix issue #420 (mh_release.pl errors on decode_json) Fix issue #420 (mh_release.pl errors out on decode_json) Aug 18, 2014
@hollie hollie added this to the Next stable 3.2 milestone Aug 18, 2014
@hollie hollie added the bug label Aug 18, 2014
@hollie
Copy link
Owner Author

hollie commented Aug 18, 2014

Hmm, I notice this change causes the following notifications on startup of MisterHouse:

Prototype mismatch: sub main::decode_json ($) vs none
Prototype mismatch: sub main::from_json ($@) vs ($)
Prototype mismatch: sub main::to_json ($@) vs ($)

This is a similar error that appeared when Jim was adding support for the Android application.
See here.

Unclear at the moment how to fix this. Do other people also see this?

@JaredF
Copy link
Collaborator

JaredF commented Aug 18, 2014

I do see those warning messages on startup when running this branch:

08/18/2014 09:34:22  Oops1: Prototype mismatch: sub main::from_json ($@) vs ($) at /usr/share/perl/5.18/Exporter.pm line 66.


08/18/2014 09:34:22  Oops1: Prototype mismatch: sub main::to_json ($@) vs ($) at /usr/share/perl/5.18/Exporter.pm line 66.

However, I am able to run a check for an updated MH version and it seems to run successfully:

08/18/2014 09:35:55  Running: Check Misterhouse version
08/18/2014 09:35:55  Retrieving download page
08/18/2014 09:35:55  Checking version...
08/18/2014 09:35:56  Download page retrieved
08/18/2014 09:35:56  Retrieving download date page
mute: Checking version date...
08/18/2014 09:35:56  FUNCTION: set_volume_pre_hook
mute: (chime.wav)
08/18/2014 09:35:57  Download date page retrieved
mute: You are running the development branch, it has no version releases.
08/18/2014 09:35:57  FUNCTION: set_volume_pre_hook
mute: (chime.wav)

I am running on Ubuntu 14.04.1 64-bit Server edition with perl v5.18.2

@hollie
Copy link
Owner Author

hollie commented Aug 18, 2014

I looked further into the JSON and the JSON:PP modules. If I understand it well JSON can redirect calls to submodules with different implementations, the PP (= Pure Perl) module being one of them.

So it is 'normal' that if in your code you both include 'use JSON' (in many MisterHouse files this module is used) and 'use JSON::PP' (in my pull request), and the function prototypes are not identical you get the warning we get.

I think one should always use 'use JSON;' and never one of the submodules directly. So I will discard this pull request (unless somebody has a different opinion on this, then please chime in).

How to fix the version parsing on my system (that only works with the ::PP version of decode_json) is not clear to me yet. It is not a priority to me but on the other hand I'd like to get it working correctly.

I'll close this pull request for the time being.

@hollie hollie closed this Aug 18, 2014
@hollie
Copy link
Owner Author

hollie commented Aug 18, 2014

I have updated the pull request so that there are no longer conflicts reported at startup time. The code works fine on my system now. Can others test and report please?

@hollie hollie reopened this Aug 18, 2014
@krkeegan
Copy link
Collaborator

This works for me, without any warnings on:

Ubuntu 14.4
Perl 5.18.2

@hollie
Copy link
Owner Author

hollie commented Sep 10, 2014

@JaredF could you please double-check if the additional fixes I committed to this branch work fine for you now? If they do then we can safely merge it into master.

@JaredF
Copy link
Collaborator

JaredF commented Sep 10, 2014

@hollie, Sorry for being out of touch lately. I just tested the latest version of your branch and it works as expected with no more warnings at startup. Great work!

@hollie
Copy link
Owner Author

hollie commented Sep 10, 2014

Hey @JaredF, thank you for making time to verify it. 2 positive test reports + mine > I will merge it into master.

@hollie hollie closed this Sep 10, 2014
@hollie hollie reopened this Sep 10, 2014
hollie added a commit that referenced this pull request Sep 10, 2014
Fix issue #420 (mh_release.pl errors out on decode_json)
@hollie hollie merged commit 0c63a1a into master Sep 10, 2014
@hollie hollie deleted the fix_issue_#420 branch September 10, 2014 19:08
@hollie hollie mentioned this pull request Sep 10, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants