[GitHub] [logging-log4cxx] ams-tschoening commented on a change in pull request #49: Documentation updates

[GitBox <gi...@apache.org>]

ams-tschoening commented on a change in pull request #49:
URL: https://github.com/apache/logging-log4cxx/pull/49#discussion_r554610161



##########
File path: CMakeLists.txt
##########
@@ -103,6 +105,25 @@ foreach(varName HAS_STD_LOCALE  HAS_ODBC  HAS_MBSRTOWCS  HAS_WCSTOMBS  HAS_FWIDE
   endif()
 endforeach()
 
+#
+# Package and sign if Apache maintainer
+#
+option(APACHE_MAINTAINER "Apache maintainer" OFF)
+if(APACHE_MAINTAINER)
+set(CPACK_SOURCE_PACKAGE_FILE_NAME "apache-log4cxx-${LOG4CXX_RELEASE_VERSION}")
+set(CPACK_SOURCE_GENERATOR "TGZ;ZIP")
+set(CPACK_SOURCE_IGNORE_FILES ".git/;build/")
+include(CPack)
+
+add_custom_target( dist 

Review comment:
       How do you execute that target, on the shell and with which command? Visual Studio doesn't seem to provide anything obvious in its UI, `installing` doesn't seem to trigger that target. Adding `ALL` like for Doxygen does, but that's not what you intended?

##########
File path: CMakeLists.txt
##########
@@ -103,6 +105,25 @@ foreach(varName HAS_STD_LOCALE  HAS_ODBC  HAS_MBSRTOWCS  HAS_WCSTOMBS  HAS_FWIDE
   endif()
 endforeach()
 
+#
+# Package and sign if Apache maintainer
+#
+option(APACHE_MAINTAINER "Apache maintainer" OFF)
+if(APACHE_MAINTAINER)
+set(CPACK_SOURCE_PACKAGE_FILE_NAME "apache-log4cxx-${LOG4CXX_RELEASE_VERSION}")
+set(CPACK_SOURCE_GENERATOR "TGZ;ZIP")
+set(CPACK_SOURCE_IGNORE_FILES ".git/;build/")

Review comment:
       I have more things to exclude, some of those even preventing things from working:
   
   ```
   Schweregrad	Code	Beschreibung	Projekt	Datei	Zeile	Unterdrückungszustand
   Fehler		Problem copying file: C:/Users/tschoening/Documents/Svn/Src/Libs/trunk/C++/Logging/log4cxx/master/src/.vs/src/v16/Browse.VC.db -> C:/Users/tschoening/Documents/Svn/Src/Libs/trunk/C++/Logging/log4cxx/master/src/out/build/x64-Debug/_CPack_Packages/win64-Source/TGZ/apache-log4cxx-0.11.0////.vs/src/v16/Browse.VC.db	C:\Users\tschoening\Documents\Svn\Src\Libs\trunk\C++\Logging\log4cxx\master\src\out\build\x64-Debug\src	C:\Users\tschoening\Documents\Svn\Src\Libs\trunk\C++\Logging\log4cxx\master\src\out\build\x64-Debug\EXEC	1
   ```
   
   I need the following instead:
   
   > set(CPACK_SOURCE_IGNORE_FILES ".git/;.vs/;build/;out/")
   
   Your `build` is `out/build` and `out/install` for me. That's configured in my JSON file, though I'm not sure if that was a default of Visual Studio or a decision I made.

##########
File path: CMakeLists.txt
##########
@@ -103,6 +105,25 @@ foreach(varName HAS_STD_LOCALE  HAS_ODBC  HAS_MBSRTOWCS  HAS_WCSTOMBS  HAS_FWIDE
   endif()
 endforeach()
 
+#
+# Package and sign if Apache maintainer
+#
+option(APACHE_MAINTAINER "Apache maintainer" OFF)
+if(APACHE_MAINTAINER)
+set(CPACK_SOURCE_PACKAGE_FILE_NAME "apache-log4cxx-${LOG4CXX_RELEASE_VERSION}")
+set(CPACK_SOURCE_GENERATOR "TGZ;ZIP")
+set(CPACK_SOURCE_IGNORE_FILES ".git/;build/")
+include(CPack)
+
+add_custom_target( dist 
+    COMMAND ${CMAKE_COMMAND} --build ${CMAKE_BINARY_DIR} -- package_source
+    COMMAND ${CMAKE_COMMAND} -E sha512sum "apache-log4cxx-${LOG4CXX_RELEASE_VERSION}.tar.gz" > "apache-log4cxx-${LOG4CXX_RELEASE_VERSION}.tar.gz.sha512"

Review comment:
       `sha512sum` is simply a binary expected to be available using `PATH`, correct? Because in my case it isn't, but the build seems to succeed. I didn't see any error message that this command or that for `gpg` failed. Though, only the archives themself got created.
   
   Is that something to ignore or make more robust?

##########
File path: src/CMakeLists.txt
##########
@@ -13,3 +13,19 @@ if(BUILD_TESTING)
    add_subdirectory(test)
    add_subdirectory(examples/cpp)
 endif()
+
+option(BUILD_SITE "Build log4cxx website" OFF)

Review comment:
       Why is this placed here instead of the upper level `CMakeLists.txt` containing `APACHE_MAINTAINER` already? Seems to make more sense to me to have both near by each other.

##########
File path: CMakeLists.txt
##########
@@ -103,6 +105,25 @@ foreach(varName HAS_STD_LOCALE  HAS_ODBC  HAS_MBSRTOWCS  HAS_WCSTOMBS  HAS_FWIDE
   endif()
 endforeach()
 
+#
+# Package and sign if Apache maintainer
+#
+option(APACHE_MAINTAINER "Apache maintainer" OFF)
+if(APACHE_MAINTAINER)
+set(CPACK_SOURCE_PACKAGE_FILE_NAME "apache-log4cxx-${LOG4CXX_RELEASE_VERSION}")

Review comment:
       I suggest adding some indentation here to make things more readable, like you did with Doxygen 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org