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