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

First argument to ctypes.CDLL can be None #2098

Merged
merged 1 commit into from Jul 22, 2016

Conversation

virtuald
Copy link
Contributor

If you try to wrap pip with pyinstaller on Ubuntu 14.04, it fails because it calls ctypes.CDLL(None) (see https://github.com/pypa/pip/blob/8.1.2/pip/pep425tags.py#L188).

This fixes that and adds a test.

@virtuald
Copy link
Contributor Author

Disclaimer: I haven't tested this on Windows, nor have I actually ran the tests... but I assume that Travis will do that for me. :-D

def __init__(self, name, *args, **kwargs):
def _frozen_name(name):
if name is not None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, would name='' be an valid input? I assume if name: would be a better test here.

(When changing this, please remove the trailing whitespace in line 125, too. Thanks.)

@htgoebel
Copy link
Member

Thanks for this fix. I'll merge it when you worked on the one line-commend I had.

@htgoebel htgoebel self-assigned this Jul 17, 2016
@htgoebel htgoebel added this to the PyInstaller 3.3 milestone Jul 17, 2016
@htgoebel htgoebel added the state:needs more work This PR needs more work label Jul 19, 2016
@virtuald
Copy link
Contributor Author

Comments addressed.

@htgoebel htgoebel removed the state:needs more work This PR needs more work label Jul 22, 2016
@htgoebel
Copy link
Member

Thanks for the quick update (and for the hook of course).

@htgoebel htgoebel merged commit 8c577c1 into pyinstaller:develop Jul 22, 2016
@bjones1
Copy link
Contributor

bjones1 commented Jul 22, 2016

Help! This breaks on Windows -- see https://ci.appveyor.com/project/matysek/pyinstaller/build/1232/job/ddjsih612lr1fl84#L1386, for example. Would you fix this?

@virtuald
Copy link
Contributor Author

I'm currently on vacation, no access to an actual computer until August 7.
On Jul 22, 2016 22:58, "Bryan A. Jones" notifications@github.com wrote:Help! This breaks on Windows -- see https://ci.appveyor.com/project/matysek/pyinstaller/build/1232/job/ddjsih612lr1fl84#L1386, for example. Would you fix this?

—You are receiving this because you authored the thread.Reply to this email directly, view it on GitHub, or mute the thread.

@htgoebel
Copy link
Member

This Appveyor really gets me on my nerves. It always breaks for reasons beyond our scope, so yoe never know if ones code will really introduce a bug or it just appveror again.

@bjones1 I'll disable this check for windows for now.

htgoebel added a commit that referenced this pull request Jul 23, 2016
@bjones1
Copy link
Contributor

bjones1 commented Jul 23, 2016

@htgoebel, I agree -- Appveyor has been very frustrating in the past. I'm hoping that recent changes will help make it more useful; in the recent past, it's:

  • Started failing because of a hosting change (Appveyor-induced problem).
  • Flagged a fix needed for cherrypy (good).
  • Just detected a Windows-only problem with ctypes (good).
  • Revealed a problem in Python 2.7 multiprocessing (good).

The biggest problem right now IMHO is speed -- it takes too long to get feedback, particularly yesterday where for some reason it was backed up almost 24 hours. Currently, it's build queue is almost empty, so hopefully that will be getter.

It's working...frustrating at times, but working...

@bjones1
Copy link
Contributor

bjones1 commented Aug 17, 2016

@virtuald, would you work on fixing this? It's still a problem for Windows.

@bjones1
Copy link
Contributor

bjones1 commented Aug 17, 2016

@htgoebel, is there any reason not to merge the fix-2098-on-windows branch? I'd like to get Appveyor working again, and this is a blocker.

@htgoebel
Copy link
Member

@bjones1 pardon, which fix-2098-on-windows branch?

@virtuald
Copy link
Contributor Author

Oh, sorry, fell off my radar. I can do something about this tonight if someone doesn't beat me to it.

@virtuald virtuald deleted the cdll-none branch August 17, 2016 14:20
@bjones1
Copy link
Contributor

bjones1 commented Aug 17, 2016

@htgoebel
Copy link
Member

@bjones1 I don’t know why this branch is not merged. You worked on it, as far as I can see. Is there any reason for not merging it?

@bjones1
Copy link
Contributor

bjones1 commented Aug 17, 2016

Great! I'll merge.

@virtuald
Copy link
Contributor Author

@bjones1 What exactly is the problem on Windows? It appears that 1ec2473 disabled the test on Windows. Or is there another issue?

I get this on linux (Python 2.7.12):

>>> import ctypes
>>> ctypes.CDLL(None)
<CDLL 'None', handle 7fdbd4645128 at 7fdbd44d2910>

I get this on Windows (Python 2.7.12 under wine, but I assume it's the same):

>>> import ctypes
>>> lib = ctypes.CDLL(None)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "....\ctypes\__init__.py", line 362, in __init__
    self._handle = _dlopen(self._name, mode)
TypeError: expected string or Unicode object, NoneType found

Should we duplicate the behavior on Windows?

@bjones1
Copy link
Contributor

bjones1 commented Aug 18, 2016

I see something very similar:

Python 3.5.1 |Anaconda 4.0.0 (32-bit)| (default, Mar  4 2016, 15:28:01) [MSC v.1900 32 bit (Intel)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import ctypes
>>> lib = ctypes.CDLL(None)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Users\bjones\Downloads\Anaconda3\lib\ctypes\__init__.py", line 347, in __init__
    self._handle = _dlopen(self._name, mode)
TypeError: bad argument type for built-in operation
>>>

So, it looks like the behavior of ctypes.CDLL(None) differs on Windows and Linux. I didn't realize this before -- so, the fix already made by @htgoebel is sufficient (skip the test case on Windows). There's no additional work needed; thanks, @virtuald, for helping me see this!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants