Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1b6HIa-0008CV-Nd for pgadmin-hackers@arkaria.postgresql.org; Fri, 27 May 2016 12:55:32 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84_2) (envelope-from ) id 1b6HIZ-00056x-UK for pgadmin-hackers@arkaria.postgresql.org; Fri, 27 May 2016 12:55:31 +0000 Received: from makus.postgresql.org ([2001:4800:1501:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.84_2) (envelope-from ) id 1b6HIZ-00056r-8D for pgadmin-hackers@postgresql.org; Fri, 27 May 2016 12:55:31 +0000 Received: from mail-qk0-x22a.google.com ([2607:f8b0:400d:c09::22a]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1b6HIV-0007l8-RE for pgadmin-hackers@postgresql.org; Fri, 27 May 2016 12:55:29 +0000 Received: by mail-qk0-x22a.google.com with SMTP id y126so78513937qke.1 for ; Fri, 27 May 2016 05:55:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc; bh=MFkEGcNeBZczUakzbFYYKEtxmCInNeAvqNpoa85qN2E=; b=UCP5Tpo0WMx/WzIoZM59GcMS+rOAhWbjFsU+jkDwqPDELR+BxDxDizHcSTdosMI7Jq Q+vGU3bLubrX9AOTWhdmtqyMiEfIYSdZQrA3XbZxqnZToski5W5+zDYt6CNXAIwDEiXY MqhxtDwYe/RLrIaVNFPiZ4CRzvMDn/hOUXVJS3Yvm3clpHNy8lQRsi56smdt2GkybRMm fgbZRZl20dWeHsLcscEkA2xG8pPdRbO3GNcrhO+r6TQzwpW4BxqS16baRFTjbxqgx+OZ VV4WKEXOdW4Ske8d/aak55E9agDogPDFJcLgLjp2fX81EtrIyBjDddvxvb8cAjBIcWDx WREg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc; bh=MFkEGcNeBZczUakzbFYYKEtxmCInNeAvqNpoa85qN2E=; b=E3Q4O97b9Wd5jrADSwQytxAF790A99vR6ggqO+tzVTY8ab56c3F5N+Do6Q0KqcIXdc nsADaAfCAxPzOvNMxP/lRl4JSsk8G8WXJpDMG9pCh24fRr/G7Dzk4WKCLF4IfIIqB33H oxig1Qc64isdAgrU+6PnbLwGqEDumO7v0DP52pq7xWBtUGSkI2q0DK7Tvk931Dfr0nUp sqrq6WYMZrIh8SbNqmbTLE7LOjYDhitAJGkgIesLLt+vvl5sUPw9+CmedI54CmaTrTWb UCpmxDkoqxGrZ+7U0YDXCWTIn+YaRuuoEYqZUzEpbJlz+3tVYMZ5zaMqBUw4zrGxJNGy 0BUw== X-Gm-Message-State: ALyK8tK9m9cinef8BqJkhVuMr4QMk6gT7sWvpREv25pz+4sJbDC63YYnO8ADTcSCi+opvtn9s5zYfKsvvJlVpaHO MIME-Version: 1.0 X-Received: by 10.200.47.6 with SMTP id j6mr13538047qta.85.1464353726923; Fri, 27 May 2016 05:55:26 -0700 (PDT) Received: by 10.140.101.198 with HTTP; Fri, 27 May 2016 05:55:26 -0700 (PDT) In-Reply-To: References: Date: Fri, 27 May 2016 18:25:26 +0530 Message-ID: Subject: Re: Patch for pgAdmin4 RPM package From: Sandeep Thakkar To: Dave Page Cc: pgadmin-hackers , Hamid Quddus Content-Type: multipart/alternative; boundary=001a1136f16cb81d4a0533d26b3d X-Pg-Spam-Score: -2.6 (--) List-Archive: List-Help: List-ID: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: X-Mailing-List: pgadmin-hackers Precedence: bulk Sender: pgadmin-hackers-owner@postgresql.org --001a1136f16cb81d4a0533d26b3d Content-Type: text/plain; charset=UTF-8 On Mon, May 9, 2016 at 6:35 PM, Dave Page wrote: > 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? > > Sure. Will do that. > 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-." 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? > Right. Now that we don't have major/minor, so, will it be /usr/pgadmin4-v1 and pgadmin4-web-v1 ? Or? > > *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//.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. > > Right. Will use python command to find the site-packages path and then concatenate pgadmin4-web directory name. > 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. > > Sure. Will share it with you soon. > Thanks. > > -- > Dave Page > Blog: http://pgsnake.blogspot.com > Twitter: @pgsnake > > EnterpriseDB UK: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > -- Sandeep Thakkar --001a1136f16cb81d4a0533d26b3d Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


On Mon, May 9, 2016 at 6:35 PM, Dave Page <dpage@pgadmin.org> wrote:
Hi

Initial eyeball review comments be= low...

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

Attached he= rewith are two patches.=C2=A0

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


Can we= keep the directory names in lower case?
=C2=A0
Sure. Will do that.=C2=A0

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

The installation p= ath for pgadmin4 is "/usr/pgadmin4-<major>.<minor>" a= nd pgadmin4-web is the site-packages/pgadmin4-web

Shouldn't the -web package also have the major.mi= nor version number in the path, to allow side-by-side installation?
<= /div>
Right. Now that we don't have major/= minor, so, will it be=C2=A0/usr/pgadmin4-v= 1 and pgadmin4-web-v1 ? Or?=C2=A0
=C2=A0

pgadmin4-server-ini.patch - This is the patch for runtime/S= erver.cpp. As said pgadmin4-web and runtime installation directories are di= fferent and that means web does not exists in parallel to runtime like in s= ources.=C2=A0

I observed that the location of applicatio= n 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 thou= ght why not to define the application settings in application directory its= elf. 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= .=C2=A0

Doesn't that pre= vent non-root users from changing the settings? Or (if you widen the permis= sions 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 f= or the Mac app bundle.=C2=A0

Right. Will use python command to find the site-packages path and= then concatenate pgadmin4-web directory name.=C2=A0
=
Other thoughts:

- Please rename the RE= ADME to README.txt

- The code to build the RPMs sh= ould 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 pa= ckage too, but was one of the original requirements).

<= div>Please resolve these issues and I'll take another look.
<= br>
Sure. Will share it with you s= oon.
=C2=A0
Thanks.
=

--
Dave Page
Blog:= http://pgsnake.b= logspot.com
Twitter: @pgsnake

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



--
Sandeep Thakk= ar

--001a1136f16cb81d4a0533d26b3d--