-
Notifications
You must be signed in to change notification settings - Fork 580
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Windows MSI build: run light.exe with -sval in GitLab #10309
base: master
Are you sure you want to change the base?
Conversation
TestGitLab runner Windows service (not RDP logged in user!) runs these (not admin!) to build Icinga:
Before
After
Works. 👍 |
That doesn't seem to be an issue on GitHub Actions, so can we disable this just in the GitLab pipelines? I mean what it does sounds useful (from your link):
So doesn't sound like a bad idea to try to keep this where possible. |
36d0daa
to
bcdb5f9
Compare
CMakeLists.txt
Outdated
if(DEFINED ENV{GITLAB_CI}) | ||
# https://wixtoolset.org/docs/tools/validation/#errors-running-validation | ||
set(CPACK_WIX_LIGHT_EXTRA_FLAGS "-sval") | ||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That change suggests it is actually impossible to do this validation while running in the GitLab CI. However, that's not the case as we did just that for years.
This is a problem that only appeared due to planned changes to our GitLab runners, hence, that's where I'd try to adjust this. Is it possible to just pass this as -DCPACK_WIX_LIGHT_EXTRA_FLAGS=-sval
to CMake in our GitLab job definition instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That change suggests it is actually impossible to do this validation while running in the GitLab CI. However, that's not the case as we did just that for years.
... as administrator! Only that made it possible for all these things to happen under a Windows service. 😅🙈😈
This is a problem that only appeared due to planned changes to our GitLab runners, hence, that's where I'd try to adjust this.
IMAO the problem has always been in this repo. It unnecessarily enforces such privileges for GitLab runners.
Is it possible to just pass this as
-DCPACK_WIX_LIGHT_EXTRA_FLAGS=-sval
to CMake in our GitLab job definition instead?
Yes, but actually no:
- .gitlab-ci.yml (GitLab) calls build.ps1 (GitLab) (https://git.icinga.com/packaging/windows-icinga2/-/blob/6895e808d82b4983dcdaac21b3749f68355424e2/.gitlab-ci.yml#L20)
- build.ps1 (GitLab) calls tools\win32\configure.ps1 (GitHub) (https://git.icinga.com/packaging/windows-icinga2/-/blob/6895e808d82b4983dcdaac21b3749f68355424e2/build.ps1#L14)
- tools\win32\configure.ps1 calls CMake (
icinga2/tools/win32/configure.ps1
Line 59 in 866db3b
& cmake.exe "$sourcePath" `
I mean, I can put this flag and env check into tools\win32\configure.ps1 if you explain why*. But that would be in the very same repo.
*) IMAO CMake stuff belongs into CMake files, especially sane defaults which apply universally, across callers of CMake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMAO the problem has always been in this repo. It unnecessarily enforces such privileges for GitLab runners.
If this repo doesn't allow to disable this externally, that should be fixed. With your proposed change, it unnecessarily prevents performing that validation from GitLab CI altogether.
I mean, I can put this flag and env check into tools\win32\configure.ps1 if you explain why*. But that would be in the very same repo.
Why does it have to be a check on the GITLAB_CI
environment variable here? What if the following would just allow passing custom CMake options?
icinga2/tools/win32/configure.ps1
Lines 59 to 66 in 866db3b
& cmake.exe "$sourcePath" ` | |
-DCMAKE_BUILD_TYPE="$env:CMAKE_BUILD_TYPE" ` | |
-G "$env:CMAKE_GENERATOR" -A "$env:CMAKE_GENERATOR_PLATFORM" -DCPACK_GENERATOR=WIX ` | |
-DOPENSSL_ROOT_DIR="$env:OPENSSL_ROOT_DIR" ` | |
-DBOOST_LIBRARYDIR="$env:BOOST_LIBRARYDIR" ` | |
-DBOOST_INCLUDEDIR="$env:BOOST_ROOT" ` | |
-DFLEX_EXECUTABLE="$env:FLEX_BINARY" ` | |
-DBISON_EXECUTABLE="$env:BISON_BINARY" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With your proposed change, it unnecessarily prevents performing that validation from GitLab CI altogether.
Why does it have to be a check on the
GITLAB_CI
environment variable here?
It necessarily prevents it because in a sane CI infra the GitLab service is not run as admin, which makes MSI validation impossible. So what? We have GitHub actions for this, as you said.
What if the following would just allow passing custom CMake options?
icinga2/tools/win32/configure.ps1
Lines 59 to 66 in 866db3b
& cmake.exe "$sourcePath" ` -DCMAKE_BUILD_TYPE="$env:CMAKE_BUILD_TYPE" ` -G "$env:CMAKE_GENERATOR" -A "$env:CMAKE_GENERATOR_PLATFORM" -DCPACK_GENERATOR=WIX ` -DOPENSSL_ROOT_DIR="$env:OPENSSL_ROOT_DIR" ` -DBOOST_LIBRARYDIR="$env:BOOST_LIBRARYDIR" ` -DBOOST_INCLUDEDIR="$env:BOOST_ROOT" ` -DFLEX_EXECUTABLE="$env:FLEX_BINARY" ` -DBISON_EXECUTABLE="$env:BISON_BINARY"
CMake allows passing custom CMake options. This is a convenience script, its job is to fiddle together CMake options which CMake itself can't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With your proposed change, it unnecessarily prevents performing that validation from GitLab CI altogether.
Why does it have to be a check on the
GITLAB_CI
environment variable here?It necessarily prevents it because in a sane CI infra the GitLab service is not run as admin, which makes MSI validation impossible. So what? We have GitHub actions for this, as you said.
Well now I'm tempted to try whether your jobs run with admin privileges on the GitLab.com hosted Windows runners and whether you just called their infrastructure insane. 😅
What if the following would just allow passing custom CMake options?
icinga2/tools/win32/configure.ps1
Lines 59 to 66 in 866db3b
& cmake.exe "$sourcePath" ` -DCMAKE_BUILD_TYPE="$env:CMAKE_BUILD_TYPE" ` -G "$env:CMAKE_GENERATOR" -A "$env:CMAKE_GENERATOR_PLATFORM" -DCPACK_GENERATOR=WIX ` -DOPENSSL_ROOT_DIR="$env:OPENSSL_ROOT_DIR" ` -DBOOST_LIBRARYDIR="$env:BOOST_LIBRARYDIR" ` -DBOOST_INCLUDEDIR="$env:BOOST_ROOT" ` -DFLEX_EXECUTABLE="$env:FLEX_BINARY" ` -DBISON_EXECUTABLE="$env:BISON_BINARY" CMake allows passing custom CMake options. This is a convenience script, its job is to fiddle together CMake options which CMake itself can't.
I've linked that script because my question was about that script allowing additional custom CMake options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well now I'm tempted to try whether your jobs run with admin privileges on the GitLab.com hosted Windows runners and whether you just called their infrastructure insane. 😅
I talked about "the GitLab service"!
I've linked that script because my question was about that script allowing additional custom CMake options.
I don't see a point in custom ones, but I can offer you #10309 (comment) if you really care about hypothetic stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I talked about "the GitLab service"!
Why is this relevant? Aren't the build job permission what matters here?
…TION is set This is equivalent to "setting <SuppressValidation>true</SuppressValidation> in your WiX MSBuild project" not to require a CI/CD service to run as an admin. https://wixtoolset.org/docs/tools/validation/#errors-running-validation
CMakeLists.txt
Outdated
@@ -509,6 +509,11 @@ set(CPACK_WIX_PATCH_FILE "${CMAKE_CURRENT_BINARY_DIR}/icinga-installer/icinga2.w | |||
set(CPACK_WIX_EXTENSIONS "WixUtilExtension" "WixNetFxExtension") | |||
set(CPACK_WIX_INSTALL_SCOPE NONE) | |||
|
|||
if(DEFINED ENV{GITLAB_CI}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if(DEFINED ENV{GITLAB_CI}) | |
if(DEFINED ENV{ICINGA2_NO_MSI_VALIDATION}) |
Better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ICINGA2_NO_MSI_VALIDATION
is better than hard-coded GITLAB_CI
. Though that being an environment variable there looks a bit odd. None of the other ICINGA2_*
options are environment variables but rather regular CMake options. Also, with tools/win32/configure.ps1
, there's a script for mapping environment to CMake options.
So I'd rather do something like if (env:ICINGA2_NO_MSI_VALIDATION) CMAKE_OPTS+=DCPACK_WIX_LIGHT_EXTRA_FLAGS=-sval
in tools/win32/configure.ps1
, or, preferably unless that gets a complete whitespace quoting nightmare - just allow setting any options so that if additional CMake options are needed in the future, this doesn't need another pull request (+ backports) here.
bcdb5f9
to
2438339
Compare
This is equivalent to "setting true in your WiX MSBuild project" not to require the CI/CD service to run as an admin. https://wixtoolset.org/docs/tools/validation/#errors-running-validation
It's just, w/o this PR our builds also perform a "package validation" which involves the installer service. Non-admins can access the latter, but only if logged in via RDP. Services can't.