Re: Patch for pgAdmin4 RPM package - Mailing list pgadmin-hackers

From Dave Page
Subject Re: Patch for pgAdmin4 RPM package
Date
Msg-id CA+OCxoxBDt82P75Q4Cy4fBkznkPBW3YJwPz+wFARbBBSoi3fgg@mail.gmail.com
Whole thread Raw
In response to Patch for pgAdmin4 RPM package  (Sandeep Thakkar <sandeep.thakkar@enterprisedb.com>)
Responses Re: Patch for pgAdmin4 RPM package  (Sandeep Thakkar <sandeep.thakkar@enterprisedb.com>)
List pgadmin-hackers
Hi

Initial eyeball review comments below...

On Fri, Apr 22, 2016 at 11:57 AM, Sandeep Thakkar <sandeep.thakkar@enterprisedb.com> wrote:
Hi Team, Dave,

Attached herewith are two patches. 

pgadmin4-rpm.patch - This is the main patch that includes scripts, makefiles and spec to create RPMs for RHEL6/RHEL7/F-22/F-23/F-24. 


Can we keep the directory names in lower case?
 

It will create two RPMs i.e pgadmin4 and pgadmin4-web. The  pgadmin4 tpm depends on web and the web rpm depends on the python packages. I have commented the list of packages which are not available on some systems so that Devrim can build them.

The installation path for pgadmin4 is "/usr/pgadmin4-<major>.<minor>" and pgadmin4-web is the site-packages/pgadmin4-web

Shouldn't the -web package also have the major.minor version number in the path, to allow side-by-side installation? 

pgadmin4-server-ini.patch - This is the patch for runtime/Server.cpp. As said pgadmin4-web and runtime installation directories are different and that means web does not exists in parallel to runtime like in sources. 

I observed that the location of application settings was not defined in Server.cpp. As per QSettings doc, the default location on Unix is the $HOME/.config/<companyname>/<appname>.conf. Here, $HOME depends on the user that runs the application. So, I thought why not to define the application settings in application directory itself. RPM then knows where to define the ApplicationPath. I tested it and it worked fine with me. I haven't done this change for platform dependent. 

Doesn't that prevent non-root users from changing the settings? Or (if you widen the permissions on the ini file), allow one user to mis-configure the app for others? I think what is needed here is a search path change, much like you added for the Mac app bundle. 

Other thoughts:

- Please rename the README to README.txt

- The code to build the RPMs should be entirely confined to pkg/rpm. A Makefile target should be added to /Makefile to build/clean the targets (this mistake was made with the Mac package too, but was one of the original requirements).

Please resolve these issues and I'll take another look.

Thanks.

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

pgadmin-hackers by date:

Previous
From: Dave Page
Date:
Subject: Re: PATCH: pgAdmin4 windows installer
Next
From: Dave Page
Date:
Subject: Re: PATCH: pgAdmin4 debian installer