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

Build: OSX: Add new hierarchical handling of info_plist. #3532

Merged
merged 1 commit into from May 29, 2018

Conversation

Projects
None yet
3 participants
@itsayellow
Contributor

itsayellow commented May 22, 2018

Some useful items in Info.plist of a Mac bundle involve keys referencing
other arrays or dicts. (e.g. CFBundleDocumentTypes or
UTExportedTypeDeclarations) Before this commit all of these items would
have to be added outside of pyinstaller by hand.

This commit adds a recursive parsing function so that a hierarchical
info_plist python object in a spec file can be parsed into a
hierarchical Info.plist dict. It also properly tabs the resulting XML.

  • Python dicts resolve to XML <dict>
  • Python lists resolve to XML <array>
  • All other Python items are added as an XML <string> (as before)
)
info_plist_txt += u"\t"*indent + u"</dict>\n"
else:
info_plist_txt += u"\t"*indent + u"<string>%s</string>\n" % plist_obj

This comment has been minimized.

@bjones1

bjones1 May 22, 2018

Member

I'd suggest verifying that plist_obj is a string here via an assert.

This comment has been minimized.

@itsayellow

itsayellow May 22, 2018

Contributor

I can definitely do that, but my thinking here is that this falls back on the past behavior, which was just to declare anything this code can't handle as a string.

Also, is there a good Exception I could raise? It might be nicer than bare assertions. I wasn't sure about raising an Exception because I didn't see any in this file.

This comment has been minimized.

@bjones1

bjones1 May 22, 2018

Member

That's a good point. Given this is current behavior, perhaps something like

from Pyinstaller.compat import string_types
from Pyinstaller import log as logging

logger = logging.getLogger(__name__)

    # In your code
    if not isinstance(plist_obj, string_types):
        logger.warning('Provided Info.plist object %s is not a string.', plist_obj)

This comment has been minimized.

@itsayellow

itsayellow May 22, 2018

Contributor

Similarly, should I change the assert indent < 16 to something that writes a logger.error, and then a blank raise, as I see for example in building/utils.py line 285?

This comment has been minimized.

@itsayellow

itsayellow May 22, 2018

Contributor

Also, I added your logger.warning, thanks.

@itsayellow

This comment has been minimized.

Contributor

itsayellow commented May 23, 2018

I think I integrated your suggestions, and also caused hitting the recursion limit to make a logger.error and raise an Exception.

The CI failures are for ipython, which I noticed in my own tests was failing before I made any changes (pre-existing.)

Let me know if there's anything else I should fix, and thanks for the suggestions.

@bjones1

This comment has been minimized.

Member

bjones1 commented May 24, 2018

Is there any way to test your code? That is, some Mac library that would check via a Mac OS call that a particular Info.plist value has the expected contents? Given this code, I'd be happy to help work it in to the Pyinstaller test framework.

I didn't understand the recursion limit problem -- what's going on there?

@itsayellow

This comment has been minimized.

Contributor

itsayellow commented May 24, 2018

The recursion limit is to prevent a "reference loop" from causing an infinite recursion. For example:

>>> b=[]
>>> a=[b,]
>>> b.append(a)
>>> a
[[[...]]]
>>> b
[[[...]]]

If you recurse into a, the recursion will never stop, as there is no end to the depth.
This is just a simple example, but it is possible to come up with more accidental and less obvious versions of such a loop.
I just wanted to make sure that no matter what, the recursion would stop at some reasonable limit.

As to checking Info.plist, that's a good question. Most basically you can run plutil from the mac Terminal, which will by default do a lint on the Info.plist file:

>$ plutil Info.plist 
Info.plist: OK

A search on the internet found the following tool. It seems to work but I can't vouch for it more than that:
https://www.jwwalker.com/pages/infoplistchecker.html

@itsayellow

This comment has been minimized.

Contributor

itsayellow commented May 24, 2018

Actually in my tryout of infoplistchecker, I found that my code is missing at least one type it should be processing: boolean. Certain Info.plist items can be </false> or </true> (but NOT <string>False</string>)

I will add this type to the code, and investigate if there's something else that could be missing.

@itsayellow

This comment has been minimized.

Contributor

itsayellow commented May 24, 2018

And with yet more research, it seems that there is plistlib which is actually part of the python standard library!
https://docs.python.org/3.6/library/plistlib.html

This seems like the best way to go for me, mature complete code that doesn't add an external dependency.

I'll look into using this module.

@bjones1

This comment has been minimized.

Member

bjones1 commented May 24, 2018

Wow, that's a great find -- thanks for taking the time to do the searching!

@itsayellow

This comment has been minimized.

Contributor

