python3Packages.{mautrix,matrix-nio}: add withOlm flags
#336901
Conversation
emilazy
force-pushed
the
push-sqwspuzylwxs
branch
from
16ba9f6
to
15da29b
Compare
emilazy
changed the title
withOlm flags
emilazy
force-pushed
the
push-sqwspuzylwxs
branch
from
15da29b
to
89d4919
Compare
emilazy
force-pushed
the
push-sqwspuzylwxs
branch
from
89d4919
to
045cc0c
Compare
|
Okay, I made this more normal and just added Once this is merged and backported I’ll go ahead with #335189, as the fallout so far seems to have been minimal except for the mistake with Result of 19 packages built:
Result of 36 packages built:
|
|
It's a Python library, so is it really that much of an effort to allow-list it in one's nixpkgs config and build it yourself? Is the use-case to just build nio without the whitelisting? I don't have a strong opinion on that as long as we don't ship any packages like mautrix-* w/o encryption by default, just trying to understand what we want to achieve :)
cc @mweinelt |
| ++ lib.optionals withOlm [ | ||
| python-olm | ||
| unpaddedbase64 | ||
| pycryptodome |
There was a problem hiding this comment.
We can turn this into a real extra
There was a problem hiding this comment.
No, I don’t think we can. That’s how it was before, but then we can’t see within the package whether it’s enabled, so we can’t conditionalize the tests on it, so it defeats the whole point of this change.
There was a problem hiding this comment.
Or is the idea that the optional-dependencies would still be there but the withOlm flag would gate the tests? That would mean that downstream consumers could forget to enable it and be using untested functionality, but I guess there’s unlikely to be new ones added.
| @@ -21,7 +21,7 @@ python3.pkgs.buildPythonPackage rec { | |||
| aiohttp | |||
| asyncpg | |||
| commonmark | |||
| mautrix | |||
| (mautrix.override { withOlm = true; }) | |||
There was a problem hiding this comment.
and then use mautrix.optional-dependencies.encryption here
| ++ lib.optionals withOlm [ | ||
| atomicwrites | ||
| cachetools | ||
| python-olm | ||
| peewee | ||
| ]; |
There was a problem hiding this comment.
| ++ lib.optionals withOlm [ | |
| atomicwrites | |
| cachetools | |
| python-olm | |
| peewee | |
| ]; | |
| lib.optionals withOlm optional-dependencies.e2 |
We want to do that and it should now be catched by our hooks in case we mix things. Also test dependencies don't mix with the normal ones so that should be even less of an issue. |
I’m just trying to fix #336052 and the like. You shouldn’t get a security warning for the packages that already don’t use the E2E functionality and aren’t going to exercise anything from libolm. |
|
The optional-dependencies definition must stay. |
emilazy
force-pushed
the
push-sqwspuzylwxs
branch
from
045cc0c
to
343ad0e
Compare
|
I’ve pushed a change that restores the |
|
Bumping this as it’s the main remaining blocker to the 24.05 libolm backport. |
There was a problem hiding this comment.
You shouldn’t get a security warning for the packages that already don’t use the E2E functionality and aren’t going to exercise anything from libolm
Fair.
My main objectives still seem to be kept (no downgrade of defaults for bridges, no silence of security warnings when using said bridges with libolm).
There are two things we should probably address before merging.
|
I also don’t particularly like it :) But I can’t think of a better compromise and I don’t want more packages than necessary to be marked insecure by this, so merging. |
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin release-24.05
git worktree add -d .worktree/backport-336901-to-release-24.05 origin/release-24.05
cd .worktree/backport-336901-to-release-24.05
git switch --create backport-336901-to-release-24.05
git cherry-pick -x 1b07c5eb2fb21d56ab17ab33dec62101156ce799 90b75a1f393735769f0e9a2d232c02b5dc9115bc 343ad0ee4a1342ba18edcf67df4d9d46104d13dd |
Description of changes
An alternate approach to #336694. This has some nice properties: we don’t have to fish into dependency trees or fiddle around with hacks like overriding
knownVulnerabilities, Hydra never builds libolm, but all available functionality of the library can be tested. The disabled tests and patching are what we’d have to do if we went the route of never testing Olm functionality anyway.On the other hand, the definition ofI did this.olmAllowedis kind of gross? It might be better if these packages took a configuration argument directly asking whether they’re going to be used with E2E, and set their dependencies and tests accordingly. Not sure. Putting it up here for review.cc @AndrewKvalheim
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.