You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by GitBox <gi...@apache.org> on 2022/09/06 09:00:23 UTC
[GitHub] [incubator-kvrocks] aleksraiden opened a new pull request, #827: Update utils.cmake
aleksraiden opened a new pull request, #827:
URL: https://github.com/apache/incubator-kvrocks/pull/827
Set an DOWNLOAD_EXTRACT_TIMESTAMP options to new behavior (by cmake policy CMP0135). Close #826
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] torwig commented on a diff in pull request #827: Add cmake policy file
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #827:
URL: https://github.com/apache/incubator-kvrocks/pull/827#discussion_r963558200
##########
cmake/policy.cmake:
##########
@@ -0,0 +1,28 @@
+# 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.
+
+# it must be a macro cause policies have scopes
+# http://www.cmake.org/cmake/help/v3.0/command/cmake_policy.html
+
+macro (kvrocks_policy)
+
+ # Avoid warning about DOWNLOAD_EXTRACT_TIMESTAMP in CMake 3.24:
Review Comment:
```suggestion
# Avoid warning about DOWNLOAD_EXTRACT_TIMESTAMP in CMake 3.24:
```
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun commented on pull request #827: Fix CMake policy CMP0135
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #827:
URL: https://github.com/apache/incubator-kvrocks/pull/827#issuecomment-1238386110
@aleksraiden merged. Thanks for your contribution!
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] aleksraiden commented on pull request #827: Add cmake policy file
Posted by GitBox <gi...@apache.org>.
aleksraiden commented on PR #827:
URL: https://github.com/apache/incubator-kvrocks/pull/827#issuecomment-1238038840
Lot of thanks @torwig
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] torwig commented on a diff in pull request #827: Add cmake policy file
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #827:
URL: https://github.com/apache/incubator-kvrocks/pull/827#discussion_r963559326
##########
cmake/policy.cmake:
##########
@@ -0,0 +1,28 @@
+# 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.
+
+# it must be a macro cause policies have scopes
+# http://www.cmake.org/cmake/help/v3.0/command/cmake_policy.html
+
+macro (kvrocks_policy)
+
+ # Avoid warning about DOWNLOAD_EXTRACT_TIMESTAMP in CMake 3.24:
+ if (CMAKE_VERSION VERSION_GREATER_EQUAL "3.24.0")
+ cmake_policy(SET CMP0135 NEW)
Review Comment:
```suggestion
cmake_policy(SET CMP0135 NEW)
```
##########
cmake/policy.cmake:
##########
@@ -0,0 +1,28 @@
+# 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.
+
+# it must be a macro cause policies have scopes
+# http://www.cmake.org/cmake/help/v3.0/command/cmake_policy.html
+
+macro (kvrocks_policy)
+
+ # Avoid warning about DOWNLOAD_EXTRACT_TIMESTAMP in CMake 3.24:
+ if (CMAKE_VERSION VERSION_GREATER_EQUAL "3.24.0")
+ cmake_policy(SET CMP0135 NEW)
+ endif()
+
Review Comment:
```suggestion
```
##########
cmake/policy.cmake:
##########
@@ -0,0 +1,28 @@
+# 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.
+
+# it must be a macro cause policies have scopes
+# http://www.cmake.org/cmake/help/v3.0/command/cmake_policy.html
+
+macro (kvrocks_policy)
+
Review Comment:
```suggestion
```
##########
cmake/policy.cmake:
##########
@@ -0,0 +1,28 @@
+# 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.
+
+# it must be a macro cause policies have scopes
+# http://www.cmake.org/cmake/help/v3.0/command/cmake_policy.html
+
+macro (kvrocks_policy)
+
+ # Avoid warning about DOWNLOAD_EXTRACT_TIMESTAMP in CMake 3.24:
+ if (CMAKE_VERSION VERSION_GREATER_EQUAL "3.24.0")
+ cmake_policy(SET CMP0135 NEW)
+ endif()
Review Comment:
```suggestion
endif()
```
##########
cmake/policy.cmake:
##########
@@ -0,0 +1,28 @@
+# 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.
+
+# it must be a macro cause policies have scopes
+# http://www.cmake.org/cmake/help/v3.0/command/cmake_policy.html
+
+macro (kvrocks_policy)
+
+ # Avoid warning about DOWNLOAD_EXTRACT_TIMESTAMP in CMake 3.24:
+ if (CMAKE_VERSION VERSION_GREATER_EQUAL "3.24.0")
Review Comment:
```suggestion
if (CMAKE_VERSION VERSION_GREATER_EQUAL "3.24.0")
```
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] aleksraiden commented on pull request #827: Add cmake policy file
Posted by GitBox <gi...@apache.org>.
aleksraiden commented on PR #827:
URL: https://github.com/apache/incubator-kvrocks/pull/827#issuecomment-1238110944
Of course, your version is more clear. Lot of thanks.
But - if we need a other patch like this, this version isn't so good as a proposition to a split code into files, as I think. I tried to watch how this work into an other top-level projects, like this - [ethereum/solidity](https://github.com/ethereum/solidity/blob/4159d13a996d8b376ae33f2be58b9651269daee4/cmake/EthPolicy.cmake) and they have a lot of policy.
But, yes, your patch is awesome simple and clean.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] aleksraiden commented on pull request #827: Update utils.cmake
Posted by GitBox <gi...@apache.org>.
aleksraiden commented on PR #827:
URL: https://github.com/apache/incubator-kvrocks/pull/827#issuecomment-1237962351
Sorry, I Am newbie in CMake, but tried to make this clear as possible. In dedicated policy.cmake we can add any options in futures.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] aleksraiden commented on pull request #827: Add cmake policy file
Posted by GitBox <gi...@apache.org>.
aleksraiden commented on PR #827:
URL: https://github.com/apache/incubator-kvrocks/pull/827#issuecomment-1238337123
@tisonkun Done, lot of 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.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun merged pull request #827: Fix CMake policy CMP0135
Posted by GitBox <gi...@apache.org>.
tisonkun merged PR #827:
URL: https://github.com/apache/incubator-kvrocks/pull/827
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] aleksraiden commented on pull request #827: Update utils.cmake
Posted by GitBox <gi...@apache.org>.
aleksraiden commented on PR #827:
URL: https://github.com/apache/incubator-kvrocks/pull/827#issuecomment-1237870445
Not so trivial, need to use macros for define options only for CMake > 3.24.0. I will patch this asap
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] torwig commented on pull request #827: Add cmake policy file
Posted by GitBox <gi...@apache.org>.
torwig commented on PR #827:
URL: https://github.com/apache/incubator-kvrocks/pull/827#issuecomment-1237963923
Applying this patch doesn't cause any issues with the build process on Fedora 35 as well.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun commented on pull request #827: Add cmake policy file
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #827:
URL: https://github.com/apache/incubator-kvrocks/pull/827#issuecomment-1238144011
> if we need a other patch like this
Let's factor policies out if this is true.
[GoogleTest](https://github.com/google/googletest/blob/0e0d9feefab1b51aaab9dfd70132e93c0b6964e5/CMakeLists.txt#L6-L16) follows this simple way, and so do libevent, lz4, rocksdb if you check our dependencies.
We don't have to follow other projects and these references are only for reference. My criteria are keeping simple things simple, and refactoring only if we do meet some problems.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org