Skip to content

BUG: preserve file permissions for sdist#102

Merged
FFY00 merged 1 commit intomesonbuild:mainfrom
rgommers:file-permissions
Jul 26, 2022
Merged

BUG: preserve file permissions for sdist#102
FFY00 merged 1 commit intomesonbuild:mainfrom
rgommers:file-permissions

Conversation

@rgommers
Copy link
Contributor

@rgommers rgommers commented Jul 9, 2022

Closes gh-82

Note that the bug report is only about the sdist. The wheel generation uses shutil.copy2, which should do the right thing already.

@rgommers
Copy link
Contributor Author

rgommers commented Jul 9, 2022

Ah fun to test - git does not preserve permissions aside from the executable bit. The checkout depends on umask (see https://stackoverflow.com/questions/22239508/git-pull-is-setting-664-instead-of-644-permissions), so we see 644 on Linux/macOS and 666 on Windows in CI.

For installing a wheel, only the executable bit matters, pip does not preserve the rest: https://github.com/pypa/pip/blob/d0051d3266ba11e7410a43a040dfd409af88a1db/src/pip/_internal/utils/unpacking.py#L105-L110

So the bug fix is fine here, but for a robust test it looks like we should only test the executable bit.

@rgommers
Copy link
Contributor Author

rgommers commented Jul 9, 2022

@FFY00 would you then prefer this to be a separate test, rather than included in test_contents_subdirs?

@FFY00
Copy link
Member

FFY00 commented Jul 9, 2022

Yep, I would, but this is acceptable too.

@h-vetinari
Copy link

so we see 644 on Linux/macOS and 666 on Windows in CI.

Permissions on windows don't follow the unix permission model anyway. You probably know this article, and it talks about much, much more than that, but I would just reflect this in the windows tests - or perhaps even better - just test for the main executable bit.

@FFY00 FFY00 force-pushed the file-permissions branch 4 times, most recently from 227e7c6 to e221af6 Compare July 26, 2022 13:53
Copy link
Member

@FFY00 FFY00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

I have added a test for the built wheel and skipped the executable bit tests on Windows.

@FFY00 FFY00 enabled auto-merge (rebase) July 26, 2022 13:54
@rgommers
Copy link
Contributor Author

thanks!

Closes mesonbuildgh-82

Signed-off-by: Filipe Laíns <lains@riseup.net>
@FFY00 FFY00 force-pushed the file-permissions branch from e221af6 to 44619a3 Compare July 26, 2022 19:06
@FFY00 FFY00 merged commit ed69cb1 into mesonbuild:main Jul 26, 2022
@rgommers rgommers deleted the file-permissions branch August 10, 2022 14:48
rgommers added a commit to rgommers/meson-python that referenced this pull request Sep 27, 2022
Note that Meson does have the correct install path in
`intro-install_plan.json`, containing `{bindir}`.
This gets translated into `script/` somewhere, at least for a
conda-installed Python on Arch Linux. This code was added in mesonbuildgh-102,
and it's not quite clear why this line was commented out.

For now, let's just make sure the test is not failing.
rgommers added a commit to rgommers/meson-python that referenced this pull request Sep 27, 2022
Note that Meson does have the correct install path in
`intro-install_plan.json`, containing `{bindir}`.
This gets translated into `script/` somewhere, at least for a
conda-installed Python on Arch Linux. This code was added in mesonbuildgh-102,
and it's not quite clear why this line was commented out.

For now, let's just make sure the test is not failing.
rgommers added a commit to rgommers/meson-python that referenced this pull request Sep 28, 2022
Note that Meson does have the correct install path in
`intro-install_plan.json`, containing `{bindir}`.
This gets translated into `script/` somewhere, at least for a
conda-installed Python on Arch Linux. This code was added in mesonbuildgh-102,
and it's not quite clear why this line was commented out.

For now, let's just make sure the test is not failing.

xref mesonbuildgh-115
rgommers added a commit that referenced this pull request Sep 28, 2022
Note that Meson does have the correct install path in
`intro-install_plan.json`, containing `{bindir}`.
This gets translated into `script/` somewhere, at least for a
conda-installed Python on Arch Linux. This code was added in gh-102,
and it's not quite clear why this line was commented out.

For now, let's just make sure the test is not failing.

xref gh-115

[ci skip]
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.

Wrong script permission for sdist

3 participants