Skip to content
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

cpython: don't use lchmod() on Linux, fix w/musl #39517

Merged
merged 1 commit into from Apr 26, 2018

Conversation

dtzWill
Copy link
Member

@dtzWill dtzWill commented Apr 26, 2018

upstream issue:
https://bugs.python.org/issue31940

There are two PR's proposed to fix this,
but both seem to be stalling waiting for review.

I previously used what appears to be the favored
of the two approaches[1] to fix this,
with plan of keeping it musl-only until PR was merged.

However, while writing up a commit message
explaining the problem and why it needed fixing...

I investigated a bit and found it increasingly
hard to justify anything other than ...
simply not using lchmod.

Here's what I found:

  • lchmod is non-POSIX, seems BSD-only these days
  • Functionality of lchmod isn't supported on Linux
    • best scenario on Linux would be an error
  • POSIX does provide lchmod-esque functionality
    with fchmodat(), which AFAICT is generally preferred.
  • Python intentionally overlooks fchmodat()[2]
    electing instead to use lchmod() behavior
    as a proxy for whether fchmodat() "works".
    I'm not sure I follow their reasoning...
  • both glibc and musl provide lchmod impls:
    • glibc returns ENOSYS "not implemented"
    • musl implements lchmod with fchmodat(),
      and so returns EOPNOTSUPP "op not supported"
  • Python doesn't expect EOPNOTSUPP from lchmod,
    since it's not valid on BSD's lchmod.
  • "configure" doesn't actually check lchmod usefully,
    instead checks for glibc preprocessor defines
    to indicate if the function is just a stub[3];
    somewhat fittingly, if the magic macros are defined
    then the next line of the C source is "choke me",
    causing the compiler to trip, fall, and point
    a finger at whatever is near where it ends up.
    (somewhat amusing, but AFAIK effective way to get an error :P)

I'm leaving out links to threads on mailing lists and such,
but for now I hope I've convinced you
(or to those reading commit history: explained my reasons)
that this is a bit of a mess[4].

And so instead of making a big mess messier,
and with hopes of never thinking about this again,
I propose we simply tell Python "don't use lchmod" on Linux.

[1] python/cpython#4783
[2] https://github.com/python/cpython/blob/28453feaa8d88bbcbf6d834b1d5ca396d17265f2/Lib/os.py#L144
[3] https://github.com/python/cpython/blob/28453feaa8d88bbcbf6d834b1d5ca396d17265f2/configure#L2198
[4] Messes happen, no good intention goes unpunished :).

(I've tested on a slightly different branch, let's see what borg says)

  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

upstream issue:
https://bugs.python.org/issue31940

There are two PR's proposed to fix this,
but both seem to be stalling waiting for review.

I previously used what appears to be the favored
of the two approaches[1] to fix this,
with plan of keeping it musl-only until PR was merged.

However, while writing up a commit message
explaining the problem and why it needed fixing...

I investigated a bit and found it increasingly
hard to justify anything other than ...
simply not using lchmod.

Here's what I found:
* lchmod is non-POSIX, seems BSD-only these days
* Functionality of lchmod isn't supported on Linux
  * best scenario on Linux would be an error
* POSIX does provide lchmod-esque functionality
  with fchmodat(), which AFAICT is generally preferred.
* Python intentionally overlooks fchmodat()[2]
  electing instead to use lchmod() behavior
  as a proxy for whether fchmodat() "works".
  I'm not sure I follow their reasoning...
* both glibc and musl provide lchmod impls:
  * glibc returns ENOSYS "not implemented"
  * musl implements lchmod with fchmodat(),
    and so returns EOPNOTSUPP "op not supported"
* Python doesn't expect EOPNOTSUPP from lchmod,
  since it's not valid on BSD's lchmod.
* "configure" doesn't actually check lchmod usefully,
  instead checks for glibc preprocessor defines
  to indicate if the function is just a stub[3];
  somewhat fittingly, if the magic macros are defined
  then the next line of the C source is "choke me",
  causing the compiler to trip, fall, and point
  a finger at whatever is near where it ends up.
  (somewhat amusing, but AFAIK effective way to get an error :P)

I'm leaving out links to threads on mailing lists and such,
but for now I hope I've convinced you
(or to those reading commit history: explained my reasons)
that this is a bit of a mess[4].

And so instead of making a big mess messier,
and with hopes of never thinking about this again,
I propose we simply tell Python "don't use lchmod" on Linux.

[1] python/cpython#4783
[2] https://github.com/python/cpython/blob/28453feaa8d88bbcbf6d834b1d5ca396d17265f2/Lib/os.py#L144
[3] https://github.com/python/cpython/blob/28453feaa8d88bbcbf6d834b1d5ca396d17265f2/configure#L2198
[4] Messes happen, no good intention goes unpunished :).
@dtzWill
Copy link
Member Author

dtzWill commented Apr 26, 2018

This really shouldn't break anything (the configure check was already "no" with glibc, it just used special checks to find that out) so I'm optimistically going ahead and merging.

Post-merge review / thoughts always appreciated, though :).

@dtzWill dtzWill merged commit bdf390f into NixOS:staging Apr 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants