python3Packages.angr: don't override unicorn

[dotlambda]

Motivation for this change

That's not allowed for pythonPackages.

Things done

• Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
• 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 nixpkgs-review --run "nixpkgs-review wip"
• Tested execution of all binary files (usually in ./result/bin/)
• Determined the impact on package closure size (by running nix path-info -S before and after)
• Ensured that relevant documentation is up to date
• Fits CONTRIBUTING.md.

[cole-h]

Is this codified anywhere? Maybe I wasn't looking thoroughly enough, but I had no idea that overrides were unacceptable (given how widely-used they are everywhere else in Nixpkgs).

If it isn't, maybe it should be?

[SuperSandro2000]

That is allowed for pythonPackages, done at multiple other places (https://search.nix.gsc.io/?q=overridePythonAttrs&i=nope&files=pkgs%2Fdevelopment%2Fpython-modules&repos=) and necessary for bigger ones like home-assistant. If a package version is required for multiple other packages it should be moved to the top level python file instead of being overwritten multiple times.

[dotlambda]

Almost all of those packages should not do so. The reason is that you can't have two versions of the same Python package in $PYTHONPATH.
It's fine for e.g. pytest since that's not in propagatedBuildInputs.
home-assistant is not in pythonPackages, so there's no problem.
We make a few exceptions for big packages like Django but such overrides should be subject to discussion before introducing them.
@FRidh @jonringer We should add this to the manual.
@SuperSandro2000 You prematurely merge or close pull requests time and time again. There are many people who take more care reviewing contributions and learning about the Nix ecosystem. You'd be well-advised not to overestimate your own abilities and to sometimes seek the expertise of others.

[SuperSandro2000]

The reason is that you can't have two versions of the same Python package in $PYTHONPATH.

That would mean we would need to remove any package with multiple versions in https://github.com/NixOS/nixpkgs/blob/master/pkgs/top-level/python-packages.nix. That would include arrow_1, dnspython_1, foundationdb, wxPython_4_0 and more.

And that information would also belong into the pull request message.

Also if the information isn't written down there is a chance that I don't know it.

Also the PR with this change was open like that since two weeks and no one else took a look at it which knew that information.

It's fine for e.g. pytest since that's not in propagatedBuildInputs.

This also needs to be written down especially for pytest-* packages.

learning about the Nix ecosystem.

Thats quite a bold statement. I am using nix since November 2020 (!) with no experience in functional languages.

[dotlambda]

learning about the Nix ecosystem.

Thats quite a bold statement. I am using nix since November 2020 (!) with no experience in functional languages.

Sorry for that. I should have said "had more time to learn about the Nix ecosystem". I think we're all glad about active contributors like you.

[jonringer]

Here's my quick reply on the matter:

Pinning within python-modules is highly discouraged. This will potentially introduce incompatible versions in a user's environment as either the new pinned version or the original version will be imported at runtime, breaking one of the packages.

The preference to handling this is to relax version bounds in the "install_requires" field. (could be in setup.py, pyproject.toml, requirements or others). In most cases, packages are still compatible with small API changes which may warrant a major version bump. We use test suites to verify that the package still works correctly.

If the package is still incompatible with the latest major version, then the most proper way to handle this is make an issue with the upstream package to adopt the latest major version. Or if upstream is not very responsive, you are free patch the source to make it compatible.

In very few circumstances, two versions of the same package are allowed to exist if the packages are extremely difficult to package. Some examples of this are tensorflow, which has huge ecosystems built around it and is hard to package. Another is django, which has 2 actively developed versions, and large ecosystems built around each.

One exception to this is applications, due to the way buildPythonApplication and toPythonApplication functions work, the related derivations will not bleed dependencies between packages. If the package doesn't need to be imported by other python modules, then this package would be a good candidate to convert into application. You can look at https://github.com/NixOS/nixpkgs/blob/master/pkgs/tools/admin/awscli/default.nix as an example of using an overlay within a python application.

Info on buildPythonApplication can be found here.
Info on toPythonApplication can be found here.

[jonringer]

but @dotlambda is correct, the issue is that if angr and another package are used in the same environment, one of them will be broken. So we should only ever use a single version of a dependency. buildPythonApplication gives you some more flexibility, but then you're limited to just using the commands from the package; which is still useful if that's the intended purposes (e.g. glances)

[dotlambda]

@jonringer So is there any objection to merging this?

[fabaff]

We all agree that overriding is a last resort. Nobody wants it and everybody tries to avoid it but it helps to keep things functional. In a perfect world no pinning would be needed.

The requirement was relaxed but it was not enough. Thus, the overriding was introduced. The overriding of unicorn allows angr to work properly ( 1.0.2-rc4 is a release that is confirmed to work at the moment). I'm wondering how the "Overriding Python packages" section fits in but from my point of view is the use case here a match.

I guess that nix-shell will detect a possible collisions if another unicorn release is pulled-in. Sure, there are other ways to get two different releases of a Python package into your environment like you could do in a classical venv.

So is there any objection to merging this?

Removing the override breaks angr in a non-transparent way. This should be merged when the fix arrives.

[dotlambda]

The requirement was relaxed but it was not enough. Thus, the overriding was introduced. The overriding of unicorn allows angr to work properly ( 1.0.2-rc4 is a release that is confirmed to work at the moment).

I would argue that anyone willing to use angr should do the override themselves. It should not be part of nixpkgs.
If we allow exceptions now, the number of overrides within pythonPackages will just keep increasing, breaking the entire package set and making it unmaintainable.

[SuperSandro2000]

I would argue that anyone willing to use angr should do the override themselves. It should not be part of nixpkgs.
If we allow exceptions now, the number of overrides within pythonPackages will just keep increasing, breaking the entire package set and making it unmaintainable.

... which will cause the downfall of nixpkgs, linux and the entirety of unix.

Jokes aside. A package shouldn't require an overlay to be in a basic functional state. If it does we either need to fix it or it can be remove entirely. Collecting half broken packages that are not working except if you add this or that overlay just collects dust.

There are already 5 or so packages who do this exact thing and python packages is totally fine. As long as we are not doing it with core packages it will most likely not cause any harm. Also nix will detect if we have two different versions of the same library and it will be pretty easy to find the RC in the tree.

We just need to be careful to not introduce it except if it is totally necessary and keep an eye out for this while reviewing because people will always come up with creative ways to get around nix mechanics.

[stale]

I marked this as stale due to inactivity. → More info

[dotlambda]

I will fix the merge conflict and merge this in a few hours. This has been left unfixed for too long.
cc @FRidh

[fabaff]

1.0.2rc4 is still the required version of unicorn.

[dotlambda]

@fabaff Not anymore: angr/angr@6e9e6a0

[fabaff]

@dotlambda, thanks for the hint. Then it can be removed with 9.2.26.