-
Notifications
You must be signed in to change notification settings - Fork 68
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
PERF: remove isdir() check in copy_file(), about 10% of the run time was checking if the file to copy is a directory. #258
base: main
Are you sure you want to change the base?
Changes from all commits
7a6f42b
a996148
b164d66
89522f9
c6b23d0
e5268b5
6c1cb08
851e71a
7dcde5e
041c42e
b07b4ed
ef8f235
03f1d85
e85efee
976e935
d6652a4
806b1ca
e5b06e1
4549de1
294b206
a37185d
745640e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -70,9 +70,8 @@ def copy_file( # noqa: C901 | |
verbose=1, | ||
dry_run=0, | ||
): | ||
"""Copy a file 'src' to 'dst'. If 'dst' is a directory, then 'src' is | ||
copied there with the same name; otherwise, it must be a filename. (If | ||
the file exists, it will be ruthlessly clobbered.) If 'preserve_mode' | ||
"""Copy a file 'src' to 'dst'. | ||
(If the file exists, it will be ruthlessly clobbered.) If 'preserve_mode' | ||
Comment on lines
+73
to
+74
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we can assume that distutils is the only consumer of this function. Maybe no one else is using it, but we can't simply remove it without risking breaking users. Let's do this instead:
What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for 3. the logic is broken as far as I can tell. we can't keep the syscall to I think we can't have a replacement function be private with a underscore. It's a public API that is used in other places. it think linters will complain if we import a private _ function in other places (note that setuptools is vendoring this repo and might have more CI rules). searching on github what I can think of?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've started the refactoring... but unfortunately found out realistically, Is there really a risk in fixing copy_file() to break other packages? packages that are not already broken because distutils is removed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hyrum's law usually hits hard on distutils/setuptools... If you want to define 2 other new functions (one for files or one for directories) it should be fine. But my opinion is that we should keep the old behaviour (maybe behind a deprecated warning) for a couple of years before dropping it (if ever dropping would be an option... we have to remember that some packages in the ecosystem, haven't even moved to setuptools yet...) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, that sounds messy. Let's consider some other options. |
||
is true (the default), the file's mode (type and permission bits, or | ||
whatever is analogous on the current platform) is copied. If | ||
'preserve_times' is true (the default), the last-modified and | ||
|
@@ -109,12 +108,6 @@ def copy_file( # noqa: C901 | |
"can't copy '%s': doesn't exist or not a regular file" % src | ||
) | ||
|
||
if os.path.isdir(dst): | ||
dir = dst | ||
dst = os.path.join(dst, os.path.basename(src)) | ||
else: | ||
dir = os.path.dirname(dst) | ||
|
||
if update and not newer(src, dst): | ||
if verbose >= 1: | ||
log.debug("not copying %s (output up-to-date)", src) | ||
|
@@ -126,10 +119,7 @@ def copy_file( # noqa: C901 | |
raise ValueError("invalid value '%s' for 'link' argument" % link) | ||
|
||
if verbose >= 1: | ||
if os.path.basename(dst) == os.path.basename(src): | ||
log.info("%s %s -> %s", action, src, dir) | ||
else: | ||
log.info("%s %s -> %s", action, src, dst) | ||
log.info("%s %s -> %s", action, src, dst) | ||
|
||
if dry_run: | ||
return (dst, 1) | ||
|
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm uneasy with this pattern being now duplicated 5 times across the codebase. I'd rather see something like:
And then do some refactoring to make
copy_file_to_dir
perform the dest operation.