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

Hint users to install development package for missing `pyconfig.h` #3348

Closed
wants to merge 3 commits into
base: develop
from

Conversation

Projects
None yet
2 participants
@mishaturnbull
Contributor

mishaturnbull commented Feb 28, 2018

  • On missing pyconfig.h file, prompt to install python-dev
  • Add .spyprojectto .gitignore

See #1539 for discussion that prompted this pull request.

@htgoebel

htgoebel requested changes Feb 28, 2018 edited

Thanks for this pull-request. Overall it looks good, just a few adjustments to be done.

Also please split the commit into two: One for the functional change and one for gitignore. https://pyinstaller.readthedocs.io/en/latest/development/pull-requests.html#updating-a-pull-request might be of use.

Thanks.

@@ -492,6 +492,20 @@ def format_binaries_and_datas(binaries_or_datas, workingdir=None):
src_root_paths = glob.glob(src_root_path_or_glob)

if not src_root_paths:

This comment has been minimized.

@htgoebel

htgoebel Feb 28, 2018

Member

Please remove this empty line.


# on Debian/Ubuntu, a missing pyconfig.h can be fixes with
# installing python-dev
if 'pyconfig.h' in src_root_path_or_glob:

This comment has been minimized.

@htgoebel

htgoebel Feb 28, 2018

Member

Hmm, this would also match "lalalpyconfig.home". I suggest testing for src_root_path_or_glob.endswith("pyconfig.h"). What do you think?

This comment has been minimized.

@mishaturnbull

mishaturnbull Feb 28, 2018

Contributor

Agreed -- I hadn't thought of that.

* apt-get install python3-dev
* apt-get install python-dev
* If you're building Python by yourself, please rebuild your Python with `--enable-shared` (or, `--enable-framework` on Darwin)
""" % src_root_path_or_glob

This comment has been minimized.

@htgoebel

htgoebel Feb 28, 2018

Member

Please wrap these ling lines at 75 characters. I know the original place is not wrapped either, but we should not inherit this fault.

* apt-get install python3-dev
* apt-get install python-dev
* If you're building Python by yourself, please rebuild your Python with `--enable-shared` (or, `--enable-framework` on Darwin)
""" % src_root_path_or_glob

This comment has been minimized.

@htgoebel

htgoebel Feb 28, 2018

Member

Having two raise statements side-by side is a bit hard to read IMHO. Also the text of the message is duplicate. How about this:

if not src_root_paths:
    msg = 'Unable to find "%s" when adding binary and data files.' % (
            src_root_path_or_glob))
    if src_root_path_or_glob.endswith(…)
        msg += """\n…"""
    raise SystemExit(msg)

What do you think?

This comment has been minimized.

@mishaturnbull

mishaturnbull Feb 28, 2018

Contributor

I agree. Should I do the raise with an empty line before it? I think that might be more readable:

if not src_root_paths:
    msg = 'Unable to find "%s" when adding binary and data files.' % (
                  src_root_path_or_glob))
    if src_root_path_or_glob.endswith("pyconfig.h"):
        msg += """blah
...
blah"""
        
    raise SystemExit(msg)

This comment has been minimized.

@htgoebel

htgoebel Feb 28, 2018

Member

If any I'd prefer an empty line behind. But both may fail the linter :-}

This comment has been minimized.

@mishaturnbull

mishaturnbull Mar 1, 2018

Contributor

I'll pray to the lint gods that it passes!

@mishaturnbull

This comment has been minimized.

Contributor

mishaturnbull commented Mar 1, 2018

I made the changes requested to the code. I did my best at splitting the commit in two, but it seems that I now just have three commits and a merge that all do the same thing... I really need to get a better understanding of Git.

@htgoebel

This comment has been minimized.

Member

htgoebel commented Mar 1, 2018

Thanks for trying :-) git is a powerful tool but with power comes complexity.

Wasn't https://pyinstaller.readthedocs.io/en/latest/development/pull-requests.html#updating-a-pull-request helpful? What is missing there?

@mishaturnbull

This comment has been minimized.

Contributor

mishaturnbull commented Mar 1, 2018

I followed that guide, but whenever I tried to push I got a "detached HEAD" error message. I think it was caused by the fact that I had already pushed changes to origin.

@htgoebel

This comment has been minimized.

Member

htgoebel commented Mar 1, 2018

So something is missing (or unclear) in that description. I assume you've been in "detached" mode earlier already. Did you checkout on a commit-id instead of a branch-name?

@mishaturnbull

This comment has been minimized.

Contributor

mishaturnbull commented Mar 1, 2018

I may well have done that... should I try it again?

@htgoebel

This comment has been minimized.

Member

htgoebel commented Mar 1, 2018

Yes please :-)

mishaturnbull added some commits Mar 2, 2018

@mishaturnbull mishaturnbull force-pushed the mishaturnbull:develop branch from fbd1205 to 9143694 Mar 2, 2018

@mishaturnbull

This comment has been minimized.

Contributor

mishaturnbull commented Mar 2, 2018

I think I got it! Two commits, showing on my branch as the only two since the fork point. Hope this looks good :)

@mishaturnbull

This comment has been minimized.

Contributor

mishaturnbull commented Mar 3, 2018

Any ideas why the CI checks are failing? The TravisCI hasn't yet built successfully on any of my commits, but the AppVeyor was working yesterday... seems to be something about a missing nspkg. Is this my fault?

@htgoebel htgoebel closed this Mar 6, 2018

@htgoebel htgoebel reopened this Mar 6, 2018

@htgoebel

This comment has been minimized.

Member

htgoebel commented Mar 6, 2018

I think I got it! Two commits, showing on my branch as the only two since the fork point. Hope this looks good :)

:-)

Ci tests partially failed due to issues with OSX.I just closes and reopend the pull-request to have test running merged with current HEAD.

Fix style issues
- Fix hanging indent on line 496
- Remove trailing space on 506
@mishaturnbull

This comment has been minimized.

Contributor

mishaturnbull commented Mar 7, 2018

Sounds good -- looks like I failed the TravisCI linters for a few minor issues, which I've fixed and committed. The other failure in Travis was in the nightly build, regarding CDLL's, which I don't believe has to do with my changes. If I'm wrong, let me know...

@htgoebel

This comment has been minimized.

Member

htgoebel commented Mar 12, 2018

Thanks for the pull-request.
Merged manually after updating the commit-messages.

@htgoebel htgoebel closed this Mar 12, 2018

@htgoebel htgoebel added this to the PyInstaller 3.4 milestone Mar 12, 2018

@mishaturnbull

This comment has been minimized.

Contributor

mishaturnbull commented Mar 14, 2018

Awesome! Thanks for the help!

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