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 2021/07/28 19:15:48 UTC
[GitHub] [geode-native] mreddington opened a new pull request #836: GEODE-9356: Adding netcore to build ALL
mreddington opened a new pull request #836:
URL: https://github.com/apache/geode-native/pull/836
This will cause our existing CI to build netcore on all platforms. The build tree is embedded in the source tree because as far as I can tell from the Microsoft documentation and community, that's how the MS build system works. We can separate source and build trees as soon as CSharp is supported in CMake for all platforms.
--
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: notifications-unsubscribe@geode.apache.org
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 #836: GEODE-9356: Adding netcore to build ALL
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #836:
URL: https://github.com/apache/geode-native/pull/836#discussion_r678702642
##########
File path: netcore/CMakeLists.txt
##########
@@ -0,0 +1,19 @@
+# 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.
+
+project(netcore LANGUAGES NONE)
+
+add_custom_target(netcore ALL COMMAND dotnet build WORKING_DIRECTORY = ${CMAKE_CURRENT_SOURCE_DIR}/NetCore DEPENDS apache-geode VERBATIM)
Review comment:
Doesn't this technically depend on the C-bindings target?
--
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: notifications-unsubscribe@geode.apache.org
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 #836: GEODE-9356: Adding netcore to build ALL
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #836:
URL: https://github.com/apache/geode-native/pull/836#discussion_r680094997
##########
File path: netcore/CMakeLists.txt
##########
@@ -22,4 +22,7 @@ find_program(DOTNET dotnet)
if(DOTNET AND INCLUDE_DOTNET)
add_custom_target(netcore ALL COMMAND ${DOTNET} build --configuration $<CONFIG> WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/NetCore VERBATIM)
add_custom_target(netcore-test ALL COMMAND ${DOTNET} build --configuration $<CONFIG> WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/NetCore.Test DEPENDS netcore VERBATIM)
+
+
+ install(FILES ${CMAKE_CURRENT_SOURCE_DIR}/NetCore/bin/$<CONFIG>/netcoreapp3.1/Apache.Geode.Netcore.dll DESTINATION netcore/$<CONFIG>)
Review comment:
Why are we including the 'Netcore' name in our library name? We don't include the language/platform name in any of our other library names.
--
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: notifications-unsubscribe@geode.apache.org
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 #836: GEODE-9356: Adding netcore to build ALL
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #836:
URL: https://github.com/apache/geode-native/pull/836#discussion_r679164298
##########
File path: netcore/CMakeLists.txt
##########
@@ -0,0 +1,19 @@
+# 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.
+
+project(netcore LANGUAGES NONE)
+
+add_custom_target(netcore ALL COMMAND dotnet build WORKING_DIRECTORY = ${CMAKE_CURRENT_SOURCE_DIR}/NetCore DEPENDS apache-geode VERBATIM)
Review comment:
> >Just a runtime dependency. PInvoke code just says "look for this function in apache-geode-c.DLL", build doesn't verify such a file exists or anything. We could make it dependent in cmake, but I don't think we'll really save anyone much time or trouble in the process. The entire .net core build is < 5 seconds on my laptop.
>
>
Ok, same difference. Runtime or compile time dependency, the dependency should be on the c binding target. If I ask CMake to build dotnet I would expect it to also build the runtime dependencies for it if they haven't or have changed. In this case any c binding changes would be missed.
--
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: notifications-unsubscribe@geode.apache.org
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 #836: GEODE-9356: Adding netcore to build ALL
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #836:
URL: https://github.com/apache/geode-native/pull/836#discussion_r678701372
##########
File path: netcore/CMakeLists.txt
##########
@@ -0,0 +1,19 @@
+# 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.
+
+project(netcore LANGUAGES NONE)
+
+add_custom_target(netcore ALL COMMAND dotnet build WORKING_DIRECTORY = ${CMAKE_CURRENT_SOURCE_DIR}/NetCore DEPENDS apache-geode VERBATIM)
Review comment:
I don't think that equals sign is supposed to be there. I am pretty surprised it doesn't scream at you or treat the path as literally "= /path/to/source/NetCore".
##########
File path: netcore/CMakeLists.txt
##########
@@ -0,0 +1,19 @@
+# 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.
+
+project(netcore LANGUAGES NONE)
+
Review comment:
Should we do a `find_program()` for `dotnet`?
Should we make building .NET Core bits optional with an `option()`?
--
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: notifications-unsubscribe@geode.apache.org
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 #836: GEODE-9356: Adding netcore to build ALL
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #836:
URL: https://github.com/apache/geode-native/pull/836#discussion_r679304794
##########
File path: netcore/CMakeLists.txt
##########
@@ -0,0 +1,19 @@
+# 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.
+
+project(netcore LANGUAGES NONE)
+
+add_custom_target(netcore ALL COMMAND dotnet build WORKING_DIRECTORY = ${CMAKE_CURRENT_SOURCE_DIR}/NetCore DEPENDS apache-geode VERBATIM)
Review comment:
This should also probably pass the current configuration to the dotnet so that it can build either `Debug` or `RelWithDebInfo` rather than just the default in the csproj.
```
add_custom_target(netcore ALL
COMMAND COMAND dotnet build --configuration $<CONFIG>
WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/NetCore
DEPENDS apache-geode
VERBATIM)
```
See https://cmake.org/cmake/help/latest/manual/cmake-generator-expressions.7.html
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] mreddington closed pull request #836: GEODE-9356: Adding netcore to build ALL
Posted by GitBox <gi...@apache.org>.
mreddington closed pull request #836:
URL: https://github.com/apache/geode-native/pull/836
--
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: notifications-unsubscribe@geode.apache.org
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 #836: GEODE-9356: Adding netcore to build ALL
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #836:
URL: https://github.com/apache/geode-native/pull/836#discussion_r679166879
##########
File path: netcore/CMakeLists.txt
##########
@@ -0,0 +1,19 @@
+# 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.
+
+project(netcore LANGUAGES NONE)
+
Review comment:
Although, given that this isn't nearly ready for release maybe it should be false.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on a change in pull request #836: GEODE-9356: Adding netcore to build ALL
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #836:
URL: https://github.com/apache/geode-native/pull/836#discussion_r678736680
##########
File path: netcore/CMakeLists.txt
##########
@@ -0,0 +1,19 @@
+# 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.
+
+project(netcore LANGUAGES NONE)
+
+add_custom_target(netcore ALL COMMAND dotnet build WORKING_DIRECTORY = ${CMAKE_CURRENT_SOURCE_DIR}/NetCore DEPENDS apache-geode VERBATIM)
Review comment:
>Just a runtime dependency. PInvoke code just says "look for this function in apache-geode-c.DLL", build doesn't verify such a file exists or anything. We could make it dependent in cmake, but I don't think we'll really save anyone much time or trouble in the process. The entire .net core build is < 5 seconds on my laptop.
--
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: notifications-unsubscribe@geode.apache.org
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 #836: GEODE-9356: Adding netcore to build ALL
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #836:
URL: https://github.com/apache/geode-native/pull/836#discussion_r685429859
##########
File path: packer/build-rhel-7.json
##########
@@ -144,6 +144,15 @@
"amazon-ebs"
]
},
+ {
+ "type": "shell",
Review comment:
Replace all theses with a single packer/linux/install-dotnet.sh script similar to the install-cmake.sh script. This way there is only one place to modify this script in the future. You can then call out this script in the packer just below the `"linux/install-cmake.sh"` line.
--
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: notifications-unsubscribe@geode.apache.org
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 #836: GEODE-9356: Adding netcore to build ALL
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #836:
URL: https://github.com/apache/geode-native/pull/836#discussion_r680092961
##########
File path: packer/build-windows-2016-vs-2017.json
##########
@@ -92,7 +92,8 @@
"choco install doxygen.install -confirm",
"choco install openssl -confirm",
"choco install strawberryperl -confirm",
- "choco install nuget.commandline -confirm"
+ "choco install nuget.commandline -confirm",
+ "choco install dotnet-5.0-sdk -confirm"
Review comment:
Our target is .NET 3.1 LTS. Does 5.0 SDK allow you to target 3.1?
##########
File path: netcore/CMakeLists.txt
##########
@@ -15,7 +15,7 @@
project(netcore LANGUAGES NONE)
-option(INCLUDE_DOTNET "" ON)
+option(INCLUDE_DOTNET "Dotnet 5" ON)
Review comment:
`option(INCLUDE_DOTNET_CORE "Build .NET Core client." ON)`
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] echobravopapa commented on a change in pull request #836: GEODE-9356: Adding netcore to build ALL
Posted by GitBox <gi...@apache.org>.
echobravopapa commented on a change in pull request #836:
URL: https://github.com/apache/geode-native/pull/836#discussion_r680189681
##########
File path: netcore/CMakeLists.txt
##########
@@ -22,4 +22,7 @@ find_program(DOTNET dotnet)
if(DOTNET AND INCLUDE_DOTNET)
add_custom_target(netcore ALL COMMAND ${DOTNET} build --configuration $<CONFIG> WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/NetCore VERBATIM)
add_custom_target(netcore-test ALL COMMAND ${DOTNET} build --configuration $<CONFIG> WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/NetCore.Test DEPENDS netcore VERBATIM)
+
+
+ install(FILES ${CMAKE_CURRENT_SOURCE_DIR}/NetCore/bin/$<CONFIG>/netcoreapp3.1/Apache.Geode.Netcore.dll DESTINATION netcore/$<CONFIG>)
Review comment:
This is true; its a bit verbose, but does ensure no one will confuse the two .Net libraries...
any suggestions on the naming?
`Apache.Geode.Platinum.dll`
`Apache.Geode.Modern.dll`
--
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: notifications-unsubscribe@geode.apache.org
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 #836: GEODE-9356: Adding netcore to build ALL
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #836:
URL: https://github.com/apache/geode-native/pull/836#discussion_r680258035
##########
File path: netcore/CMakeLists.txt
##########
@@ -22,4 +22,7 @@ find_program(DOTNET dotnet)
if(DOTNET AND INCLUDE_DOTNET)
add_custom_target(netcore ALL COMMAND ${DOTNET} build --configuration $<CONFIG> WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/NetCore VERBATIM)
add_custom_target(netcore-test ALL COMMAND ${DOTNET} build --configuration $<CONFIG> WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/NetCore.Test DEPENDS netcore VERBATIM)
+
+
+ install(FILES ${CMAKE_CURRENT_SOURCE_DIR}/NetCore/bin/$<CONFIG>/netcoreapp3.1/Apache.Geode.Netcore.dll DESTINATION netcore/$<CONFIG>)
Review comment:
Pretty hard to confuse the two since both compile time and runtime would fail. It seems common in the .NET space to have the assemblies and base package names consistent and to distinguish different versions of the platform by directory name.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] mreddington commented on a change in pull request #836: GEODE-9356: Adding netcore to build ALL
Posted by GitBox <gi...@apache.org>.
mreddington commented on a change in pull request #836:
URL: https://github.com/apache/geode-native/pull/836#discussion_r679143400
##########
File path: netcore/CMakeLists.txt
##########
@@ -0,0 +1,19 @@
+# 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.
+
+project(netcore LANGUAGES NONE)
+
Review comment:
I can do these things. Should the build option be on by default?
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] mreddington commented on a change in pull request #836: GEODE-9356: Adding netcore to build ALL
Posted by GitBox <gi...@apache.org>.
mreddington commented on a change in pull request #836:
URL: https://github.com/apache/geode-native/pull/836#discussion_r679142894
##########
File path: netcore/CMakeLists.txt
##########
@@ -0,0 +1,19 @@
+# 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.
+
+project(netcore LANGUAGES NONE)
+
+add_custom_target(netcore ALL COMMAND dotnet build WORKING_DIRECTORY = ${CMAKE_CURRENT_SOURCE_DIR}/NetCore DEPENDS apache-geode VERBATIM)
Review comment:
Ha! I'm just as surprised as you are.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on a change in pull request #836: GEODE-9356: Adding netcore to build ALL
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #836:
URL: https://github.com/apache/geode-native/pull/836#discussion_r678734864
##########
File path: netcore/CMakeLists.txt
##########
@@ -0,0 +1,19 @@
+# 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.
+
+project(netcore LANGUAGES NONE)
+
+add_custom_target(netcore ALL COMMAND dotnet build WORKING_DIRECTORY = ${CMAKE_CURRENT_SOURCE_DIR}/NetCore DEPENDS apache-geode VERBATIM)
Review comment:
Actually no, it's a runtime dependency. PInvoke code just says "look for this function in apache-geode-c.DLL", build doesn't verify such a file exists or anything.
--
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: notifications-unsubscribe@geode.apache.org
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 #836: GEODE-9356: Adding netcore to build ALL
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #836:
URL: https://github.com/apache/geode-native/pull/836#discussion_r685426550
##########
File path: netcore/CMakeLists.txt
##########
@@ -0,0 +1,19 @@
+# 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.
+
+project(netcore LANGUAGES NONE)
+
+add_custom_target(netcore ALL COMMAND dotnet build WORKING_DIRECTORY = ${CMAKE_CURRENT_SOURCE_DIR}/NetCore DEPENDS apache-geode VERBATIM)
Review comment:
> Still disagree on the dependencies - IMO CMakeLists.txt dependencies should enforce build-time dependencies _only_, and this ain't that. Either netcore or c bindings failing will still fail the entire build, but adding the dependency complicates the build graph and introduces potential to slow things down or introduce bugs.
Not entirely true. Make a change to C-binding to correct or change behavior, change .NET Core library to compensate for changes, now when you compile the .NET Core tests to validate your new behavior it fails at runtime because you also didn't build the C-binding. You have to go build the C-binding manually to pick up that change then run the test. While not strictly a compile time dependency it is a very hard runtime dependency. Even runtime dependencies should be checked to make sure we have the correct version and since this one is source based the correct version may require a compilation.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] moleske edited a comment on pull request #836: GEODE-9356: Adding netcore to build ALL
Posted by GitBox <gi...@apache.org>.
moleske edited a comment on pull request #836:
URL: https://github.com/apache/geode-native/pull/836#issuecomment-889533452
It would be nice to update the README and BUILDING files to reflect the new framework support and requirements. Probably can tack on the word `experimental` if you don't want to commit general availability yet. It took me some time to figure out what was going on as I had .NET 5 installed and not .NET Core 3.1 so I could not build at first.
--
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: notifications-unsubscribe@geode.apache.org
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 #836: GEODE-9356: Adding netcore to build ALL
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #836:
URL: https://github.com/apache/geode-native/pull/836#discussion_r680095158
##########
File path: packer/build-windows-2016-vs-2017.json
##########
@@ -92,7 +92,8 @@
"choco install doxygen.install -confirm",
"choco install openssl -confirm",
"choco install strawberryperl -confirm",
- "choco install nuget.commandline -confirm"
+ "choco install nuget.commandline -confirm",
+ "choco install dotnet-5.0-sdk -confirm"
Review comment:
You beat me to it.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on a change in pull request #836: GEODE-9356: Adding netcore to build ALL
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #836:
URL: https://github.com/apache/geode-native/pull/836#discussion_r685359076
##########
File path: netcore/CMakeLists.txt
##########
@@ -0,0 +1,19 @@
+# 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.
+
+project(netcore LANGUAGES NONE)
+
+add_custom_target(netcore ALL COMMAND dotnet build WORKING_DIRECTORY = ${CMAKE_CURRENT_SOURCE_DIR}/NetCore DEPENDS apache-geode VERBATIM)
Review comment:
Completely agree on passing on configuration, tho.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on a change in pull request #836: GEODE-9356: Adding netcore to build ALL
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #836:
URL: https://github.com/apache/geode-native/pull/836#discussion_r678734864
##########
File path: netcore/CMakeLists.txt
##########
@@ -0,0 +1,19 @@
+# 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.
+
+project(netcore LANGUAGES NONE)
+
+add_custom_target(netcore ALL COMMAND dotnet build WORKING_DIRECTORY = ${CMAKE_CURRENT_SOURCE_DIR}/NetCore DEPENDS apache-geode VERBATIM)
Review comment:
Actually no, it's a runtime dependency. PInvoke code just says "look for this function in apache-geode-c.DLL", build doesn't verify such a file exists or anything.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] moleske commented on pull request #836: GEODE-9356: Adding netcore to build ALL
Posted by GitBox <gi...@apache.org>.
moleske commented on pull request #836:
URL: https://github.com/apache/geode-native/pull/836#issuecomment-889533452
It would be nice to update the README and BUILDING files to reflect support and requirements. Probably can tack on the word `experimental` if you don't want to commit general availability yet. It took me some time to figure out what was going on as I had .NET 5 installed and not .NET Core 3.1 so I could not build at first.
--
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: notifications-unsubscribe@geode.apache.org
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 #836: GEODE-9356: Adding netcore to build ALL
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #836:
URL: https://github.com/apache/geode-native/pull/836#discussion_r679304794
##########
File path: netcore/CMakeLists.txt
##########
@@ -0,0 +1,19 @@
+# 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.
+
+project(netcore LANGUAGES NONE)
+
+add_custom_target(netcore ALL COMMAND dotnet build WORKING_DIRECTORY = ${CMAKE_CURRENT_SOURCE_DIR}/NetCore DEPENDS apache-geode VERBATIM)
Review comment:
This should also probably pass the current configuration to the dotnet so that it can build either `Debug` or `DebWithRelInfo` rather than just the default in the csproj.
```
add_custom_target(netcore ALL
COMMAND COMAND dotnet build --configuration $<CONFIG>
WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/NetCore
DEPENDS apache-geode
VERBATIM)
```
See https://cmake.org/cmake/help/latest/manual/cmake-generator-expressions.7.html
--
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: notifications-unsubscribe@geode.apache.org
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 #836: GEODE-9356: Adding netcore to build ALL
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #836:
URL: https://github.com/apache/geode-native/pull/836#discussion_r679165744
##########
File path: netcore/CMakeLists.txt
##########
@@ -0,0 +1,19 @@
+# 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.
+
+project(netcore LANGUAGES NONE)
+
Review comment:
Maybe it should default to true if dotnet executable is found otherwise false? 🤷
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] mreddington commented on a change in pull request #836: GEODE-9356: Adding netcore to build ALL
Posted by GitBox <gi...@apache.org>.
mreddington commented on a change in pull request #836:
URL: https://github.com/apache/geode-native/pull/836#discussion_r686199743
##########
File path: packer/build-rhel-7.json
##########
@@ -144,6 +144,15 @@
"amazon-ebs"
]
},
+ {
+ "type": "shell",
Review comment:
I was just thinking this myself, once I noticed there were shell scripts in there already.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on a change in pull request #836: GEODE-9356: Adding netcore to build ALL
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #836:
URL: https://github.com/apache/geode-native/pull/836#discussion_r685358575
##########
File path: netcore/CMakeLists.txt
##########
@@ -0,0 +1,19 @@
+# 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.
+
+project(netcore LANGUAGES NONE)
+
+add_custom_target(netcore ALL COMMAND dotnet build WORKING_DIRECTORY = ${CMAKE_CURRENT_SOURCE_DIR}/NetCore DEPENDS apache-geode VERBATIM)
Review comment:
Still disagree on the dependencies - IMO CMakeLists.txt dependencies should enforce build-time dependencies _only_, and this ain't that. Either netcore or c bindings failing will still fail the entire build, but adding the dependency complicates the build graph and introduces potential to slow things down or introduce bugs.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org