bpo-30912: Don't check ffi.h for specific contents by shlomif · Pull Request #2687 · python/cpython

shlomif · 2017-07-12T20:50:28Z

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.

the-knights-who-say-ni · 2017-07-12T20:50:30Z

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!

nirs · 2017-07-12T21:12:11Z

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

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 thomas@python.org
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

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.

shlomif · 2017-07-13T17:16:35Z

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 .

nirs · 2017-07-14T17:08:30Z

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.

shlomif · 2017-07-14T17:48:40Z

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 .

nirs · 2017-07-14T18:06:10Z

The body of the commit message looks ok, but the title:

Fix issue30912 - cannot find "ffi.h" w multiarch.

Does not match other commits in the git log. I don't know if this is required, but I would
use this for consistency:

bpo-30912: Fixed cannot find "ffi.h" with multiarch

zware · 2017-07-14T18:22:38Z

@nirs, @shlomif Don't worry about the commit messages or squashing commits. Leave that to the core contributor who merges the PR. The merge is done by GitHub's 'squash and merge' option, and the merger has to make some edits to the message anyway.

nirs

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) . ).

zware · 2017-07-14T18:32:11Z

@@ -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:


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?

@zware: the problem is that find_file does not always get called and moreover that message may be useful to pinpoint a build problem,

shlomif · 2017-08-23T11:36:45Z

Hi all!

Do you need anything else from me or may this pull-req be merged already? It's been over a month.

zware · 2017-08-23T14:05:14Z

I agree with @nirs that it would be nice to have @doko42's input. If we don't get that before the first week of September, I'll merge it then.

shlomif · 2017-08-23T15:27:49Z

@zware : thanks! I can wait for now.

shlomif · 2017-09-07T11:08:11Z

Thanks for merging it and thus fixing the problem, @zware! Also thanks to @nirs for the commentary.

the-knights-who-say-ni added the CLA not signed label Jul 12, 2017

nirs reviewed Jul 12, 2017

View reviewed changes

zware removed the CLA not signed label Jul 13, 2017

the-knights-who-say-ni added the CLA signed label Jul 13, 2017

nirs suggested changes Jul 14, 2017

View reviewed changes

shlomif force-pushed the issue30912-find-libffi-on-mageia branch from 4b4956a to 4b85973 Compare July 14, 2017 17:41

zware changed the title ~~bpo-30912: fix issue30912 - cannot find "ffi.h" with multiarch.~~ bpo-30912: Don't check ffi.h for specific contents Jul 14, 2017

nirs approved these changes Jul 14, 2017

View reviewed changes

bpo-30912: Fixed cannot find "ffi.h" with multiarch

4aadd1c

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) . ).

shlomif force-pushed the issue30912-find-libffi-on-mageia branch from 4b85973 to 4aadd1c Compare July 14, 2017 18:30

zware reviewed Jul 14, 2017

View reviewed changes

zware approved these changes Sep 6, 2017

View reviewed changes

bedevere-bot added the awaiting merge label Sep 6, 2017

zware merged commit 6d51b87 into python:master Sep 6, 2017

Mariatta removed the awaiting merge label Oct 8, 2017

		@@ -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:

Uh oh!

Conversation

shlomif commented Jul 12, 2017

Uh oh!

the-knights-who-say-ni commented Jul 12, 2017

Uh oh!

nirs Jul 12, 2017

Choose a reason for hiding this comment

Uh oh!

shlomif Jul 12, 2017

Choose a reason for hiding this comment

Uh oh!

zware Jul 13, 2017

Choose a reason for hiding this comment

Uh oh!

shlomif commented Jul 13, 2017 via email

Uh oh!

nirs Jul 14, 2017

Choose a reason for hiding this comment

Uh oh!

shlomif commented Jul 14, 2017 via email

Uh oh!

nirs commented Jul 14, 2017

Uh oh!

zware commented Jul 14, 2017

Uh oh!

nirs left a comment

Choose a reason for hiding this comment

Uh oh!

zware Jul 14, 2017

Choose a reason for hiding this comment

Uh oh!

shlomif Jul 14, 2017

Choose a reason for hiding this comment

Uh oh!

shlomif commented Aug 23, 2017

Uh oh!

zware commented Aug 23, 2017

Uh oh!

shlomif commented Aug 23, 2017

Uh oh!

shlomif commented Sep 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants