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:
[email protected]


Reply via email to