Opened 8 years ago

Closed 6 years ago

#2026 closed defect (fixed)

detect Py_hash_t in ./configure and define it internally if not

Reported by: jinmei Owned by: muks
Priority: high Milestone: Sprint-20131015
Component: build system Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Core Feature Depending on Ticket:
Estimated Difficulty: 4 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

From the commit log for 3da2b4d:

[master] use Py_hash_t for return value of tp_hash, and define it for old vers.

The previous code (using long) caused build failure on Solaris.
Python 3.2 changed the return type of internal hash API:
http://docs.python.org/py3k/c-api/object.html#PyObject_Hash

from long to Py_hash_t and solaris seems to be more strict about the
difference of these types.

As noted in the log, that patch is a bit ad hoc (define it for older
python versions referring to PY_MINOR_VERSION, and define it only for
the libdns++ wrapper while in theory this can happen for other
modules) for an urgent care fix.

We should do it more cleanly: my suggestion is to detect the existence
of Py_hash_t in ./configure and if not, define it (or introduce our
internal type name) in some more commonly used header file.

Subtickets

Change History (11)

comment:1 Changed 6 years ago by shane

  • Milestone set to Sprint-20131015
  • Summary changed from detect Py_hash_t in ./configure and define it internally if not to [kean] detect Py_hash_t in ./configure and define it internally if not

comment:2 Changed 6 years ago by kean

  • Owner changed from UnAssigned to kean
  • Status changed from new to accepted

comment:3 Changed 6 years ago by kean

  • Owner changed from kean to UnAssigned
  • Status changed from accepted to reviewing

After analysing this I disagree with the suggestion in the ticket description. The only place Py_hash_t is used is in libdns, and it is already set in a central header file. The check for the python version is at least as good as anything we could do in configure.ac so it is my opinion that nothing needs to change for this. Please can someone review and close if you agree.

comment:4 follow-up: Changed 6 years ago by muks

  • Owner changed from UnAssigned to kean

What the description suggests is detecting the presence of Py_hash_t instead of doing this version check hack. As an example, the current code will fail for future major versions of Python.

A suitable fix would be to check if code with Py_hash_t compiles successfully at configure time, and if not, typedef it to the long type. This typedef can occur in pydnspp_common.h itself for now as it is central enough. But instead of the version test, check the result of the test in configure. This ticket was created for doing that.

In general, we should avoid testing for operating systems, versions and such if we are able to test for bugs, issues and features directly. It will handle regressions, issues with new platforms, etc.

comment:5 in reply to: ↑ 4 Changed 6 years ago by kean

  • Owner changed from kean to UnAssigned

Replying to muks:

What the description suggests is detecting the presence of Py_hash_t instead of doing this version check hack. As an example, the current code will fail for future major versions of Python.

I don't think it's a "hack" at all. Whether or not a new major version of Python comes along is meaningless, we only support Python 3.

In general, we should avoid testing for operating systems, versions and such if we are able to test for bugs, issues and features directly. It will handle regressions, issues with new platforms, etc.

In general, I agree. In this specific case I do not. We only support Python 3, the issue is only extant in minor versions < 2, so the test as it currently stands is perfectly valid. It also keeps the issue centralized, as opposed to spread across different languages and systems. To put the change in configure means that pydnspp_common.h has to change to pydnspp_common.h.in, and get generated by configure with the appropriate typedef. That's a lot of churn in the source for something I just don't see as being a problem.

comment:6 Changed 6 years ago by muks

  • Owner changed from UnAssigned to muks
  • Summary changed from [kean] detect Py_hash_t in ./configure and define it internally if not to detect Py_hash_t in ./configure and define it internally if not

I'll take care of this ticket.

comment:7 Changed 6 years ago by muks

  • Owner changed from muks to UnAssigned

Up for review. We check in configure.ac if the type is available and typedef accordingly. I've added a note that this should be removed when we require Python 3.2 as a minimum dependency.

comment:8 Changed 6 years ago by muks

No ChangeLog is necessary.

comment:9 Changed 6 years ago by kean

  • Owner changed from UnAssigned to kean

comment:10 Changed 6 years ago by kean

  • Owner changed from kean to muks

Verified on FC19. make check passes. Please merge and close.

comment:11 Changed 6 years ago by muks

  • Resolution set to fixed
  • Status changed from reviewing to closed

Merged to master branch in commit e52fc6f0961bb5c54548760b7363b6ccb2bc1020:

* e3913c4 [2026] Detect Py_hash_t in ./configure and define it internally if not

Resolving as fixed. Thank you for the review Kean.

Note: See TracTickets for help on using tickets.