Use pathlib.Path #376

YannickJadoul · 2020-06-15T00:00:59Z

As per #316, we can now play around with pathlib.Path and do it safely, because:

Python 3.6 and above support the file system path protocol, so we can pass Path objects to old os.path functions, etc.
We have typing (let's see how much errors mypy failed to catch)

YannickJadoul · 2020-06-15T00:03:38Z

Argh. Of course: we monkeypatched os.path.exists but not Path.exists :-/

To fix later, then.

YannickJadoul · 2020-06-16T17:35:27Z

Alright, except for Windows on Travis CI (see commit in #378), things seem to be working.

So, @joerick and @Czaki (and perhaps @mayeut?): this PR is officially open for review and discussion on a) whether we want this and b) whether we should extend it to the test & tools code.

joerick

A big +1 from me! Improves readability, and makes things more explicit.

If you're keen, converting the tests would be valuable I think, yes.

cibuildwheel/linux.py

cibuildwheel/macos.py

joerick · 2020-06-16T18:23:11Z

cibuildwheel/macos.py

-        dst = os.path.join(options.output_dir, os.path.basename(repaired_wheel))
-        shutil.move(repaired_wheel, dst)
+        dst = options.output_dir / repaired_wheel.name
+        shutil.move(str(repaired_wheel), dst)


Mayybe this str() can also go?

Yes, I just tested it, and that should work, but mypy was complaining here. I don't really know why :-(
The other alternative is shutting up mypy?

cibuildwheel/util.py

cibuildwheel/__main__.py

joerick · 2020-06-16T18:26:06Z

cibuildwheel/util.py

-        specific_file_path = specific + ext
-        if os.path.exists(specific_file_path):
+        specific_stem = self.base_file_path.stem + f'-python{version_parts[0]}{version_parts[1]}'
+        sepcific_name = specific_stem + self.base_file_path.suffix


typo! sepcific_name -> specific_name

Thanks!
Not sure the os.path to pathlib conversion is worth it here, but let's be consistent?

Czaki

Good PR, but for me not all conversation to Path object are necessary, and reduce readability of code.

cibuildwheel/__main__.py

cibuildwheel/linux.py

cibuildwheel/macos.py

Czaki · 2020-06-16T18:53:45Z

cibuildwheel/macos.py

            # pure Python wheel or empty repair command
-            shutil.move(built_wheel, repaired_wheel_dir)
+            shutil.move(str(built_wheel), repaired_wheel_dir)


Path.rename ?

Hmmm, shutil.move is slightly smarter (just looked it up: apparently it works when the files are on different filesystems), and I suppose /tmp could be on a different partition?

@joerick, do you remember why shutil.move rather than os.rename? Is it worth trying os.rename/Path.rename?

This string conversion here is needed?

@joerick made the same comment already. mypy does something weird:

cibuildwheel/macos.py:210: error: Argument 1 to "move" has incompatible type "Path"; expected "str"

I think it's wrong, but to silence the error, we probably need str(...)

...
Some bug report to mypy?

Turns out it's not wrong... exactly

https://bugs.python.org/issue32689
python/typeshed#3559

@joerick, do you remember why shutil.move rather than os.rename? Is it worth trying os.rename/Path.rename?

Probably just because there are fewer gotchas, as you mention. But Path.rename is worth a shot.

Turns out it's not wrong... exactly

https://bugs.python.org/issue32689
python/typeshed#3559

Ah, thanks for finding that!

Probably just because there are fewer gotchas, as you mention. But Path.rename is worth a shot.

OK, just did that. I think I just figured out á potential reason for it: the second argument to shutil.move (destination) was in some cases (like this one) a directory. I suppose this wouldn't work with rename.

cibuildwheel/macos.py

cibuildwheel/util.py

YannickJadoul · 2020-06-16T22:23:14Z

A big +1 from me! Improves readability, and makes things more explicit.

If you're keen, converting the tests would be valuable I think, yes.

Great. I'll do that, one of these days! :-)

Good PR, but for me not all conversation to Path object are necessary, and reduce readability of code.

