You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@madlib.apache.org by GitBox <gi...@apache.org> on 2021/11/15 04:23:52 UTC
[GitHub] [madlib] orhankislal opened a new pull request #574: Build: Use dynamic_library_path for module pathname
orhankislal opened a new pull request #574:
URL: https://github.com/apache/madlib/pull/574
MADlib used to hard code the path for the module_pathname.
This has been changed to accomodate for major version upgrades of
Postgres and Greenplum. However, some systems do not play well
with symlinks, so we revert that change and use the
dynamic_library_path GUC to find the .so file instead.
--
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-unsubscribe@madlib.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [madlib] orhankislal commented on a change in pull request #574: Build: Use dynamic_library_path for module pathname
Posted by GitBox <gi...@apache.org>.
orhankislal commented on a change in pull request #574:
URL: https://github.com/apache/madlib/pull/574#discussion_r759700641
##########
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:
The If part is under the maddir_abs folder but the elif part is outside (since we want to make sure we are looking at the Current folder, not Versions). This makes glob complicated because we don't want to search outside the madlib folder accidentally.
--
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-unsubscribe@madlib.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [madlib] orhankislal merged pull request #574: Build: Use dynamic_library_path for module pathname
Posted by GitBox <gi...@apache.org>.
orhankislal merged pull request #574:
URL: https://github.com/apache/madlib/pull/574
--
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-unsubscribe@madlib.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [madlib] kaknikhil commented on a change in pull request #574: Build: Use dynamic_library_path for module pathname
Posted by GitBox <gi...@apache.org>.
kaknikhil commented on a change in pull request #574:
URL: https://github.com/apache/madlib/pull/574#discussion_r763467209
##########
File path: src/madpack/madpack.py
##########
@@ -1274,6 +1283,44 @@ def get_madlib_operator_drop_str(schema):
schema, i['name'], i['left_op'], i['right_op'])
return drop_str
+def find_madlib_library_path():
+
+ global madlib_library_path
+ # Local build at ~/workspace/madlib/build/
+ if os.path.isfile(maddir + "/../src/ports/" + portid + "/" + dbver +
+ "/lib/libmadlib.so"):
+ madlib_library_path = maddir + "/../src/ports/" + portid + "/" + dbver + \
+ "/lib"
+
+ # Package build at /usr/local/madlib/Versions or $GPHOME/madlib/Versions
+ elif os.path.isfile(maddir + "/../../Current/ports/" + portid + "/" + dbver +
+ "/lib/libmadlib.so"):
+ madlib_library_path = maddir + "/../../Current/ports/" + portid + "/" + dbver + \
+ "/lib"
+ else:
+ madlib_library_path = maddir + "/lib"
+
+def set_dynamic_library_path_in_database(dbver_split):
+
+ global dynamic_library_path
Review comment:
Do we still need this and `madlib_library_path` to be global variables ? find_madlib_library_path can return `madlib_library_path` which can then be used by `set_dynamic_library_path_in_database`
--
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-unsubscribe@madlib.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [madlib] orhankislal commented on a change in pull request #574: Build: Use dynamic_library_path for module pathname
Posted by GitBox <gi...@apache.org>.
orhankislal commented on a change in pull request #574:
URL: https://github.com/apache/madlib/pull/574#discussion_r759700641
##########
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:
The If part is under the maddir_abs folder but the elif part is outside (since we want to make sure we are looking at the Current folder, not Versions). This makes glob complicated because we don't want to search outside the madlib build folder accidentally.
--
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-unsubscribe@madlib.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [madlib] kaknikhil commented on a change in pull request #574: Build: Use dynamic_library_path for module pathname
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@madlib.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [madlib] orhankislal commented on a change in pull request #574: Build: Use dynamic_library_path for module pathname
Posted by GitBox <gi...@apache.org>.
orhankislal commented on a change in pull request #574:
URL: https://github.com/apache/madlib/pull/574#discussion_r759700395
##########
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:
The `If` part is under the maddir_abs folder but the `elif` part is outside (since we want to make sure we are looking at the Current folder, not Versions). This makes glob complicated because we don't want to search outside the madlib folder accidentally.
--
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-unsubscribe@madlib.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [madlib] orhankislal commented on a change in pull request #574: Build: Use dynamic_library_path for module pathname
Posted by GitBox <gi...@apache.org>.
orhankislal commented on a change in pull request #574:
URL: https://github.com/apache/madlib/pull/574#discussion_r759700641
##########
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:
The `if` part is under the maddir_abs folder but the `elif` part is outside (since we want to make sure we are looking at the Current folder, not Versions). This makes glob complicated because we don't want to search outside the madlib build folder accidentally.
--
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-unsubscribe@madlib.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org