-
-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
bpo-30912: Don't check ffi.h for specific contents #2687
Conversation
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. Thanks again to your contribution and we look forward to looking at it! |
setup.py
Outdated
@@ -2003,11 +2003,11 @@ def detect_ctypes(self, inc_dirs, lib_dirs): | |||
ffi_inc = find_file('ffi.h', [], inc_dirs) | |||
if ffi_inc is not None: | |||
ffi_h = ffi_inc[0] + '/ffi.h' | |||
# Just check that the file is not empty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking that the header exists and not empty is strange. If you install an empty ffi header failing the build seems like the best way.
I would check why the code checking for LIBFFI_H was added.
If we really want to know if there is a non empty file we can just do:
os.path.getsize(ffi_h) > 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on my "git blame"-fu it was introduced by this commit which is a large merge commit from a non-existent branch in the git repo:
commit 49fd7fa
Author: Thomas Wouters [email protected]
Date: Fri Apr 21 10:40:58 2006 +0000
Merge p3yk branch with the trunk up to revision 45595. This breaks a fair
number of tests, all because of the codecs/_multibytecodecs issue described
here (it's not a Py3K issue, just something Py3K discovers):
http://mail.python.org/pipermail/python-dev/2006-April/064051.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the real origin is bpo-1464444, in which @doko42 noted that the check for #define LIBFFI_H
may or may not be worth much. At this point, I don't think we need to even check that there is anything in the file, just that ffi.h
exists.
On Thu, 13 Jul 2017 14:25:55 +0000 (UTC) Zachary Ware ***@***.***> wrote:
zware commented on this pull request.
> @@ -2003,11 +2003,11 @@ def detect_ctypes(self, inc_dirs, lib_dirs):
ffi_inc = find_file('ffi.h', [], inc_dirs)
if ffi_inc is not None:
ffi_h = ffi_inc[0] + '/ffi.h'
+ # Just check that the file is not empty
Looks like the real origin is
[bpo-1464444](https://bugs.python.org/issue1464444), in which @doko42 noted
that the check for `#define LIBFFI_H` may or may not be worth much. At this
point, I don't think we need to even check that there is anything in the
file, just that `ffi.h` exists.
Thanks! I'll amend it.
…--
-----------------------------------------------------------------
Shlomi Fish http://www.shlomifish.org/
What Makes Software Apps High Quality - http://shlom.in/sw-quality
Buffy: I’m so glad we’re not one of those married-plus couples that don’t know
any single people.
— http://www.shlomifish.org/humour/Buffy/A-Few-Good-Slayers/
Please reply to list if it's a mailing list post - http://shlom.in/reply .
|
'ffi_wrapper_h'.format(ffi_h)) | ||
if not os.path.exists(ffi_h): | ||
ffi_inc = None | ||
print('Header file {} does not exist'.format(ffi_h)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
But we need a proper commit message describing this change. The previous patch commit message is not relevant any more, and this patch commit message is not useful. People looking at python git history should not have to search github pull requests comments.
I think the proper way to do this would be to fixup this change into the previous patch, and upgrade the commit message to describe the actual change.
4b4956a
to
4b85973
Compare
On Fri, 14 Jul 2017 17:10:06 +0000 (UTC) Nir Soffer ***@***.***> wrote:
nirs requested changes on this pull request.
> @@ -2003,16 +2003,9 @@ def detect_ctypes(self, inc_dirs, lib_dirs):
ffi_inc = find_file('ffi.h', [], inc_dirs)
if ffi_inc is not None:
ffi_h = ffi_inc[0] + '/ffi.h'
- # Just check that the file is not empty
- with open(ffi_h) as f:
- for line in f:
- line = line.strip()
- if len(line) > 0:
- break
- else:
- ffi_inc = None
- print('Header file {} does not define LIBFFI_H or '
- 'ffi_wrapper_h'.format(ffi_h))
+ if not os.path.exists(ffi_h):
+ ffi_inc = None
+ print('Header file {} does not exist'.format(ffi_h))
Looks good.
But we need a proper commit message describing this change. The previous
patch commit message is not relevant any more, and this patch commit message
is not useful. People looking at python git history should not have to search
github pull requests comments.
I think the proper way to do this would be to fixup this change into the
previous patch, and upgrade the commit message to describe the actual change.
Thanks for your comments, Nir!
I've squashed the two commits with a reworked and combined commit message, per
the instructions here -
https://stackoverflow.com/questions/5189560/squash-my-last-x-commits-together-using-git#5201642 .
Please let me know if there's anything else that should be done.
…--
-----------------------------------------------------------------
Shlomi Fish http://www.shlomifish.org/
Original Riddles - http://www.shlomifish.org/puzzles/
The Messiah will come when everybody assume his role.
— http://www.shlomifish.org/philosophy/the-eternal-jew/
Please reply to list if it's a mailing list post - http://shlom.in/reply .
|
The body of the commit message looks ok, but the title:
Does not match other commits in the git log. I don't know if this is required, but I would
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, but I think @doko42 should approve this, since he added the code removed by this patch.
See http://bugs.python.org/issue30912 - ffi.h does not match the substrings on Mageia v6. Now we just check that it exists, otherwise it is not our problem. (Fixed per the comments on the pull-req at python#2687 (comment) . ).
4b85973
to
4aadd1c
Compare
@@ -2003,16 +2003,9 @@ def detect_ctypes(self, inc_dirs, lib_dirs): | |||
ffi_inc = find_file('ffi.h', [], inc_dirs) | |||
if ffi_inc is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't checked the implementation of find_file
, but it looks like it probably takes care of the os.path.exists
check and I'm not sure that the printed message is worth a whole lot in the grand spam of things. Could you check on that and probably just remove this whole branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zware: the problem is that find_file does not always get called and moreover that message may be useful to pinpoint a build problem,
Hi all! Do you need anything else from me or may this pull-req be merged already? It's been over a month. |
@zware : thanks! I can wait for now. |
See http://bugs.python.org/issue30912 - ffi.h does not match the
substrings on Mageia v6. Now we just check that it is not empty,
otherwise it is not our problem. Notifying @zware per his request.