Re: [PR] MINIFICPP-2203 Add support for building Windows MSI without any redistributables included [nifi-minifi-cpp]
szaszm closed pull request #1734: MINIFICPP-2203 Add support for building Windows MSI without any redistributables included URL: https://github.com/apache/nifi-minifi-cpp/pull/1734 -- 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-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINIFICPP-2203 Add support for building Windows MSI without any redistributables included [nifi-minifi-cpp]
lordgamez commented on code in PR #1734:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1734#discussion_r1522884699
##
CMakeLists.txt:
##
@@ -493,20 +504,21 @@ if(WIN32)
list(GET VCRUNTIME_X64_MERGEMODULES 0
VCRUNTIME_X64_MERGEMODULE_PATH)
endif()
-if(CMAKE_SIZEOF_VOID_P EQUAL 8)
+if (BUILD_PLATFORM STREQUAL "x64")
message("Using ${VCRUNTIME_X64_MERGEMODULE_PATH} VC
Redistributable Merge Module")
configure_file("msi/x64.wsi" "msi/x64.wsi" @ONLY)
list(APPEND CPACK_WIX_EXTRA_SOURCES
"${CMAKE_CURRENT_BINARY_DIR}/msi/x64.wsi")
-elseif(CMAKE_SIZEOF_VOID_P EQUAL 4)
+else()
message("Using ${VCRUNTIME_X86_MERGEMODULE_PATH} VC
Redistributable Merge Module")
configure_file("msi/x86.wsi" "msi/x86.wsi" @ONLY)
list(APPEND CPACK_WIX_EXTRA_SOURCES
"${CMAKE_CURRENT_BINARY_DIR}/msi/x86.wsi")
-else()
-message(FATAL_ERROR "Could not determine architecture,
CMAKE_SIZEOF_VOID_P is unexpected: ${CMAKE_SIZEOF_VOID_P}")
endif()
-set(CPACK_WIX_TEMPLATE
"${CMAKE_CURRENT_SOURCE_DIR}/msi/WixWinMergeModules.wsi")
-else()
-message("Creating installer with redistributables")
+
+file(READ "${CMAKE_CURRENT_SOURCE_DIR}/msi/MergeModulesFeature.xml"
WIX_EXTRA_FEATURES)
+configure_file("${CMAKE_CURRENT_SOURCE_DIR}/msi/WixWin.wsi.in"
"${CMAKE_CURRENT_SOURCE_DIR}/msi/WixWin.wsi")
+set(CPACK_WIX_TEMPLATE "${CMAKE_CURRENT_SOURCE_DIR}/msi/WixWin.wsi")
Review Comment:
Good point, updated in b58c443735e7d065d37619945f6fe88e6fc534ef
--
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-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINIFICPP-2203 Add support for building Windows MSI without any redistributables included [nifi-minifi-cpp]
fgerlits commented on code in PR #1734:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1734#discussion_r1522822212
##
CMakeLists.txt:
##
@@ -493,20 +504,21 @@ if(WIN32)
list(GET VCRUNTIME_X64_MERGEMODULES 0
VCRUNTIME_X64_MERGEMODULE_PATH)
endif()
-if(CMAKE_SIZEOF_VOID_P EQUAL 8)
+if (BUILD_PLATFORM STREQUAL "x64")
message("Using ${VCRUNTIME_X64_MERGEMODULE_PATH} VC
Redistributable Merge Module")
configure_file("msi/x64.wsi" "msi/x64.wsi" @ONLY)
list(APPEND CPACK_WIX_EXTRA_SOURCES
"${CMAKE_CURRENT_BINARY_DIR}/msi/x64.wsi")
-elseif(CMAKE_SIZEOF_VOID_P EQUAL 4)
+else()
message("Using ${VCRUNTIME_X86_MERGEMODULE_PATH} VC
Redistributable Merge Module")
configure_file("msi/x86.wsi" "msi/x86.wsi" @ONLY)
list(APPEND CPACK_WIX_EXTRA_SOURCES
"${CMAKE_CURRENT_BINARY_DIR}/msi/x86.wsi")
-else()
-message(FATAL_ERROR "Could not determine architecture,
CMAKE_SIZEOF_VOID_P is unexpected: ${CMAKE_SIZEOF_VOID_P}")
endif()
-set(CPACK_WIX_TEMPLATE
"${CMAKE_CURRENT_SOURCE_DIR}/msi/WixWinMergeModules.wsi")
-else()
-message("Creating installer with redistributables")
+
+file(READ "${CMAKE_CURRENT_SOURCE_DIR}/msi/MergeModulesFeature.xml"
WIX_EXTRA_FEATURES)
+configure_file("${CMAKE_CURRENT_SOURCE_DIR}/msi/WixWin.wsi.in"
"${CMAKE_CURRENT_SOURCE_DIR}/msi/WixWin.wsi")
+set(CPACK_WIX_TEMPLATE "${CMAKE_CURRENT_SOURCE_DIR}/msi/WixWin.wsi")
Review Comment:
these two lines are the same in every branch, so they could be moved to
after the `if` block
--
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-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINIFICPP-2203 Add support for building Windows MSI without any redistributables included [nifi-minifi-cpp]
lordgamez commented on code in PR #1734:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1734#discussion_r1519529421
##
cmake/MiNiFiOptions.cmake:
##
@@ -82,6 +80,8 @@ list(APPEND STRICT_GSL_CHECKS_Values AUDIT ON DEBUG_ONLY OFF)
set_property(CACHE STRICT_GSL_CHECKS PROPERTY STRINGS
${STRICT_GSL_CHECKS_Values})
if (WIN32)
+add_minifi_option(INSTALLER_MERGE_MODULES "Creates installer with merge
modules" OFF)
+add_minifi_option(INSTALLER_WITH_VC_REDISTRIBUTABLES "Creates installer
with Visual C++ redistributables included" OFF)
Review Comment:
Updated the flag names and descriptions in
b2da5ed65397525433093fa654edfe0d7836ca13 also making them mutually exclusive
##
cmake/MiNiFiOptions.cmake:
##
@@ -82,6 +80,8 @@ list(APPEND STRICT_GSL_CHECKS_Values AUDIT ON DEBUG_ONLY OFF)
set_property(CACHE STRICT_GSL_CHECKS PROPERTY STRINGS
${STRICT_GSL_CHECKS_Values})
if (WIN32)
+add_minifi_option(INSTALLER_MERGE_MODULES "Creates installer with merge
modules" OFF)
+add_minifi_option(INSTALLER_WITH_VC_REDISTRIBUTABLES "Creates installer
with Visual C++ redistributables included" OFF)
Review Comment:
Updated in b2da5ed65397525433093fa654edfe0d7836ca13
--
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-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINIFICPP-2203 Add support for building Windows MSI without any redistributables included [nifi-minifi-cpp]
szaszm commented on code in PR #1734:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1734#discussion_r1517981895
##
cmake/MiNiFiOptions.cmake:
##
@@ -82,6 +80,8 @@ list(APPEND STRICT_GSL_CHECKS_Values AUDIT ON DEBUG_ONLY OFF)
set_property(CACHE STRICT_GSL_CHECKS PROPERTY STRINGS
${STRICT_GSL_CHECKS_Values})
if (WIN32)
+add_minifi_option(INSTALLER_MERGE_MODULES "Creates installer with merge
modules" OFF)
+add_minifi_option(INSTALLER_WITH_VC_REDISTRIBUTABLES "Creates installer
with Visual C++ redistributables included" OFF)
Review Comment:
This sounds good to me
--
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-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINIFICPP-2203 Add support for building Windows MSI without any redistributables included [nifi-minifi-cpp]
lordgamez commented on code in PR #1734:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1734#discussion_r1509271721
##
cmake/MiNiFiOptions.cmake:
##
@@ -82,6 +80,8 @@ list(APPEND STRICT_GSL_CHECKS_Values AUDIT ON DEBUG_ONLY OFF)
set_property(CACHE STRICT_GSL_CHECKS PROPERTY STRINGS
${STRICT_GSL_CHECKS_Values})
if (WIN32)
+add_minifi_option(INSTALLER_MERGE_MODULES "Creates installer with merge
modules" OFF)
+add_minifi_option(INSTALLER_WITH_VC_REDISTRIBUTABLES "Creates installer
with Visual C++ redistributables included" OFF)
Review Comment:
I agree the naming should be changed, to be more clear, and clarify which
option bundles what artifacts exactly. It also needs to be specified if a
bundle falls under a different license.
On the other hand I would keep all three bundle options separate. The
rationale behind this version is it's easier and more direct to specify a
single option for each installer bundle option instead of combining build
options like `INCLUDE_MS_BLOBS` + `USE_UCRT_DLLS`. So I would keep them
separately like `INCLUDE_UCRT_DLLS`, `INCLUDE_VC_REDIST_DLLS`, and
`INCLUDE_VC_REDIST_MERGE_MODULES`. This way you can directly specify which
libraries you want to include in the installer, and by not specifying anything,
no Microsoft blobs will be included. What are your thoughts on this?
--
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-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINIFICPP-2203 Add support for building Windows MSI without any redistributables included [nifi-minifi-cpp]
martinzink commented on code in PR #1734:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1734#discussion_r1513120771
##
cmake/MiNiFiOptions.cmake:
##
@@ -82,6 +80,8 @@ list(APPEND STRICT_GSL_CHECKS_Values AUDIT ON DEBUG_ONLY OFF)
set_property(CACHE STRICT_GSL_CHECKS PROPERTY STRINGS
${STRICT_GSL_CHECKS_Values})
if (WIN32)
+add_minifi_option(INSTALLER_MERGE_MODULES "Creates installer with merge
modules" OFF)
+add_minifi_option(INSTALLER_WITH_VC_REDISTRIBUTABLES "Creates installer
with Visual C++ redistributables included" OFF)
Review Comment:
Could we add the MINIFI_ prefix to these new options?
(In my opinion we should add that to every option thats ours so it doesnt
conflict with thirdparty options and it makes it easier to filter out
thirdparty related options (e.g. python bootstrap).)
So adding it to new flags should be good way to start this transition.
--
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-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINIFICPP-2203 Add support for building Windows MSI without any redistributables included [nifi-minifi-cpp]
lordgamez commented on code in PR #1734:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1734#discussion_r1509271721
##
cmake/MiNiFiOptions.cmake:
##
@@ -82,6 +80,8 @@ list(APPEND STRICT_GSL_CHECKS_Values AUDIT ON DEBUG_ONLY OFF)
set_property(CACHE STRICT_GSL_CHECKS PROPERTY STRINGS
${STRICT_GSL_CHECKS_Values})
if (WIN32)
+add_minifi_option(INSTALLER_MERGE_MODULES "Creates installer with merge
modules" OFF)
+add_minifi_option(INSTALLER_WITH_VC_REDISTRIBUTABLES "Creates installer
with Visual C++ redistributables included" OFF)
Review Comment:
I agree the naming should be changed, to be more clear, and clarify which
option bundles what artifacts exactly. It also needs to be specified if a
bundle falls under a different license.
On the other hand I would keep all three bundle options separate. The
rationale behind this version is it's easier and more direct to specify a
single option for each installer bundle option instead of combining build
options like `INCLUDE_MS_BLOBS` + `USE_UCRT_DLLS`. So I would keep them
separately like `INCLUDE_UCRT_IN_INSTALLER`, `INCLUDE_VC_REDIST_DLLS`, and
`INCLUDE_VC_REDIST_MERGE_MODULES`. This way you can directly specify which
libraries you want to include in the installer, and by not specifying anything,
no Microsoft blobs will be included. What are your thoughts on this?
--
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-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINIFICPP-2203 Add support for building Windows MSI without any redistributables included [nifi-minifi-cpp]
szaszm commented on code in PR #1734:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1734#discussion_r1509232446
##
cmake/MiNiFiOptions.cmake:
##
@@ -82,6 +80,8 @@ list(APPEND STRICT_GSL_CHECKS_Values AUDIT ON DEBUG_ONLY OFF)
set_property(CACHE STRICT_GSL_CHECKS PROPERTY STRINGS
${STRICT_GSL_CHECKS_Values})
if (WIN32)
+add_minifi_option(INSTALLER_MERGE_MODULES "Creates installer with merge
modules" OFF)
+add_minifi_option(INSTALLER_WITH_VC_REDISTRIBUTABLES "Creates installer
with Visual C++ redistributables included" OFF)
Review Comment:
The naming is confusing, and not specific enough. Merge modules and bundled
DLLs are both Visual C++ redistributable runtimes (runtime meaning library or
DLL in this case), they're just different ways of installing the same DLLs of
the C and C++ standard libraries. Under the old version, if the user didn't ask
for merge modules, they got bundled DLLs.
There is also the Universal C Runtime (UCRT) DLLs that are not part of the
same Visual C++ redistributable runtimes package, but they are also standard
library DLLs, they also fall under the same license terms, and are not
redistributable under the Apache License. If someone wants to NOT include the
Visual C++ redistributable runtime, then they probably want a package free of
Microsoft blobs, and they don't want to include the UCRT either.
The previous version was fine, as it treated both kinds of redistributables
under the same banner, disabling them altogether. This new version seems to
treat the Visual C++ redistributable runtime package separately from the UCRT,
and I don't see the rationale behind this.
--
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-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINIFICPP-2203 Add support for building Windows MSI without any redistributables included [nifi-minifi-cpp]
szaszm commented on code in PR #1734: URL: https://github.com/apache/nifi-minifi-cpp/pull/1734#discussion_r1505905580 ## cmake/MiNiFiOptions.cmake: ## Review Comment: What happens if all of these remain off? I'd switch to no redistributables by default, unless the user asks for merge modules or redist DLLs. -- 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-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] MINIFICPP-2203 Add support for building Windows MSI without any redistributables included [nifi-minifi-cpp]
lordgamez opened a new pull request, #1734: URL: https://github.com/apache/nifi-minifi-cpp/pull/1734 - Also remove unused FORCE_WINDOWS build option -- Thank you for submitting a contribution to Apache NiFi - MiNiFi C++. In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: ### For all changes: - [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [ ] Does your PR title start with MINIFICPP- where is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character. - [ ] Has your PR been rebased against the latest commit within the target branch (typically main)? - [ ] Is your initial contribution a single, squashed commit? ### For code changes: - [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? - [ ] If applicable, have you updated the LICENSE file? - [ ] If applicable, have you updated the NOTICE file? ### For documentation related changes: - [ ] Have you ensured that format looks appropriate for the output in which it is rendered? ### Note: Please ensure that once the PR is submitted, you check GitHub Actions CI results for build issues and submit an update to your PR as soon as possible. -- 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-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
