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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
