python3Packages.pytest-xdist: Treat --forked problems on darwin

[mweinelt]

We run pytest with --forked in nixpkgs, to reduce side effects that
can occur when multiple tests mutate their environment in incompatible
ways.

Forking on macOS 10.13 and later is unsafe when an application does work
between calls to fork() and its followup exec(). This may lead to
crashes when calls into the Objective-C runtime are issued, which will
in turn coredump the Python interpreter.

One good reproducer for this scenario is when the urllib module tries
to lookup proxy configurations in urllib.request.getproxies() through
get_proxies_macos_sysconf into the native _scproxy module.

This is a class of issues that is of course not limited to the urllib
module. The general recommendation is to use spawn instead of fork,
but we don't have any influence on upstream developers to do one or the
other.

One often cited workaround would be to disable fork safety entirely on
calls to initialize(), which is probably a better solution than
running without multithreading (slow) or without the --forked (prone
to side effects) mode.

This currently happens on aarch64-linux only, where we use more recent
11.0 SDK version, while x86_64-darwin has been stuck on 10.12 for a
while now.

python/cpython#77906 (comment)
http://www.sealiesoftware.com/blog/archive/2017/6/5/Objective-C_and_fork_in_macOS_1013.html

Closes: #194290

Description of changes

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 22.11 Release Notes (or backporting 22.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
  • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
• Fits CONTRIBUTING.md.

[tjni]

I'm seeing a test failure in python310Packages.build (test_main#test_venv_fail) that looks related to colors being present when they shouldn't be.

[tjni]

I guess the order that the tests run is different on my machine, and test_main.py#test_venv_fail cannot run after another test that enables colors. If I apply pypa/build#519, my error goes away.

It sort of looks to me like running the tests in test_main.py in parallel within the same process is not safe because they depend on some static initialization in a module -- so tests can interleave and interfere with each other, but that might not happen enough in practice to matter.

[mweinelt]

Ugh, precisely why I don't want to disable --forked in general.

I wonder if we would be better of just disabling fork safety. It's safety after all, not security. And only during tests.

[tjni]

After working around the build issue, I can verify that poetry is fixed for me.

I don't have a strong opinion on whether to disable fork safety or not. As a practical matter, I have never encountered this issue, though I read on the web that some people do. I'd be fine with that option too (e.g. for poetry).

In a vacuum, I would prefer to leave the option of forking to packages, but if forking solves a lot of problems that package managers / maintainers don't address, it makes sense to add it by default.

[mweinelt]

We did see this at work using ansible and could trace it back to urllib requesting proxy settings on MacOS, so that would be pretty widespread.

I think it is a fair to consider disabling fork safety, given that we only use it during tests. I pushed an update to make that happen.

I welcome further input on the matter.

[tjni]

I just tested this, and it turns out that the case of "YES" matters. It needs to be:

export OBJC_DISABLE_INITIALIZE_FORK_SAFETY=YES

and then poetry builds again. I'm personally fine with this approach, but I also remembered #193254 (comment) earlier that advocates for using no_proxy = '*', which is a narrower solution which could resolve the poetry case as well. So I submit it for your consideration.

[mweinelt]

If this primarily happens when urllib.request.getproxies() gets called this will probably crash at runtime as well. And then I wonder if it is fine to leave the user with the decision how to handle that.

Disabling proxying during tests is fine, but it is likely only one category of failing tests, and it is somewhat likely that multiple different packages are currently failing due to similar reasons on aarch64-darwin.

I mean … we could take no_proxy and see how far that takes us. It is probably easier to justify.

[mweinelt]

I think we should give OBJC_DISABLE_INITIALIZE_FORK_SAFETY=YES a spin and see where it takes us.