From: | Dave Page <dpage(at)pgadmin(dot)org> |
---|---|
To: | Sandeep Thakkar <sandeep(dot)thakkar(at)enterprisedb(dot)com> |
Cc: | pgadmin-hackers <pgadmin-hackers(at)postgresql(dot)org>, Hamid Quddus <hamid(dot)quddus(at)enterprisedb(dot)com> |
Subject: | Re: Patch for pgAdmin4 RPM package |
Date: | 2016-05-09 13:05:37 |
Message-ID: | CA+OCxoxBDt82P75Q4Cy4fBkznkPBW3YJwPz+wFARbBBSoi3fgg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgadmin-hackers |
Hi
Initial eyeball review comments below...
On Fri, Apr 22, 2016 at 11:57 AM, Sandeep Thakkar <
sandeep(dot)thakkar(at)enterprisedb(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Dave Page | 2016-05-09 13:15:24 | Re: PATCH: pgAdmin4 debian installer |
Previous Message | Dave Page | 2016-05-09 12:43:35 | Re: PATCH: pgAdmin4 windows installer |