On Mon, May 9, 2016 at 6:35 PM, Dave Page <dp...@pgadmin.org> wrote: > Hi > > Initial eyeball review comments below... > > On Fri, Apr 22, 2016 at 11:57 AM, Sandeep Thakkar < > sandeep.thak...@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-<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? > 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/<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. > > 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
