matrix-synapse: only include used optional dependencies in check #369859
Conversation
xinyangli
force-pushed
the
matrix-synapse
branch
from
d156dbb to
bc1bdf6
Compare
github-actions
bot
added
10.rebuild-darwin: 0
|
I think this is the wrong approach. AIUI this makes it so that everyone uses a customized package and therefore can't rely on the cached package. What should be done instead is most likely to override pyopenssl to fix pysaml2 for synapse. |
|
Oh, I agree that we should override pyopenssl to a previous version. Correct me if I'm wrong, but I think this will only require a rebuild for those who override extras in the wrapper, as the default wrapper will be cached by hydra? Will keep this open for a bit to see if anyone has other thoughts. |
|
I was considering going for removing just pysaml2 from the nativeCheckInputs diff --git a/pkgs/servers/matrix-synapse/default.nix b/pkgs/servers/matrix-synapse/default.nix
index a1f78d9a2..cfb8594f5 100644
--- a/pkgs/servers/matrix-synapse/default.nix
+++ b/pkgs/servers/matrix-synapse/default.nix
@@ -149,7 +149,8 @@ python3.pkgs.buildPythonApplication rec {
mock
parameterized
])
- ++ lib.flatten (lib.attrValues optional-dependencies);
+ # TODO: re-add pysaml2 when it stops being broken
+ ++ lib.flatten (lib.attrValues (optional-dependencies // { saml2 = [ ]; }));
doCheck = !stdenv.hostPlatform.isDarwin;
and then skipping that failing test… On the surface, downgrading pyopenssl seems wrong to me, holding security-sensitive libraries at an older version and such. But I really don't know what that lib does. |
I think most people should still get a cached package since From what I understood, pysaml2 has not only tests broken, after a quick look it seems as if the functionality would be degraded with current pyopenssl (correct me if I'm wrong), so skipping the tests is not an option. So my suggestion would be to
Does that make sense? |
|
Given that SAML support is probably a weird niche I think that approach is fine. |
xinyangli
force-pushed
the
matrix-synapse
branch
from
bc1bdf6 to
e12142a
Compare
|
Fixed according to comments. But this approach kind of assumes that no one uses |
xinyangli
force-pushed
the
matrix-synapse
branch
from
e12142a to
59e900c
Compare
|
Not sure why the ci failed? |
wegank
added
the
12.approvals: 1
|
Still get tests errors in matrix-synapse itself though: |
|
Fix #371815 |
Remove unnecessary optional dependencies from
nativeCheckInputs. This will make pysaml2 an optional dependencies (#367976). Unfortunately, this still doesn't fix build because of element-hq/synapse#17878, but it seems like this is more likely to be fixed sooner than IdentityPython/pysaml2#975Related: #367976 #369303
Ping maintainers: maybe @NickCao @fadenb
others: @jcaesar (Might be interested?)
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.