I agree it does not always look better, but I think it almost never looks worse than all the sprinkled os.path either. Or do you have examples where you think the old way was better?

That's why I wanted to just first do it, then see how you both feel about it :-) But I think that for consistency, it might be nice to switch as much as possible to pathlib, because: 1) it has the extra layer of typecheck (i.e., we can't confuse paths with strings anymore), and 2) things could become confusing if we keep mixing them?

But I'm open to changing some things back. The current state of this PR was just to show: what would happen if we replace os.path by pathlib everywhere in the code?

YannickJadoul · 2020-06-16T22:45:40Z

Finally, the elephant in the room, the signature of the arguments argument to call/shell:

A Path is not a str and mypy indeed complains (even though dynamically speaking, subprocess would handle Paths perfectly).
I did try changing the signature of call to take a List[Union[str, os.PathLike]] (could just be Path as well, rather than os.PathLike, but there's no real difference here). This results in the complaint that you can't pass a List[str] to a List[Union[str, os.PathLike]] (this would allow you to append a Path to the list inside the function, while the type outside the function doesn't allow that! See https://mypy.readthedocs.io/en/stable/common_issues.html#invariance-vs-covariance).
The previous issue can be solved with Sequence[Union[str, os.PathLike]], which works. Except for e.g. ['python', get_pip_script] + dependency_constraint_flags where mypy does not allow you to concatenate two lists of a different type (even though the types inside the lists do overlap). Try out for yourself:

x = [4]
y: List[Union[str, int]] = [1, 2, '3'] + x

results in

cibuildwheel/__main__.py:29: error: Incompatible types in assignment (expression has type "List[object]", variable has type "List[Union[str, int]]")
cibuildwheel/__main__.py:29: error: Unsupported operand types for + ("List[object]" and "List[int]")

The solution now was intermediate variables with type annotations (or use of typing.cast), like

x: List[Union[str, int]] = [4]
y: List[Union[str, int]] = [1, 2, '3'] + x

or

x = [4]
y: List[Union[str, int]] = [1, 2, '3'] + cast(List[Union[str, int]], x)

but at that point I gave up, as that looked worse than putting str in a few extra places (though this could probably be made to look slightly better by assigning List[Union[str, os.PathLike]] to an alias?). It's especially easy to fix because mypy nicely points out where you need str, and type-wise it might even make sense to turn your Paths into strs before you call call?
But maybe there is a better way of fixing this; I can't imagine that we're the only ones to bump into such a problem? If you have ideas, I'd very happily explore them :-)

bis. Another option would be to silence mypy on that line, of course. But disabling the types on these lines is probably even worse than writing explicit types.

Czaki · 2020-06-18T11:59:46Z

A Path is not a str and mypy indeed complains (even though dynamically speaking, subprocess would handle Paths perfectly).

hmm maybe hide conversion to str in call/shell function and pass Path objects as argument of shell/call?

YannickJadoul · 2020-06-18T12:03:30Z

hmm maybe hide conversion to str in call/shell function and pass Path objects as argument of shell/call?

Did you read the rest of my explanation? I tried that.

I did try changing the signature of call to take a List[Union[str, os.PathLike]]

YannickJadoul · 2020-06-18T14:13:56Z

If you're keen, converting the tests would be valuable I think, yes.

Less occurrences of os.path than I though in there, but done, I think.
So, if we find what to do with call/shell, and you tell me whether to possibly revert a few ones, this is ready :-)

joerick

Nice one @YannickJadoul - let's get this merged :)

YannickJadoul · 2020-06-22T10:12:13Z

Hurray! :-)

So we're giving up on solving the issue with call and shell (cfr. #376 (comment)) for now?

joerick · 2020-06-22T10:19:46Z

So we're giving up on solving the issue with call and shell (cfr. #376 (comment)) for now?

I played with a few approaches and concluded that your approach was neatest. I still don't understand why List[str] + List[Path] doesn't equal List[Union[str, Path]] (or even List[Any]!) but there we go. Maybe if I get time, I'll see if a mypy plugin can help, but for now it's good :)

