Skip to content

Conversation

@jorikdima
Copy link
Contributor

@jorikdima jorikdima commented Mar 25, 2020

Summary of changes

This PR is supposed to fix a regression caused by PR #1607 and initially explained in the issue #1424
Originally the issue #1424 was fixed by not preserving flags for package data, but this caused issues in many other projects described e.g. in #2043
The new approach is that we again keep file flags as is, but set write permission flag for the target file afterwards. This should not clear eXecutable bit, but allow file deleting.
An updated test also sets eX bit for package file and checks that target file still has it. The test fails for current version of setuptools and passes for the proposed PR.
Closes #2041

Pull Request Checklist

  • Changes have tests
  • News fragment added in changelog.d. See documentation for details

@leekillough
Copy link

LGTM

@jorikdima
Copy link
Contributor Author

One comment about tests. Currently I updated read_only test which have been prepared by @jaraco . That script tests the initial solution. But it may work (really test the solution) only on Windows platform, since on Linux there is no such issue with read only files at all. My add on checks eX flags, which do not exist on Windows but on Linux only. My test needs to be checked on Windows, I don't have access to Win platform to run tox.
I think the best solution (I am talking about tests only) would be to split the test between testing read only flags and eX flags (two test functions). One test should be run only on Win, the other one only on Linux.
If that makes sense, let me know, please. I am too inexperienced in setuptools to make such decisions.

@jaraco
Copy link
Member

jaraco commented Mar 25, 2020

Thanks for putting this together.

One test should be run only on Win, the other one only on Linux.

I do aim to test behavior as uniformally as possible. So for example, the previous test would run on both Windows and Linux, and even though it only failed on Windows, it still asserted the expectation on all platforms uniformally.

In some cases, it's not possible to assert a behavior across all platforms, and in that case, it's okay to either skip or xfail a test on that platform (skip if it has no value, xfail if monitoring the failed behavior is more appropriate).

I can see the tests are failing on Windows, so I can take it from here. Thanks @jorikdima.

@jaraco jaraco mentioned this pull request Mar 25, 2020
@jaraco jaraco closed this Mar 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Packaged script execute permissions lost from v46.1.0 onwards

3 participants