-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
Pgadmin update 6.8, fix build breakage on master #169370
Conversation
8fc0ae5
to
e47b264
Compare
Apparently pgadmin4 6.7 (odd versioning 🤔) built without these overrides over in #169434. |
Yes. It builds. But it doesn't start and the tests in nixos fail. Due to the nature of the tests, they cannot be included as tests in the main package.. |
in | ||
{ | ||
name = "pgadmin4"; | ||
meta.maintainers = with lib.maintainers; [ gador ]; | ||
|
||
nodes.machine = { pkgs, ... }: { | ||
imports = [ ./common/x11.nix ]; | ||
# needed because pgadmin 6.8 will fail, if those dependencies get updated | ||
nixpkgs.overlays = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overriding is already done in the derivation, so why do it here as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the comment below.
-
Pgadmin is not wrapped with test only python packages
-
Pgadmin (the app) is wrapped, but not the tests. To run the python tests, a python environment is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this into the package possible behind a flag which builds it with test dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already in the package in L74ff (pgadmin4
). The override in the python package set need to be in the package, as well as here in the test. (Or do you know any way to re-use the overrideScope
between derivations?)
Adding a test-flag to the pgadmin4 derivation is more work and more confusing than just adding the two test packages here in L71-L72 (nixos/tests/pgadmin4
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the overrideScope is required pgadmin should be not in pythonPackages and do them there. This will definitely bit rot over time and be forgotten when updating packages.
It also makes the test flawed and not well written because if it gets out of sync (which will definitely happen eventually) we are not testing what we are shipping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, if I understand you correctly, the nixpkgs.overlays
must be removed here from the test and the overrideScope
from the pgadmin.nix derivation must be imported here?
I really wrestled a long time with this (see #169370 (comment)) and this is the only way I got it to work. Any pointers of including the overrideScope
from the pgadmin.nix into this test environment would be greatly appreciated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, apparently I work better when its not late at night. I gave it a try to pass the python environment to the test from the derivation. Id appreciate a second (or third, forth...) look ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, if I understand you correctly, the
nixpkgs.overlays
must be removed here from the test and theoverrideScope
from the pgadmin.nix derivation must be imported here?
I took a second, deeper look and how it is right now not necessarily but if you would run the test standalone it wouldn't use the python env from pgadmin, so we still need to change it.
I think the easiest way would be to passthru the right things from pgadmin and use them in the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I think I did that.
The testcall to standalone is left as it is. The imported pgadmin package there is the one with the built-in python overrides.
The testcall to package is changed to pass the python environment.
This way we only have to change it in one place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SuperSandro2000
Do you have time for (yet) another look?
@FRidh could you take another look? Thanks 😄 |
622c3ee
to
09a39ca
Compare
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
Signed-off-by: Florian Brandes <[email protected]>
Signed-off-by: Florian Brandes <[email protected]>
Signed-off-by: Florian Brandes <[email protected]>
this commit passes the build dependencies to the pgadmin nixos test for package and regression testing. Also added changelog and some clarifying comments. Signed-off-by: Florian Brandes <[email protected]>
Signed-off-by: Florian Brandes <[email protected]>
09a39ca
to
0dd6926
Compare
Signed-off-by: Florian Brandes <[email protected]>
Signed-off-by: Florian Brandes <[email protected]>
e447df6
to
0c8e4f4
Compare
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/pgadmin4-fails-to-start/19004/4 |
Description of changes
Update to the new release.
This fixes a regression tracked in #169322.
Unfortunately, the recent update of
flask
andwerkzeug
cause a build failure.This makes the override necessary. Since pgadmin4 isn't imported anywhere else (a leaflet), it shouldn't cause inconsistent python environments.
I haven't figured out a way to nicely import the different pythonScope in the test suite for the regression testing and had to do with
overlays
.Thanks @FRidh for the pointer to
overrideScope
! (Maybe it is worthwhile adding that to the manual..) and to @teto for the initial bug report.Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)nixos/doc/manual/md-to-db.sh
to update generated release notes