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

Separate compilation of time and datetime modules #53258

Closed
abalkin opened this issue Jun 16, 2010 · 24 comments
Closed

Separate compilation of time and datetime modules #53258

abalkin opened this issue Jun 16, 2010 · 24 comments
Labels
OS-windows performance Performance or resource usage

Comments

@abalkin
Copy link
Member

abalkin commented Jun 16, 2010

BPO 9012
Nosy @tim-one, @loewis, @birkenfeld, @rhettinger, @doko42, @mdickinson, @abalkin, @pitrou, @vstinner, @tjguk, @bitdancer, @rnk, @zware, @zooba
Superseder
  • bpo-14180: Factorize code to convert int/float to time_t, timeval or timespec
  • Files
  • timefunc-split.diff
  • issue9012.diff
  • time.diff: patch to build as a statically linked extension
  • issue9012a.diff: Adding _time.c in Setup.dist
  • add_time_to_vc8_build.diff: Corrected relative path in _time.c for VS8 build
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2014-06-22.07:57:17.462>
    created_at = <Date 2010-06-16.15:46:52.636>
    labels = ['OS-windows', 'performance']
    title = 'Separate compilation of time and datetime modules'
    updated_at = <Date 2014-06-22.07:57:17.460>
    user = 'https://github.com/abalkin'

    bugs.python.org fields:

    activity = <Date 2014-06-22.07:57:17.460>
    actor = 'loewis'
    assignee = 'none'
    closed = True
    closed_date = <Date 2014-06-22.07:57:17.462>
    closer = 'loewis'
    components = ['Windows']
    creation = <Date 2010-06-16.15:46:52.636>
    creator = 'belopolsky'
    dependencies = []
    files = ['17684', '17686', '17853', '17854', '18113']
    hgrepos = []
    issue_num = 9012
    keywords = ['patch']
    message_count = 24.0
    messages = ['107929', '107936', '107937', '107961', '107962', '107963', '109215', '109216', '109219', '109221', '109222', '109224', '109225', '110572', '111078', '111103', '111109', '111110', '123892', '123895', '219948', '220064', '221199', '221229']
    nosy_count = 17.0
    nosy_names = ['tim.peters', 'loewis', 'georg.brandl', 'rhettinger', 'doko', 'mark.dickinson', 'belopolsky', 'pitrou', 'vstinner', 'ruseel', 'tim.golden', 'tlesher', 'r.david.murray', 'rnk', 'BreamoreBoy', 'zach.ware', 'steve.dower']
    pr_nums = []
    priority = 'high'
    resolution = 'out of date'
    stage = 'needs patch'
    status = 'closed'
    superseder = '14180'
    type = 'resource usage'
    url = 'https://bugs.python.org/issue9012'
    versions = ['Python 3.2']

    @abalkin
    Copy link
    Member Author

    abalkin commented Jun 16, 2010

    Here is the history of the issue per Martin v. Löwis on python-dev:

    """

    This was added with

    ------------------------------------------------------------------------
    r36221 | bcannon | 2004-06-24 03:38:47 +0200 (Do, 24. Jun 2004) | 3 Zeilen

    Add compilation of timemodule.c with datetimemodule.c to get
    __PyTime_DoubleToTimet().

    ------------------------------------------------------------------------

    So it's clearly intentional. I doubt its desirable, though. If only
    __PyTime_DoubleToTimet needs to be duplicated, I'd rather put that
    function into a separate C file that gets included twice, instead of including the full timemodule.c into datetimemodule.c.
    """
    -- "Sharing functions between C extension modules in stdlib", http://mail.python.org/pipermail/python-dev/2010-June/100587.html

    I hope this is non-controversial, but I would like someone to comment on the choice of file name. I chose _timefunc.c to make it more distinguishable from module file names. Ideally, however I would like to see this in either _time.h/_time.c pair (both in Module dir like _math.{c,h}) or in pytime.h/pytime.c (like pymath.{c,h}).

    Marking this as "resource usage" because the patch will result in smaller size of datetime module.

    @abalkin abalkin self-assigned this Jun 16, 2010
    @abalkin abalkin added the performance Performance or resource usage label Jun 16, 2010
    @abalkin
    Copy link
    Member Author

    abalkin commented Jun 16, 2010

    Based on IRC discussion, here is a modified patch that places C code in _time.c and creates a stub for _time.h so that future shared definitions can go there.

    @abalkin
    Copy link
    Member Author

    abalkin commented Jun 16, 2010

    Added new Module/_time.h to the patch.

    @abalkin
    Copy link
    Member Author

    abalkin commented Jun 16, 2010

    Committed in r82034.

    @abalkin abalkin closed this as completed Jun 16, 2010
    @vstinner
    Copy link
    Member

    Reopen: r82034 broke Windows build

    http://www.python.org/dev/buildbot/all/builders/x86%20Windows7%203.x/builds/802/steps/compile/logs/stdio

    -------
    Build started: Project: pythoncore, Configuration: Debug|Win32
    Linking...
    Creating library D:\cygwin\home\db3l\buildarea\3.x.bolen-windows7\build\PCbuild\python32_d.lib and object D:\cygwin\home\db3l\buildarea\3.x.bolen-windows7\build\PCbuild\python32_d.exp
    datetimemodule.obj : error LNK2019: unresolved external symbol __PyTime_DoubleToTimet referenced in function _date_local_from_time_t
    timemodule.obj : error LNK2001: unresolved external symbol __PyTime_DoubleToTimet
    D:\cygwin\home\db3l\buildarea\3.x.bolen-windows7\build\PCbuild\\python32_d.dll : fatal error LNK1120: 1 unresolved externals
    Build log was saved at "file://D:\cygwin\home\db3l\buildarea\3.x.bolen-windows7\build\PCbuild\Win32-temp-Debug\pythoncore\BuildLog.htm"
    -------

    @vstinner vstinner reopened this Jun 16, 2010
    @vstinner
    Copy link
    Member

    Reopen: r82034 broke Windows build

    Fixed by r82035.

    @doko42
    Copy link
    Member

    doko42 commented Jul 4, 2010

    this breaks the build if the time and datetime extensions are built statically into the python interpreter.

    The build fails with a link error; adding the _time extension to Modules/Setup.dist

    @@ -160,6 +160,7 @@
     #cmath cmathmodule.c _math.c # -lm # complex math library functions
     #math mathmodule.c _math.c # -lm # math library functions, e.g. sin()
     #_struct _struct.c     # binary structure packing/unpacking
    +#_time _time.c # # segregated code shared between time and datetime modules
     #time timemodule.c # -lm # time operations and variables
     #operator operator.c   # operator.add() and similar goodies
     #_weakref _weakref.c   # basic weak reference support

    you get another link error:

    libpython3.2.a(config.o):(.data+0x58): undefined reference to `PyInit__time'
    collect2: ld returned 1 exit status

    because _time isn't a proper extension.

    raising the severity as this is a regression

    @doko42 doko42 reopened this Jul 4, 2010
    @doko42
    Copy link
    Member

    doko42 commented Jul 4, 2010

    please find attached a patch to build as a statically linked extension, registering the empty _time module.

    @abalkin
    Copy link
    Member Author

    abalkin commented Jul 4, 2010

    _time.c is not supposed to be compiled into an extension. It is similar to _math.c - if you add a line

    _math _math.c

    to Setup, you get a similar compile error. What needs to be done, however, is to add _time.c to time and datetime lines. See issue9012a.diff.

    @doko42
    Copy link
    Member

    doko42 commented Jul 4, 2010

    doesn't work:

    ranlib libpython3.2.a
    Modules/_time.o: In function _PyTime_DoubleToTimet': /scratch/packages/python/3.2/python3.2-3.2~~20100704/build-shared/../Modules/_time.c:11: multiple definition of _PyTime_DoubleToTimet'
    Modules/_time.o:/scratch/packages/python/3.2/python3.2-3.2~~20100704/build-shared/../Modules/_time.c:11: first defined here
    collect2: ld returned 1 exit status
    ln: accessing `libpython3.2.so.1.0': No such file or directory
    make[1]: *** [libpython3.2.so] Error 1

    @mdickinson
    Copy link
    Member

    Matthias, does building both the math and cmath modules statically into the interpreter fail in a similar manner?

    @doko42
    Copy link
    Member

    doko42 commented Jul 4, 2010

    yes, same issue with math/cmath.

    @mdickinson
    Copy link
    Member

    Thanks. Looks like we need a general solution to the problem of shared (non-module) dependencies in modules.

    So all that's needed is a way to stop _time.o showing up twice in MODOBJS in the Makefile, right?

    (For some reason, ranlib on OS X doesn't seem to have any problem with multiply-defined symbols; obviously Linux is different.)

    @abalkin
    Copy link
    Member Author

    abalkin commented Jul 17, 2010

    I am merging in the nosy list from bpo-9079 after we had a lengthy discussion there and on IRC about the best way to share code between stdlib extension modules.

    For the bpo-9079, we decided to bring the shared code into python core, but this cannot be a general solution. I still don't see any solution other than exposing C API via a capsule mechanism, but Antoine objected saying that capsule mechanism is for 3rd party modules and should not be used in stdlib.

    @tlesher
    Copy link
    Mannequin

    tlesher mannequin commented Jul 21, 2010

    Added patch that replicates the change in r82035 for Visual Studio 2005 (VC8) builds.

    @abalkin
    Copy link
    Member Author

    abalkin commented Jul 21, 2010

    What about PC/VS7.1/pythoncore.vcproj? Is there some automation in place to keep the project files in sync? ISTM, all the info to generate these should be in setup.py.

    The patch looks good, but I am hesitant to commit changes that I cannot test.

    @tlesher
    Copy link
    Mannequin

    tlesher mannequin commented Jul 21, 2010

    No, there's no automated way to keep "legacy" Windows toolchains in sync; short of adopting something like Scons or CMAKE (which I'm *not* suggesting) I don't think I've seen a trustworthy way of doing so.

    The PCBuild's "readme.txt" states:

    "You can find build directories for older versions of Visual Studio and Visual C++ in the PC directory. The legacy build directories are no longer actively maintained and may not work out of the box."

    To date, I believe that the attitude toward these older build files has been "unsupported; use at own risk; patches welcome".

    @abalkin
    Copy link
    Member Author

    abalkin commented Jul 21, 2010

    On Wed, Jul 21, 2010 at 3:58 PM, Tim Lesher <report@bugs.python.org> wrote:
    ..

    To date, I believe that the attitude toward these older build files has been
    "unsupported; use at own risk; patches welcome".

    OK, I guess there is little risk in committing this patch. I'll do it
    later this week unless someone who actually knows anything about VC8
    beats me to it. :-)

    @bitdancer
    Copy link
    Member

    Bump. This bug has priority high and it sounds like the patch is ready for commit.

    @abalkin
    Copy link
    Member Author

    abalkin commented Dec 13, 2010

    it sounds like the patch is ready for commit.

    there are two pending patches here:

    time.diff - a hack that makes _time.c look like a module while it is not.

    add_time_to_vc8_build.diff - a patch for VC 8.0 project file.

    I don't like time.diff because it fixes the symptom rather than the core problem. I believe the code that is shared between C modules should be segregated into a true module which would expose its C API via capsule mechanism. Others suggested that this is an overkill and that capsule mechanism is not suitable for the stdlib. On the latter argument, I would like to point that unicodedata already uses capsule mechanism to make itself available to Python's builtin str type.

    As I mentioned in msg109219, this situation is not unique to time/datetime. The math and cmath modules similarly use linker tricks to share code in _math.c.

    Unassigning because I don't have an informed opinion on add_time_to_vc8_build.diff and cannot move the _time.c issue forward until similar problem is solved for _math.c which I used as the basis for _time.c design.

    @abalkin abalkin removed their assignment Dec 13, 2010
    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Jun 7, 2014

    msg111078 refers to r82035 for Visual Studio 2005 (VC8) builds. Given that http://code.activestate.com/lists/python-dev/131023/ refers to VC14 for 3.5 can we close this as out of date?

    @bitdancer
    Copy link
    Member

    No. The problem has nothing to do with the VS version.

    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Jun 21, 2014

    @Steve/Zach any interest in this one?

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Jun 22, 2014

    This issue has been superseded by bpo-14180. __PyTime_DoubleToTimet no longer exists; its successor now lives in pytime.c.

    @loewis loewis mannequin closed this as completed Jun 22, 2014
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    OS-windows performance Performance or resource usage
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants