You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2020/11/20 02:10:19 UTC
[GitHub] [geode-native] gaussianrecurrence opened a new pull request #700: GEODE-8735: Change factory logic for symbols lookup
gaussianrecurrence opened a new pull request #700:
URL: https://github.com/apache/geode-native/pull/700
- In order to allow that factory logic looks into the application space
for symbols, `library-name` field has been made optional.
- This removes the need to create shared library for in order to define
custom factories like cache loaders, cache writers, cache listeners,
partition resolvers or persistance managers whenever the cache is
instantiated declaratively.
- Whenever library-name is not specified a handle to the executable is
obtained.
- Take into account that for OS like Linux, Mac... in addition to exporting
the symbols in the application, you would need to specify `-rdynamic`
option while compiling.
- Documentation has been updated.
---
**NOTE**. Take a look into the Jira issue for additional context.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on a change in pull request #700: GEODE-8735: Change factory logic for symbols lookup
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #700:
URL: https://github.com/apache/geode-native/pull/700#discussion_r546034053
##########
File path: cppcache/integration/test/resources/pr_app_client_cache.xml
##########
@@ -0,0 +1,29 @@
+<?xml version="1.0"?>
+
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+<client-cache xmlns="http://geode.apache.org/schema/cache"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://geode.apache.org/schema/cache http://geode.apache.org/schema/cache/cache-1.0.xsd"
Review comment:
@pivotal-jbarrett shouldn't the schemaLocation for both of these be "http://geode.apache.org/schema/cpp-cache/cpp-cache-1.0.xsd"?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #700: GEODE-8735: Change factory logic for symbols lookup
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #700:
URL: https://github.com/apache/geode-native/pull/700#discussion_r546533140
##########
File path: cppcache/integration/test/resources/pr_app_client_cache.xml
##########
@@ -0,0 +1,29 @@
+<?xml version="1.0"?>
+
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+<client-cache xmlns="http://geode.apache.org/schema/cache"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://geode.apache.org/schema/cache http://geode.apache.org/schema/cache/cache-1.0.xsd"
Review comment:
No, it's lost of pairs of namespace uri to url mapping.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] gaussianrecurrence commented on a change in pull request #700: GEODE-8735: Change factory logic for symbols lookup
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #700:
URL: https://github.com/apache/geode-native/pull/700#discussion_r557317856
##########
File path: cppcache/integration/test/resources/pr_app_client_cache.xml
##########
@@ -0,0 +1,29 @@
+<?xml version="1.0"?>
+
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+<client-cache xmlns="http://geode.apache.org/schema/cache"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://geode.apache.org/schema/cache http://geode.apache.org/schema/cache/cache-1.0.xsd"
Review comment:
Sadly I am not really aware with the XML internals, but just to be clear, should I change the namespace to: https://geode.apache.org/schema/cpp-cache and the schema to https://geode.apache.org/schema/cpp-cache/cpp-cache-1.0.xsd ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] gaussianrecurrence commented on pull request #700: GEODE-8735: Change factory logic for symbols lookup
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on pull request #700:
URL: https://github.com/apache/geode-native/pull/700#issuecomment-733945720
There's one thing I am not sure about if it's the right call and that's having added a modification of GenerateExportHeaders. This is necessary in order to specify the visibility flags to the factory function added to cpp-integration-test for testing purposes. Thing is GenerateExportHeaders does not support executables, so that's the rationale behind ExecutableExportHeader.cmake
Please let me know what you think about it and if you think there might be a better alternative.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request #700: GEODE-8735: Change factory logic for symbols lookup
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on pull request #700:
URL: https://github.com/apache/geode-native/pull/700#issuecomment-730822328
> * Take into account that for OS like Linux, Mac... in addition to exporting
> the symbols in the application, you would need to specify `-rdynamic`
> option while compiling.
This might make great example with corresponding CMake configuration.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] gaussianrecurrence commented on pull request #700: GEODE-8735: Change factory logic for symbols lookup
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on pull request #700:
URL: https://github.com/apache/geode-native/pull/700#issuecomment-747348815
Maybe @pdxcodemonkey @echobravopapa @moleske @mivanac @mkevo could you also review this, so it can be merged? Thanks!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #700: GEODE-8735: Change factory logic for symbols lookup
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #700:
URL: https://github.com/apache/geode-native/pull/700#discussion_r527365312
##########
File path: cppcache/src/Utils.cpp
##########
@@ -173,20 +174,22 @@ const boost::dll::shared_library& getSharedLibrary(
const auto& find = sharedLibraries.find(libraryName);
if (find == sharedLibraries.end()) {
+ auto path = libraryName.empty() ? boost::dll::program_location()
Review comment:
Great timing for me merging the boost::dll changes the other day. This change looks pretty straight forward!
##########
File path: cppcache/src/Utils.cpp
##########
@@ -173,20 +174,22 @@ const boost::dll::shared_library& getSharedLibrary(
const auto& find = sharedLibraries.find(libraryName);
if (find == sharedLibraries.end()) {
+ auto path = libraryName.empty() ? boost::dll::program_location()
+ : boost::dll::fs::path{libraryName};
try {
return *sharedLibraries
.emplace(
libraryName,
std::make_shared<boost::dll::shared_library>(
- libraryName,
+ path,
boost::dll::load_mode::rtld_global |
boost::dll::load_mode::rtld_lazy |
boost::dll::load_mode::append_decorations |
boost::dll::load_mode::search_system_folders))
.first->second;
} catch (const boost::dll::fs::system_error& e) {
- throw IllegalArgumentException("cannot open library: " + libraryName +
- ": reason: " + e.what());
+ throw IllegalArgumentException("cannot open library: \"" + libraryName +
Review comment:
I wonder if path makes more sense here now? Can the `shared_library` construction fail ever in the case of `program_location`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #700: GEODE-8735: Change factory logic for symbols lookup
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #700:
URL: https://github.com/apache/geode-native/pull/700#discussion_r561322182
##########
File path: cppcache/integration/test/resources/pr_app_client_cache.xml
##########
@@ -0,0 +1,29 @@
+<?xml version="1.0"?>
+
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+<client-cache xmlns="http://geode.apache.org/schema/cache"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://geode.apache.org/schema/cache http://geode.apache.org/schema/cache/cache-1.0.xsd"
Review comment:
Yes, but I think the parser is still non-validating so it won't really matter. I suspect many of our client xml files are bad. The Java server and client will validated and fail of the namespace and location are bad.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #700: GEODE-8735: Change factory logic for symbols lookup
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #700:
URL: https://github.com/apache/geode-native/pull/700#discussion_r561322182
##########
File path: cppcache/integration/test/resources/pr_app_client_cache.xml
##########
@@ -0,0 +1,29 @@
+<?xml version="1.0"?>
+
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+<client-cache xmlns="http://geode.apache.org/schema/cache"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://geode.apache.org/schema/cache http://geode.apache.org/schema/cache/cache-1.0.xsd"
Review comment:
Yes, but I think the parser is still non-validating so it won't really matter. I suspect many of our client xml files are bad. The Java server and client will validated and fail of the namespace and location are bad.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pdxcodemonkey merged pull request #700: GEODE-8735: Change factory logic for symbols lookup
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey merged pull request #700:
URL: https://github.com/apache/geode-native/pull/700
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] gaussianrecurrence commented on pull request #700: GEODE-8735: Change factory logic for symbols lookup
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on pull request #700:
URL: https://github.com/apache/geode-native/pull/700#issuecomment-733946173
> > * Take into account that for OS like Linux, Mac... in addition to exporting
> > the symbols in the application, you would need to specify `-rdynamic`
> > option while compiling.
>
> This might make great example with corresponding CMake configuration.
I've found out that CMake support exposing executable symbols by setting ENABLE_EXPORTS=1 property to the executable target. I've used it on IT, so it could serve as an example. If you think it should be stated more explicitly somewhere else, please let me know :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org