YannickJadoul · 2020-06-22T10:24:19Z

Great! :-) I had also reasoned it might be fine to force a conversion from the object to a plain string once you're creating a command to run.

See python/mypy#720 and python/mypy#5492, btw, for the interested.

YannickJadoul force-pushed the pathlib branch from 0530902 to 29115ae Compare June 16, 2020 15:07

joerick reviewed Jun 16, 2020

View reviewed changes

Czaki reviewed Jun 16, 2020

View reviewed changes

YannickJadoul added 8 commits June 18, 2020 00:23

Use pathlib.Path in main cibuildwheel files

a7f73f1

Fix paths in unit tests

46a39b0

Remove unused imports

c53e782

Fix order of pathlib.Path.symlink_to arguments in windows.py

b3ffb04

Fix overlooked os.path.abspath

e58aed1

Attend to review remarks

2657088

Replace shutil.move with Path.rename or Path.replace

b62b17a

Mock Path.mkdir calls in unit tests

f012fa1

YannickJadoul force-pushed the pathlib branch 3 times, most recently from 1618a7e to e5267ff Compare June 18, 2020 11:51

YannickJadoul force-pushed the pathlib branch from e5267ff to 363afed Compare June 18, 2020 12:02

YannickJadoul force-pushed the pathlib branch 2 times, most recently from d46b91f to fc0e336 Compare June 18, 2020 12:17

Use patlib.Path in tests and tools

acf5c24

YannickJadoul force-pushed the pathlib branch from fc0e336 to acf5c24 Compare June 18, 2020 13:29

Revert use of Path.joinpath instead of Path.__div__

d552d29

joerick approved these changes Jun 22, 2020

View reviewed changes

YannickJadoul merged commit 9a1f8a9 into pypa:master Jun 22, 2020

YannickJadoul deleted the pathlib branch June 22, 2020 10:28

Czaki mentioned this pull request Jun 24, 2020

OSError: [WinError 17] The system cannot move the file to a different disk drive #388

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use pathlib.Path #376

Use pathlib.Path #376

YannickJadoul commented Jun 15, 2020

YannickJadoul commented Jun 15, 2020

YannickJadoul commented Jun 16, 2020

joerick left a comment

joerick Jun 16, 2020

YannickJadoul Jun 16, 2020

joerick Jun 16, 2020

YannickJadoul Jun 16, 2020

Czaki left a comment

Czaki Jun 16, 2020

YannickJadoul Jun 16, 2020

Czaki Jun 16, 2020

YannickJadoul Jun 16, 2020

Czaki Jun 17, 2020

joerick Jun 17, 2020

joerick Jun 17, 2020

YannickJadoul Jun 17, 2020

YannickJadoul commented Jun 16, 2020

YannickJadoul commented Jun 16, 2020 •

edited

Loading

Czaki commented Jun 18, 2020

YannickJadoul commented Jun 18, 2020 •

edited

Loading

YannickJadoul commented Jun 18, 2020

joerick left a comment

YannickJadoul commented Jun 22, 2020

joerick commented Jun 22, 2020

YannickJadoul commented Jun 22, 2020

Use pathlib.Path #376

Use pathlib.Path #376

Conversation

YannickJadoul commented Jun 15, 2020

YannickJadoul commented Jun 15, 2020

YannickJadoul commented Jun 16, 2020

joerick left a comment

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

Choose a reason for hiding this comment

Czaki left a comment

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

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

Choose a reason for hiding this comment

YannickJadoul commented Jun 16, 2020

YannickJadoul commented Jun 16, 2020 • edited Loading

Czaki commented Jun 18, 2020

YannickJadoul commented Jun 18, 2020 • edited Loading

YannickJadoul commented Jun 18, 2020

joerick left a comment

Choose a reason for hiding this comment

YannickJadoul commented Jun 22, 2020

joerick commented Jun 22, 2020

YannickJadoul commented Jun 22, 2020

YannickJadoul commented Jun 16, 2020 •

edited

Loading

YannickJadoul commented Jun 18, 2020 •

edited

Loading