kaknikhil commented on a change in pull request #574:
URL: https://github.com/apache/madlib/pull/574#discussion_r759614374



##########
File path: deploy/gppkg/gppkg_spec.yml.in
##########
@@ -17,6 +17,3 @@ PostInstall:
            echo 'For additional options run:';
            echo '$ madpack --help';
            echo 'Release notes and additional documentation can be found at 
http://madlib.apache.org';"
-PostUninstall:

Review comment:
       Since this change is because of the reverted commit, it would be good to 
add a message to the reverted commit 

##########
File path: src/madpack/madpack.py
##########
@@ -1409,16 +1419,41 @@ def main(argv):
         else:
             maddir_conf = maddir + "/config"
 
-        global maddir_lib
-        if portid == 'greenplum' and \
-           os.path.islink(maddir + "/../../../lib/postgresql/libmadlib.so"):
-           maddir_lib = '$libdir/libmadlib.so'
-        elif os.path.isfile(maddir + "/ports/" + portid + "/" + dbver +
+        global madlib_library_path
+        # Local build at ~/workspace/madlib/build/
+        if os.path.isfile(maddir_abs + "/src/ports/" + portid + "/" + dbver +

Review comment:
       Just an idea, isn't it possible to use python's glob module to find the 
location of libmadlib.so inside `maddir_abs` ? That way we won't have to depend 
on these if else conditions

##########
File path: src/madpack/madpack.py
##########
@@ -39,6 +39,7 @@
 # (b) __file__ (instead of sys.argv[0]) because madpack.py could be called
 # (a) through a symbolic link and (b) not as the main module.
 maddir = os.path.abspath(os.path.dirname(os.path.realpath(__file__)) + "/..")  
 # MADlib root dir
+maddir_abs = os.path.abspath(os.path.dirname(os.path.abspath(__file__)) + 
"/../..")   # MADlib abs root dir

Review comment:
       The names `maddir` and `maddir_abs` are a bit confusing since they can 
be used interchangeably. Do you think there's a better way to name these 
variables ?

##########
File path: src/madpack/madpack.py
##########
@@ -1409,16 +1419,41 @@ def main(argv):
         else:
             maddir_conf = maddir + "/config"
 
-        global maddir_lib
-        if portid == 'greenplum' and \
-           os.path.islink(maddir + "/../../../lib/postgresql/libmadlib.so"):
-           maddir_lib = '$libdir/libmadlib.so'
-        elif os.path.isfile(maddir + "/ports/" + portid + "/" + dbver +
+        global madlib_library_path

Review comment:
       maybe extract all this logic into two functions
   1.  find_madlib_library_path
   2.  set_dynamic_library_path_in_database

##########
File path: src/madpack/madpack.py
##########
@@ -1409,16 +1419,41 @@ def main(argv):
         else:
             maddir_conf = maddir + "/config"
 
-        global maddir_lib
-        if portid == 'greenplum' and \
-           os.path.islink(maddir + "/../../../lib/postgresql/libmadlib.so"):
-           maddir_lib = '$libdir/libmadlib.so'
-        elif os.path.isfile(maddir + "/ports/" + portid + "/" + dbver +
+        global madlib_library_path

Review comment:
       why do we need all these variables to be global ? Isn't there a way to 
get this done without global variables ? 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@madlib.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


Reply via email to