Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1azksZ-0004fw-Vw for pgadmin-hackers@arkaria.postgresql.org; Mon, 09 May 2016 13:05:44 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84_2) (envelope-from ) id 1azksZ-0002gq-Iy for pgadmin-hackers@arkaria.postgresql.org; Mon, 09 May 2016 13:05:43 +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 1azksZ-0002gk-5b for pgadmin-hackers@postgresql.org; Mon, 09 May 2016 13:05:43 +0000 Received: from mail-ig0-x235.google.com ([2607:f8b0:4001:c05::235]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1azksV-0000dx-28 for pgadmin-hackers@postgresql.org; Mon, 09 May 2016 13:05:42 +0000 Received: by mail-ig0-x235.google.com with SMTP id bi2so103340252igb.0 for ; Mon, 09 May 2016 06:05:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pgadmin-org.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc; bh=DgXYDcOQ55DdD3vCgA8f0NaMu+A5YJxfnjDgfvjcj0o=; b=anP6qAedYlMh7GMJ1qQ/6n6FJKgUSMKFNSSn5Yyw71GWHd06Ym23yDYeqvQsPQ/1R8 /OEYHGQdj9q4ecBSWYtzeqINMbm1WyUkRoQQP05LsB4bjdLEkmTjp/R/uFR92qRFpYNW olhGRmydSIcPLaYHa8UuP3m0/WIAUJZgEOpKmngm8udWgaJC4oocozXM++15gfSWemWe h1UyaUjtStYI2ufYH1jm1NWYUOCLp4m3qYqVc1Jl/Mzdo+yswQ+yihQv/yhCmT5UV8RP h3zafUxvTu5JLcqDTu+zDD9+jHg9jprXGADsfuNiionkgqQ41dAfsh0Q782xf8IGF+eE edkg== 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=DgXYDcOQ55DdD3vCgA8f0NaMu+A5YJxfnjDgfvjcj0o=; b=J7Mr/0vCdB2ZHfW4WApuw+afU9YaBvCNc1qdIW5ObbbFNRr4mcZplb/VY5uxbJtUDo 6RhX0xp0aOG0Et5FxPIdhYu077H5xFV//w7RFCeYAf2uk+bvz/vu/yReORELdBUqBEgc J7aQ2te5KalMQauSaCtpjwzhgDOfD1z+X0weFLVrMgVaC8jYA9qEaFIk43ZCRNAlf/sZ ZAIx0k3QAA+P9JDib2g1WZdiQhobnV+wBjblec5Mzlb6TRiNMsesifpzVkKSezLNMtQz c80LB9yrvLyuNkohpQN23o3YZXjXkDpAypYRiEbbaC+yYK3ixJZYOjC7gZrL+3dxwGyU qJKg== X-Gm-Message-State: AOPr4FX79ck18HW2T8BebzXkeeToi4MDUNDl3uxw2v/ogpPNsdwNgIDdHIMvWc1Vvp9GEX6E7wH4ZxoWZeRVIQ== MIME-Version: 1.0 X-Received: by 10.50.110.35 with SMTP id hx3mr11169885igb.69.1462799137964; Mon, 09 May 2016 06:05:37 -0700 (PDT) Received: by 10.64.105.131 with HTTP; Mon, 9 May 2016 06:05:37 -0700 (PDT) In-Reply-To: References: Date: Mon, 9 May 2016 14:05:37 +0100 Message-ID: Subject: Re: Patch for pgAdmin4 RPM package From: Dave Page To: Sandeep Thakkar Cc: pgadmin-hackers , Hamid Quddus Content-Type: multipart/alternative; boundary=089e0129448aff332f0532687678 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 --089e0129448aff332f0532687678 Content-Type: text/plain; charset=UTF-8 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-." 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//.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 --089e0129448aff332f0532687678 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Hi

Initial eyeball review comments below...

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

=
Attached herewith 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 t= he directory names in lower case?
=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 packages. I have comme= nted the list of packages which are not available on some systems so that D= evrim can build them.

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

S= houldn't the -web package also have the major.minor version number in t= he path, to allow side-by-side installation?=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 prevent no= n-root users from changing the settings? Or (if you widen the permissions o= n the ini file), allow one user to mis-configure the app for others? I thin= k what is needed here is a search path change, much like you added for the = Mac app bundle.=C2=A0

Other thoughts:
- Please rename the README to README.txt

<= div>- 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 (th= is mistake was made with the Mac package too, but was one of the original r= equirements).

Please resolve these issues and I= 9;ll take another look.

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

EnterpriseDB UK: http://www.enterprisedb.com
The Ent= erprise PostgreSQL Company
--089e0129448aff332f0532687678--