itsayellow commented May 25, 2018

With pylistlib I've managed to reduce the code for generating Info.plist to just a few lines. And I believe it is much more robust and complete than before.

plistlib allows for nested objects, and various datatypes besides string that are correctly identified in the xml (e.g. strings, integers, floats, booleans, tuples, lists, dictionaries (but only with string keys), Data, bytes, bytesarray or datetime.datetime objects.)

I've also double-checked (on python 3.6) that this works with unicode string inputs to guard against a regression of the reason for PR #2615. I've also verified that plistlib writes out UTF-8, including unicode strings that are able to be encoded in UTF-8.

# python >= 3.4
with open(plist_filename, "wb") as plist_fh:
plistlib.dump(info_plist_dict, plist_fh)
except AttributeError:

This comment has been minimized.

@bjones1

bjones1 May 25, 2018

Member

You can drop this -- Python 3.3 is end of life, so I'd rather not put code in to support it.

This comment has been minimized.

@itsayellow

itsayellow May 25, 2018

Contributor

Actually that's for Python 2.7.

I could change the comment to: "For python 2.7" ?

This comment has been minimized.

@bjones1

bjones1 May 25, 2018

Member

That would be helpful.

This comment has been minimized.

@htgoebel

htgoebel May 26, 2018

Member

writePlist seams to accept an optn file, too. Thus I I suggest changing this into

with open(plist_filename, "wb") as plist_fh: 
    try:
        plistlib.dump(info_plist_dict, plist_fh)
    except AttributeError:
         # is_py2
        plistlib.writePlist(info_plist_dict, plist_filename)

Ths is_py2 to to find this place when we will remove support for Python 2 in 2020.

@bjones1

This comment has been minimized.

Member

bjones1 commented May 25, 2018

Very nice. Thanks for your contribution! Please clean up the Python 3.3 code, and I'll merge.

@htgoebel htgoebel added the OS X label May 26, 2018

@htgoebel

This comment has been minimized.

Member

htgoebel commented May 26, 2018

Thankd for thsi update. Using plistlib is a great idea! (When I started reading this pull-requests comments I was about suggesting to use some xml lib, but plistlib is even better :-)

@bjones1 When merging, please use a "squash" merge and clean up the commit message. Or even better @itsayellow could squash and update this pull-request, Thanks.

@bjones1

This comment has been minimized.

Member

bjones1 commented May 26, 2018

@itsayellow, thanks for your contributions. Do you know how to squash commits and edit log messages, or shall I? See standard prefixes and commit messages.

@itsayellow

This comment has been minimized.

Contributor

itsayellow commented May 26, 2018

I don't know how to squash commits, and I've only edited past log messages once or twice. But if possible I'd like to learn. Do you have some specific recommendations on how to combine all of my commits with git into one commit?

@itsayellow

This comment has been minimized.

Contributor

itsayellow commented May 26, 2018

Do I do the following?:
git rebase -i 3d411edf89659205

  • squash every commit except for the first one on the branch
  • reword the remaining commit
@bjones1

This comment has been minimized.

Member

bjones1 commented May 27, 2018

Yes, that's it. Then force push the update. (I use smartgit, which has a nice GUI and makes rebase/squash/etc. fairly easy.)

Build: OSX: Use plistlib to write Info.plist.
plistlib is part of the python standard library, and allows converting
many different python object types to their equivalents in a Info.plist
file, in addition to allowing nested python objects to be converted to
nested XML items in an Info.plist file.

plistlib handles the following python datatypes, converting them to
the proper Info.plist XML type: strings, integers, floats, booleans,
tuples, lists, dictionaries (but only with string keys), Data, bytes,
bytesarray or datetime.datetime objects.)

@itsayellow itsayellow force-pushed the itsayellow:info_plist_hierarchical branch from 2724474 to 1cbf072 May 27, 2018

@itsayellow

This comment has been minimized.

Contributor

itsayellow commented May 27, 2018

I think I squashed it and reworded it correctly. It should be ready to go if everything checks out with you all.

@htgoebel

This comment has been minimized.

Member

htgoebel commented May 28, 2018

@itsayellow May I ask you to also take care about #3450? Thanks!

@itsayellow

This comment has been minimized.

Contributor

itsayellow commented May 28, 2018

Yes I'd be happy to.

@bjones1 bjones1 merged commit a550574 into pyinstaller:develop May 29, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bjones1

This comment has been minimized.

Member

bjones1 commented May 29, 2018

Thanks for your contribution!

@itsayellow itsayellow deleted the itsayellow:info_plist_hierarchical branch Jun 8, 2018

@htgoebel htgoebel added this to the PyInstaller 3.4 milestone Sep 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment