bpo-40447: accept all path-like objects in compileall.compile_file #19883

FFY00 · 2020-05-03T12:55:05Z

https://bugs.python.org/issue40447

the-knights-who-say-ni · 2020-05-03T12:55:08Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@FFY00

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

isidentical

Is there a reason to not to use os.fspath for supporting all path like objects?

FFY00 · 2020-05-03T14:43:19Z

Nop, I just totally forgot about that. Good catch :D

hauntsaninja

Thanks! Probably worth adding to the tests too :-)

hauntsaninja · 2020-05-03T19:08:47Z

Lib/compileall.py

@@ -157,8 +157,8 @@ def compile_file(fullname, ddir=None, force=False, rx=None, quiet=0,
        dfile = os.path.join(ddir, name)

    if stripdir is not None:
-        fullname_parts = fullname.split(os.path.sep)
-        stripdir_parts = stripdir.split(os.path.sep)
+        fullname_parts = os.fspath(fullname).split(os.path.sep)


This has redundancy with the logic on line 151. Not sure why that's gated by the condition on quiet, though.

Oh, yes. It is odd.

@berkerpeksag sorry for bothering, do you remember why the quiet check is there? It looks like quiet is being used across the code to provide some set some kind of failing level here. The docstring is also a bit ambiguous.

full output with False or 0, errors only with 1, no output with 2

I am not sure how to parse the behavior for 1.

Should this behavior be changed? If not, the docstring should definitely be touched up.

Anyway, I am moving the path-like object translation up there.

I also added the other path arguments.

taleinat

Thanks for this PR!

This seems like a good fix, but needs a bit more work:

ISTM that this should also fix the handling of the same parameters in compile_dir.
Tests?
Clarify the NEWS entry (see inline comment.)

Misc/NEWS.d/next/Library/2020-05-03-12-55-55.bpo-40447.oKR0Lj.rst

bedevere-bot · 2020-10-18T07:16:43Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

FFY00 · 2020-10-19T21:58:50Z

requested changes; please review again

taleinat · 2020-10-20T06:27:00Z

Thanks @FFY00, that was quick!

The NEWS entry mentions fixing compile_dir as well as compile_file, but I don't see that here yet in the code or tests?

hauntsaninja · 2020-10-20T06:38:03Z

Lib/compileall.py

@@ -152,8 +152,14 @@ def compile_file(fullname, ddir=None, force=False, rx=None, quiet=0,
                          "in combination with stripdir or prependdir"))

    success = True
-    if quiet < 2 and isinstance(fullname, os.PathLike):
+    if isinstance(fullname, os.PathLike):


nit: os.fspath is happy to take str and bytes, so you don't need this check for fullname (for the other three, you do need something to guard against None)

You're right! The last threeos.fspath calls can simply be moved below into each of the blocks handling the three directory arguments, without the isinstance checks. The first one shouldn't need the isinstance guard at all; letting os.fspath error on invalid types seems like precisely what we want here.

Sorry for the delay! This fell off my working pool. Made the changes 😊

hauntsaninja · 2020-11-20T00:37:12Z

Lib/compileall.py

    name = os.path.basename(fullname)

    dfile = None

    if ddir is not None:
-        dfile = os.path.join(ddir, name)
+        dfile = os.path.join(os.fspath(ddir), name)


One more nit: I think the call to os.fspath is only needed on line 165 with stripdir; os.path.join is happy to take PathLike objects

Although actually neither split works when stripdir is bytes, which the documentation says it's allowed to be, since os.path.sep is a str. Same for fullname although the documentation doesn't mention that bytes is allowed there.

I made the changes, let me know if there's anything else.

FFY00 · 2021-10-19T00:58:32Z

This got stale, I now rebased it on main and solved the conflicts. Is there anything else needed?

Signed-off-by: Filipe Laíns <lains@archlinux.org>

FFY00 · 2021-10-25T15:16:18Z

@taleinat or @jaraco, would you be able to have a look if you have time?

taleinat · 2021-10-26T06:00:07Z

@FFY00, I'll try to take a look today.

taleinat · 2021-10-26T06:40:47Z

Lib/compileall.py

@@ -165,7 +164,7 @@ def compile_file(fullname, ddir=None, force=False, rx=None, quiet=0,

    if stripdir is not None:
        fullname_parts = fullname.split(os.path.sep)
-        stripdir_parts = stripdir.split(os.path.sep)
+        stripdir_parts = os.fspath(stripdir).split(os.path.sep)


ISTM it would be clearer to put the input normalization (os.fspath()) up above with the normalization of fullname and name. I.e., add up above:

stripdir = os.fspath(stripdir) if stripdir is not None else None

Suggested change

stripdir_parts = os.fspath(stripdir).split(os.path.sep)

stripdir_parts = stripdir.split(os.path.sep)

taleinat · 2021-10-26T06:43:24Z

Misc/NEWS.d/next/Library/2020-05-03-12-55-55.bpo-40447.oKR0Lj.rst

+Accept :class:`pathlib.Path` in the ``ddir``, ``stripdir`` and ``prependdir``
+arguments of :meth:`compileall.compile_file` and :meth:`compileall.compile_dir`.


This NEWS entry mentions ddir and compile_file(), but this PR still doesn't address those, unless I'm missing something.

I think they already work, so we can just remove their mention from here.

The original version of this PR had some more os.fspath, but that was unnecessary and removed because #19883 (comment)

I agree, their mention should be removed from the NEWS entry.

I think you mean ddir and prependdir, and yes, it seems like they already work due to the os.path.join usage.

taleinat · 2021-10-26T06:45:00Z

@FFY00, this still needs some more attention. If the changes are made quickly then I'll be able to review them promptly.

Signed-off-by: Filipe Laíns <lains@riseup.net>

FFY00 · 2021-10-26T23:52:56Z

@taleinat pushed the requested changes.

jaraco

LGTM, though I haven't reviewed in detail. Let me know if there's something in particular you'd like me to critique.

hauntsaninja

Thanks, this looks correct to me.

There's still one issue that I pointed out in #19883 (comment) , but that is separate enough that we can deal with it later.

I'm not sure whether to backport. While the code itself is "new feature" it is a feature that the docs in previous versions promise exists. We should either a) backport this change, or b) amend the docs on old branches. I lean towards a) for simplicity.

cc @JelleZijlstra

Misc/NEWS.d/next/Library/2020-05-03-12-55-55.bpo-40447.oKR0Lj.rst

the-knights-who-say-ni added the CLA not signed label May 3, 2020

bedevere-bot added the awaiting review label May 3, 2020

FFY00 force-pushed the bpo-40447 branch from 8ec7bcf to ac3f9c1 Compare May 3, 2020

FFY00 changed the title ~~bpo-40447: accept pathlib.path in compileall.compile_file~~ bpo-40447: accept pathlib.Path in compileall.compile_file May 3, 2020

FFY00 force-pushed the bpo-40447 branch 2 times, most recently from 99f62ad to 453d882 Compare May 3, 2020

isidentical reviewed May 3, 2020

View changes

FFY00 force-pushed the bpo-40447 branch 2 times, most recently from 924a7fd to 7b48ce8 Compare May 3, 2020

FFY00 changed the title ~~bpo-40447: accept pathlib.Path in compileall.compile_file~~ bpo-40447: accept all path-like objects in compileall.compile_file May 3, 2020

FFY00 force-pushed the bpo-40447 branch 2 times, most recently from 4a1312b to 6cafeac Compare May 3, 2020

hauntsaninja reviewed May 3, 2020

View changes

FFY00 force-pushed the bpo-40447 branch from 6cafeac to 5096432 Compare May 3, 2020

the-knights-who-say-ni added CLA signed and removed CLA not signed labels May 4, 2020

taleinat requested changes Oct 18, 2020

View changes

Misc/NEWS.d/next/Library/2020-05-03-12-55-55.bpo-40447.oKR0Lj.rst Outdated Show resolved Hide resolved

bedevere-bot added awaiting changes and removed awaiting review labels Oct 18, 2020

FFY00 force-pushed the bpo-40447 branch 2 times, most recently from d63b4e5 to 31251fb Compare Oct 19, 2020

hauntsaninja reviewed Oct 20, 2020

View changes

FFY00 force-pushed the bpo-40447 branch 2 times, most recently from a6a1788 to 599bfd9 Compare Nov 20, 2020

hauntsaninja reviewed Nov 20, 2020

View changes

FFY00 force-pushed the bpo-40447 branch from 599bfd9 to 3672420 Compare Nov 29, 2020

FFY00 force-pushed the bpo-40447 branch from 3672420 to 0d3ccf3 Compare Oct 19, 2021

bpo-40447: accept all path-like objects in compileall.compile_file

45af214

Signed-off-by: Filipe Laíns <lains@archlinux.org>

FFY00 force-pushed the bpo-40447 branch from 0d3ccf3 to 45af214 Compare Oct 19, 2021

taleinat reviewed Oct 26, 2021

View changes

taleinat requested changes Oct 26, 2021

View changes

requested changes

63d02ea

Signed-off-by: Filipe Laíns <lains@riseup.net>

jaraco approved these changes Nov 27, 2021

View changes

bedevere-bot added awaiting merge and removed awaiting changes labels Nov 27, 2021

hauntsaninja mentioned this pull request Jun 21, 2022

compile_file's stripdir does not accept pathlib.Path #84627

Open

ezio-melotti removed the CLA signed label Jul 13, 2022

Merge branch 'main' into bpo-40447

3dba07d

hauntsaninja approved these changes Dec 23, 2022

View changes

Merge branch 'main' into bpo-40447

0426944

hauntsaninja reviewed Dec 23, 2022

View changes

Misc/NEWS.d/next/Library/2020-05-03-12-55-55.bpo-40447.oKR0Lj.rst Outdated Show resolved Hide resolved

Update Misc/NEWS.d/next/Library/2020-05-03-12-55-55.bpo-40447.oKR0Lj.rst

5d56bba

bpo-40447: accept all path-like objects in compileall.compile_file #19883

bpo-40447: accept all path-like objects in compileall.compile_file #19883

FFY00 commented May 3, 2020 •

edited by bedevere-bot

the-knights-who-say-ni commented May 3, 2020

isidentical left a comment

FFY00 commented May 3, 2020

hauntsaninja left a comment

hauntsaninja May 3, 2020 •

edited

FFY00 May 3, 2020

FFY00 May 3, 2020

FFY00 May 3, 2020

taleinat left a comment

bedevere-bot commented Oct 18, 2020

FFY00 commented Oct 19, 2020

taleinat commented Oct 20, 2020

hauntsaninja Oct 20, 2020 •

edited

taleinat Oct 20, 2020 •

edited

FFY00 Nov 20, 2020

hauntsaninja Nov 20, 2020

hauntsaninja Nov 20, 2020 •

edited

FFY00 Nov 29, 2020

FFY00 commented Oct 19, 2021

FFY00 commented Oct 25, 2021

taleinat commented Oct 26, 2021

taleinat Oct 26, 2021

taleinat Oct 26, 2021 •

edited

hauntsaninja Oct 26, 2021 •

edited

taleinat Oct 26, 2021

FFY00 Oct 26, 2021

taleinat commented Oct 26, 2021

FFY00 commented Oct 26, 2021

jaraco left a comment

hauntsaninja left a comment

	stripdir_parts = os.fspath(stripdir).split(os.path.sep)
	stripdir_parts = stripdir.split(os.path.sep)

		Accept :class:`pathlib.Path` in the ``ddir``, ``stripdir`` and ``prependdir``
		arguments of :meth:`compileall.compile_file` and :meth:`compileall.compile_dir`.

bpo-40447: accept all path-like objects in compileall.compile_file #19883

Are you sure you want to change the base?

bpo-40447: accept all path-like objects in compileall.compile_file #19883

Conversation

FFY00 commented May 3, 2020 • edited by bedevere-bot

the-knights-who-say-ni commented May 3, 2020

CLA Missing

isidentical left a comment

FFY00 commented May 3, 2020

hauntsaninja left a comment

hauntsaninja May 3, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taleinat left a comment

bedevere-bot commented Oct 18, 2020

FFY00 commented Oct 19, 2020

taleinat commented Oct 20, 2020

hauntsaninja Oct 20, 2020 • edited

Choose a reason for hiding this comment

taleinat Oct 20, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hauntsaninja Nov 20, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FFY00 commented Oct 19, 2021

FFY00 commented Oct 25, 2021

taleinat commented Oct 26, 2021

Choose a reason for hiding this comment

taleinat Oct 26, 2021 • edited

Choose a reason for hiding this comment

hauntsaninja Oct 26, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taleinat commented Oct 26, 2021

FFY00 commented Oct 26, 2021

jaraco left a comment

hauntsaninja left a comment

FFY00 commented May 3, 2020 •

edited by bedevere-bot

hauntsaninja May 3, 2020 •

edited

hauntsaninja Oct 20, 2020 •

edited

taleinat Oct 20, 2020 •

edited

hauntsaninja Nov 20, 2020 •

edited

taleinat Oct 26, 2021 •

edited

hauntsaninja Oct 26, 2021 •

edited