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.

Sign up for GitHub

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

Jump to bottom

Add Python implementation of FileIO #66058

Closed
serhiy-storchaka opened this issue Jun 24, 2014 · 21 comments
Closed

Add Python implementation of FileIO #66058

serhiy-storchaka opened this issue Jun 24, 2014 · 21 comments
Assignees
@serhiy-storchaka
Labels
stdlib Python modules in the Lib dir topic-IO type-feature A feature request or enhancement

Comments

@serhiy-storchaka
Copy link
Member

BPO 21859
Nosy @pitrou, @vstinner, @benjaminp, @alex, @vadmium, @serhiy-storchaka
Dependencies
  • bpo-23668: Support os.ftruncate on Windows
    		•
    Files
  • pyio_fileio.patch
  • pyio_fileio_2.patch
  • pyio_fileio_3.patch
  • pyio_fileio_4.patch
  • pyio_fileio_5.patch
  • pyio_fileio_6.patch
  • pyio_fileio_7.patch
  • pyio_fileio_8.patch
  • pyio_fileio_9.patch
  • pyio_fileio_10.patch
  • pyio_fileio_fix.patch
    		•

    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 = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2015-05-12.11:06:09.075>
created_at = <Date 2014-06-24.11:06:53.860>
labels = ['type-feature', 'library', 'expert-IO']
title = 'Add Python implementation of FileIO'
updated_at = <Date 2015-05-12.11:06:09.073>
user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2015-05-12.11:06:09.073>
actor = 'serhiy.storchaka'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2015-05-12.11:06:09.075>
closer = 'serhiy.storchaka'
components = ['Library (Lib)', 'IO']
creation = <Date 2014-06-24.11:06:53.860>
creator = 'serhiy.storchaka'
dependencies = ['23668']
files = ['35769', '35858', '35871', '36032', '36183', '36782', '38651', '38653', '38885', '38888', '38889']
hgrepos = []
issue_num = 21859
keywords = ['patch']
message_count = 21.0
messages = ['221449', '222303', '222375', '223659', '224446', '228240', '239004', '239012', '239046', '239071', '240390', '240402', '240412', '240415', '240416', '240420', '240421', '240429', '240430', '240437', '242961']
nosy_count = 9.0
nosy_names = ['pitrou', 'vstinner', 'benjamin.peterson', 'stutzbach', 'alex', 'python-dev', 'martin.panter', 'piotr.dobrogost', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue21859'
versions = ['Python 3.5']
    @serhiy-storchaka
    Copy link
    Member Author

    Proposed patch adds Python implementation of FileIO in _pyio. This will help to make io and _pyio dependency on _io optional (bpo-17984).

    @serhiy-storchaka serhiy-storchaka added stdlib Python modules in the Lib dir topic-IO type-feature A feature request or enhancement labels Jun 24, 2014
    @serhiy-storchaka
    Copy link
    Member Author

    Updated patch has included recent changes from C implementation (bpo-21679 and bpo-21090).

    @serhiy-storchaka
    Copy link
    Member Author

    Many thanks Victor for your review. Updated patch addresses your comments. It also fixes debugging remnants in test_file_eintr.

    @serhiy-storchaka
    Copy link
    Member Author

    Next iteration of the patch addressed Victor's comments.

    @serhiy-storchaka
    Copy link
    Member Author

    Next iteration of the patch addressed Victor's and Akira's comments.

    @serhiy-storchaka
    Copy link
    Member Author

    Synchronized with the tip. _io.FileIO now behave same as _pyio,FileIO when there is a NUL in name.

    @serhiy-storchaka
    Copy link
    Member Author

    Updated to the tip (added the closefd attribute in repr). os.fstat() is now called only once in the constructor.

    @serhiy-storchaka
    Copy link
    Member Author

    Restored first os.fstat() (however it now is redundant) and addressed most other Victor's comments.

    In general I prefer EAFP over BDFL, and often "except AttributeError" looks better to me than getattr().

    @vstinner
    Copy link
    Member

    I opened the issue bpo-23752: "Cleanup io.FileIO".

    @vstinner
    Copy link
    Member

    New issue bpo-23754: "Add a new os.read_into() function to avoid memory copies".

    @vstinner
    Copy link
    Member

    vstinner commented Apr 9, 2015

    What's the status of the patch? Is it ready to be commited?

    @serhiy-storchaka
    Copy link
    Member Author

    Removed redundant fstat() call (as in bpo-23752). Slightly corrected docstrings (but for larger changes C and Python implementations should be changed consistently).

    I wait only for your approval Victor.

    @vstinner
    Copy link
    Member

    Slightly corrected docstrings (but for larger changes C and Python implementations should be changed consistently).

    I reviewed pyio_fileio_9.patch. The main issue is that I don't understand how self._fd is set to None. In the C implemented, it's set to -1 even if closefd is False.

    For comments on docstrings which also concern the C code, can you maybe open a new issue with a reference to this issue or maybe even a link to my review, if you don't want to modify them in this issue?

    @serhiy-storchaka
    Copy link
    Member Author

    Updated patch addresses Victor's and Berker's comments.

    @vstinner
    Copy link
    Member

    pyio_fileio_10.patch looks good to me. Great job.

    @serhiy-storchaka
    Copy link
    Member Author

    Thank you for your review Victor and others.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 10, 2015

    New changeset 0db36098b908 by Serhiy Storchaka in branch '2.7':
    Issue bpo-21859: Corrected FileIO docstrings.
    https://hg.python.org/cpython/rev/0db36098b908

    New changeset d080f5ecdcd3 by Serhiy Storchaka in branch '3.4':
    Issue bpo-21859: Corrected FileIO docstrings.
    https://hg.python.org/cpython/rev/d080f5ecdcd3

    New changeset 330910697e23 by Serhiy Storchaka in branch 'default':
    Issue bpo-21859: Corrected FileIO docstrings.
    https://hg.python.org/cpython/rev/330910697e23

    New changeset 9ef5765d56b7 by Serhiy Storchaka in branch 'default':
    Issue bpo-21859: Added Python implementation of io.FileIO.
    https://hg.python.org/cpython/rev/9ef5765d56b7

    @serhiy-storchaka
    Copy link
    Member Author

    There are test failures on Windows: http://buildbot.python.org/all/builders/AMD64%20Windows7%20SP1%203.x/builds/6073/steps/test/logs/stdio

    1. ValueError is not raised if file name contains a null character.

    2. os.ftruncate doesn't exist on Windows.

    Here is a patch that adds explicit check for null character and skips tests with not implemented truncate.

    @vstinner
    Copy link
    Member

    1. os.ftruncate doesn't exist on Windows.

    Steve Dower wrote a patch and the latest version looks good to me:
    https://bugs.python.org/issue23668#msg240385

    + if 0 in os.fsencode(file):
    + raise ValueError('embedded null character')

    IMO the check must be implemented in os.open() (in path_converter). It doesn't look right to have a differen behaviour on UNIX and Windows. path_converter() calls PyUnicode_FSConverter() on UNIX and this function checks for embedded null bytes.

    So I would prefer to see the issue bpo-23668 fixed and path_converter() fixed, instead of pushing pyio_fileio_fix.patch.

    @serhiy-storchaka
    Copy link
    Member Author

    Opened bpo-23908 for null characters in paths.

    @serhiy-storchaka
    Copy link
    Member Author

    Both issues are fixed.

    @serhiy-storchaka serhiy-storchaka self-assigned this May 12, 2015
    @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
    Assignees

    @serhiy-storchaka serhiy-storchaka

    Labels
    stdlib Python modules in the Lib dir topic-IO type-feature A feature request or enhancement
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    2 participants
    @vstinner @serhiy-storchaka