avoid false positive PermissionError in declare_dependency #14342
Conversation
bonzini
force-pushed
the
fix13584
branch
2 times, most recently
from
cf05ccc to
e6eb60b
Compare
| declaredepdir.mkdir() | ||
| try: | ||
| tmpdir.chmod(0o444) | ||
| self.init(testdir, extra_args=f'-Ddir={declaredepdir}') | ||
| finally: | ||
| tmpdir.chmod(0o755) | ||
|
|
There was a problem hiding this comment.
This is essentially testing a design bug in the python standard library.
There was a problem hiding this comment.
To be clear, the purpose of my comment here is that I'm not convinced we need another unittest in order to check that this case doesn't regress.
I'm not even convinced that we're fixing a bug here at all -- just avoiding a slow op.
There was a problem hiding this comment.
It's just an optimization... But since it was giving a PermissionError it was a buggy optimization which is why I added the test.
bonzini
added
the
pathlib
bonzini
force-pushed
the
fix13584
branch
2 times, most recently
from
12a037d to
09c9676
Compare
|
This PR cannot be backported (?) to the stable release as it has not been merged to master (!!) -- unsetting from the milestone... |
|
The interpretation of "should probably be merged to master before 1.7.1 is cut" is fairly obvious. And unsetting 1.7.1 without setting 1.8 kinda throws the baby out with the bathwater.... |
|
Are you saying this PR is a release blocker for 1.7.1? Because maintenance releases are literally constructed via a script that downloads the list of PRs associated with the milestone and then backporting those PRs. From my perspective when trying to cut a new maintenance release, the milestone is "just" a list of PRs that are ready to be backported (and in some cases, major regression fixes that need to be back ported with the highest urgency). |
Well, not me but @bruchar1. I wouldn't say it's a release blocker, but it's a candidate. I think it makes sense to have that, prior to running the script, whoever runs it looks at any open PRs in the milestones and either applies them to master or moves the PR to the next 1.x release. Since GitHub only allows a single milestone it removes the extra step of having to add the 1.x.y milestone after the PR is merged. But if you want to reserve that for urgent PRs/regressions that's understandable. |
|
It doesn't have to be a blocker. I tagged it because it was a simple bugfix, and I though it would be merged quickly. |
Path.is_dir() can raise a PermissionError if a parent does not have the executable permission set; plus the "in p.parents" tests are very expensive. Do not use Path at all. Signed-off-by: Paolo Bonzini <[email protected]>
|
NixOS/nix#13081 I wouldn't mind a backport! :) |
If a directory that is passed to declare_dependency is not accessible (a parent has the x permission cleared), this can result in an OSError:
This can cause false positives, for example, if the directory is under /var and root-owned. Do the is_dir() test last, once it's known that the directory is related to the source directory, to avoid the false positives.
Fixes: #13584