Skip to content

Modernize the Windows MSI: WiX v5, x64, service registration, upgrade from 5.1.1 - #701

Open
vharseko wants to merge 39 commits into
OpenIdentityPlatform:masterfrom
vharseko:features/windows-distribution-new
Open

Modernize the Windows MSI: WiX v5, x64, service registration, upgrade from 5.1.1#701
vharseko wants to merge 39 commits into
OpenIdentityPlatform:masterfrom
vharseko:features/windows-distribution-new

Conversation

@vharseko

@vharseko vharseko commented Jul 4, 2026

Copy link
Copy Markdown
Member

Modernizes the Windows MSI end to end. Builds on #664 (branched from it) and includes the Windows script quoting fixes from #671 (they merge cleanly once #671 lands).

Toolchain: WiX 3.11 + wine → WiX v5, Windows-native

  • WiX 3.11.1 (2017, EOL) → WiX v5.0.2 (.NET wix tool): heat/candle/light + wine + winetricks + dotnet40 collapse into a single wix build -arch x64. WiX v5 is the newest version without the Open Source Maintenance Fee (v7 refuses to run without accepting the OSMF EULA).
  • The MSI is built on Windows only. Verified empirically (wix 5.0.2/6.0.1/7.0.0 on macOS): non-Windows builds fail on WIX0389 for any Directory/@Name and, with no names at all, on the missing msi.dll — the MSI database is written by the Windows Installer library, so no cross-platform build exists. wine is removed from build.yml, deploy.yml and release.yml.
  • package.wxs rewritten to the v4+ schema: single <Package>, MediaTemplate CompressionLevel="high", InstallerVersion=500, <Files> harvest from Ant-staged payloads, empty instance directories shipped via CreateFolder components (the harvest ships files only).
  • The opendj-msi module is pulled in by a distribution-windows-msi profile that activates only when the wix .NET tool is installed, so a plain mvn install on a contributor's Windows machine no longer fails mid-reactor.

Installer behavior

  • x64: installs into C:\Program Files\OpenDJ (was x86 / Program Files (x86)).
  • The MSI registers the OpenDJ Windows service (key = display name; ServiceInstall on opendj_service.exe, not started during install — run setup first; a failed registration fails the install). The display name deliberately equals the key so windows-service.bat --enableService in additional zip instances can still register OpenDJ Server/OpenDJ Server-2. When an existing OpenDJ is detected (upgrade or detected location — a fresh install on a host with an unrelated zip instance leaves it alone), a running legacy service is stopped before RemoveExistingProducts and removed via MSI's own ServiceControl. windows-service.bat --disableService, --cleanupService and uninstall.bat all treat the MSI-managed service as an informational skip (dedicated wrapper return code), so msiexec /x always finds it; repair does not stop a running server (Stop="uninstall"). The MSI also registers the event-log source for the service.
  • Upgrade over an existing install: the installer detects the location recorded in the registry by this package, or the legacy x86 default directory (two separate searches with explicit precedence: registry wins), and installs there — instance data (config, db, logs) is preserved; an explicit OPENDJ=... always wins, and a silent upgrade whose location cannot be determined (5.1.x wrote no registry value) refuses with guidance instead of relocating to the default. Verified in CI by upgrading from the released 5.1.2 MSI at its real x86 default with no OPENDJ given. AllowSameVersionUpgrades keeps a same-version hotfix re-release from producing two ARP entries.
  • Java: the installer does not check for or install a JRE (a JAVA_HOME/PATH launch condition was tried and dropped — it blocked upgrades of working servers and never consulted OPENDJ_JAVA_HOME, see the review discussion and Add CI install-test for the Windows MSI + document MSI install/upgrade/uninstall #664). setup and the server require Java 11+; the install guide points at https://adoptium.net. The MSI is not code-signed (no certificate) — SmartScreen note added to the docs.

Release / deploy

  • deploy.yml reuses the MSI already built by the triggering Build run (no rebuild).
  • release.yml: new release-msi job (windows, continue-on-error) installs the released server zip into the local repository and packages only :opendj-msi-standard, then attaches the MSI to the GitHub release.
  • opendj-msi-standard is intentionally no longer published to Maven Central: it was previously wine-built on Linux and deployed from the ubuntu deploy jobs; it is now built natively on Windows and distributed through GitHub Releases and the Package/Deploy OpenDJ MSI Package artifact. The Maven coordinate stays at 5.1.2 as its last published version.

CI

  • test-msi: installs into the spaced x64 default directory, asserts the service ImagePath is quoted (CWE-428), runs setup, starts/stops the MSI-registered service, ldapsearch, asserts --disableService leaves the MSI-managed service in place, runs uninstall.bat --cli (must delete the instance files and leave the service to msiexec /x), uninstalls.
  • test-msi-upgrade: released 5.1.2 x86 MSI at its native Program Files (x86)\OpenDJ default → configure → legacy service → upgrade with the new x64 MSI with no OPENDJ given: same directory kept, data intact, legacy service replaced by OpenDJ, upgrade.bat, start, search; plus fresh-install scenarios asserting the legacy default directory is auto-detected, the registry-recorded location wins over it, and a silent upgrade from an undetectable custom directory refuses with guidance.
  • The Windows job compares the committed launcher binaries under opendj-server-legacy/lib with what it just built from source (/Brepro makes the build reproducible) and emits a loud warning on drift — warning-only, so an MSVC toolchain bump on the runner image cannot take down the artifact consumers (test-msi*, deploy.yml); nmake/xcopy failures now fail the step.

Windows script fixes surfaced by these tests (in #671)

Install paths with spaces/parentheses (C:\Program Files (x86)\OpenDJ — the old default!) broke the batch scripts in four places: unquoted java.io.tmpdir, setcp.bat argument parsing, "%VAR%" == "" checks with the now-quoted tmpdir, and the start-ds.bat tmp-cleanup block. All fixed and exercised by the MSI tests in this PR.

Deferred

  • Authenticode signing — no certificate.
  • A Burn bundle auto-installing the JRE — dropped; the installer neither bundles nor checks for a JRE (see above), the docs carry the download link.

The launcher binaries under opendj-server-legacy/lib are pending a refresh from the windows-exe-11 artifact of the next Build run (service.c changed in the second review round); until then the binaries-match-sources check reports a warning. The GUI wizard path (license-dialog skip, InstallDirDlg pre-population) is not covered by CI — one manual run before merge is planned.

vharseko added 22 commits June 26, 2026 20:37
The opendj-msi package was built and uploaded but never installed or
exercised in CI. Add a test-msi job (needs: build-maven) that, on a
windows-latest runner, installs the built .msi silently (msiexec /i),
runs setup, registers and starts/stops the OpenDJ Windows service with
an ldapsearch liveness check, then uninstalls (msiexec /x).
…E 25)

The MSI ships no JRE, so add a WiX launch condition that fails the install
early with a clear message when Java is not detected (it does not install
Java). JAVA_HOME is captured from the environment before LaunchConditions;
`Installed` keeps uninstall/repair working regardless of Java.

Fix the test-msi CI job (it failed with `net start` exit 2 "service already
started"):
- setup.bat is now invoked with --doNotStart, so the server is started only by
  `net start "OpenDJ Server"` (setup.bat no longer starts a standalone instance
  first).
- Bump actions/setup-java from 21 to 25 (latest LTS; smoke-tests the MSI under a
  fresh JRE). The runner has JAVA_HOME from setup-java, so the new launch
  condition is satisfied and the install proceeds.
The launch condition (Installed OR JAVA_HOME_ENV) false-blocked valid installs: a JRE does not always set JAVA_HOME (it may be only on PATH). Drop it - the MSI again only copies files and Java availability stays the admin's responsibility. The test-msi fix (--doNotStart, JRE 25) is unaffected.
The install guide covered only the .zip and native .deb/.rpm. Add Windows MSI sections to chap-install/chap-upgrade/chap-uninstall: GUI and silent msiexec install, Java as a runtime prerequisite the installer does not enforce, configure via setup.bat, optional Windows service registration via windows-service.bat, MSI upgrade (disable service, back up, install newer .msi, upgrade.bat, re-enable), and uninstall via Apps & features / msiexec /x.
Migrate the Windows MSI off the EOL WiX 3.11.1 (heat/candle/light run under
wine + winetricks dotnet40) to the cross-platform WiX v5 .NET tool, and
address the MSI validation findings:

- package.wxs rewritten to the WiX v4+ schema: single <Package>, x64
  (ProgramFiles64Folder), MediaTemplate CompressionLevel="high",
  InstallerVersion=500, <Files> directory harvest (replaces heat), and a
  <ServiceInstall>/<ServiceControl> on opendj_service.exe so the MSI itself
  registers the "OpenDJ Server" Windows service (Start=auto, not started during
  install - setup must configure the instance first). UpgradeCode preserved.
- opendj-msi-standard/pom.xml builds the MSI with `wix build -arch x64`; removed
  the external-dependency-plugin (wix3111 + winetricks), heat/candle/light and
  the wine/winetricks antrun steps.
- opendj-msi/pom.xml: wine-path profiles replaced by an unconditional module so
  the MSI builds natively on every OS where opendj-msi is included.
- build.yml: drop wine (keep rpm); add a "Setup WiX (.NET tool)" step; test-msi
  no longer calls windows-service.bat --enableService (the MSI registers it).
- install guide: x64 install path; the MSI registers the service.

Signing (OpenIdentityPlatform#4) deferred (no certificate). The Burn bundle that auto-installs a JRE
is a follow-up, to be added after this MSI build is validated in CI.
WiX 5 ships as a net6.0 .NET tool, but the runners only have a newer .NET runtime (no .NET 6), so 'wix' failed to launch (exit 131, missing_runtime) on arm64 macOS. Set DOTNET_ROLL_FORWARD=Major in the Setup WiX step (exported and written to GITHUB_ENV) so the tool rolls forward to the installed runtime, both for that step and the Maven build that invokes 'wix build'.
wix build failed with WIX0004 ('Files element contains an unexpected attribute Exclude'): in WiX 5.0.2 exclusion is a child <Exclude Path=.../> element (with <Include Path=.../>), not an inline attribute. Convert both <Files> harvests to the child-element form.

macOS (arm64) runners have no .NET runtime for the net6.0 wix apphost (DOTNET_ROOT unset, exit 131); add actions/setup-dotnet (8.0.x) on macOS so the tool finds a runtime and rolls forward.
…only Include)

WiX 5.0.2 requires Include as an attribute on <Files> and rejects an Exclude attribute as well as <Include>/<Exclude Path> child elements (WIX0004/0005/0010). Move all exclusions to the Ant staging step: stagingRoot = payload minus lib (and macOS/Unix bits), stagingLib = lib minus opendj_service.exe (registered via an explicit component sourced from the package dir). Each <Files> now uses only the Include attribute, and there is no duplicate lib directory.
On Linux/macOS the wix tool treats backslash as a literal, not a path separator, which may cascade into the WIX0389 'not a relative path' errors. Use '/**' (accepted on Windows too) to test whether the cross-platform wix build then succeeds.
The WiX Toolset can create MSI databases on Windows only: it warns "only
supports Windows" and, on Linux/macOS, fails with WIX0389 on every Directory
name and (with no name) on a missing msi.dll (the Windows Installer library).
Verified locally on macOS with WiX 5.0.2, 6.0.1 and 7.0.0 - none can build.

- opendj-packages/pom.xml: build opendj-msi only in the distribution-windows
  profile (removed from distribution-unix and distribution-mac).
- build.yml: run the WiX setup step only on Windows; drop macOS setup-dotnet.
- package.wxs: keep the Windows-proven backslash glob in <Files Include>.
- .gitattributes: force LF for *.wxs/*.wxi.
The MSI can only be built on Windows. Instead of rebuilding opendj-server-legacy
on a Windows runner:

- deploy.yml: reuse the MSI already built by the triggering Build run (download
  the windows-latest-11 artifact, re-upload as "OpenDJ MSI Package"); both steps
  continue-on-error; drop wine (rpm only).
- release.yml: release-maven uploads the released server zip as an artifact;
  new release-msi job (windows, continue-on-error so an MSI failure does not
  break the release) installs the zip into the local Maven repository and only
  packages :opendj-msi-standard (no -am), then attaches the MSI to the GitHub
  release; drop the .msi from the ubuntu release file list; drop wine.
…e legacy service

- package.wxs: refuse to install when no Java is detectable (JAVA_HOME\bin\java.exe
  or java-looking PATH entries; MSI cannot scan PATH, so substring heuristic) with
  a message pointing at https://adoptium.net; detect an existing install (InstallDir
  registry value written by this package, else the legacy x86 default directory)
  and apply it only when OPENDJ is not set explicitly; drop the legacy
  windows-service.bat "OpenDJ Server" service before InstallServices (its display
  name collides with the MSI ServiceInstall).
- build.yml: test-msi asserts the installer fails without a JRE before one is set
  up; new test-win-upgrade installs the released 5.1.1 x86 MSI, configures an
  instance with the legacy service, upgrades with the newly built x64 MSI without
  OPENDJ and asserts same directory, intact data, MSI-managed service, then runs
  upgrade.bat, starts the service and searches the pre-upgrade data.
- docs: Java is required by the installer (adoptium.net link); MSI upgrade
  autodetects the directory and replaces the service; uninstall removes the
  service automatically.
- _script-util.bat: quote -Djava.io.tmpdir="%OPENDJ_TMP_DIR%" - any install
  directory containing spaces (including the x64 default C:\Program Files\OpenDJ
  and the legacy Program Files (x86)) broke the CheckJVMVersion probe and every
  script with "The detected Java version could not be used".
- build.yml test-msi: the no-JRE negative test now hides Java at MACHINE scope
  (the Windows Installer service evaluates launch conditions with the machine
  environment) and restarts msiserver, restoring everything afterwards; the
  positive install now goes to the spaced x64 default directory end-to-end.
- build.yml test-msi-upgrade (renamed from test-win-upgrade): the released 5.1.1
  scripts cannot run from a spaced directory, so the old install lives in
  C:\opendj and the upgrade passes OPENDJ explicitly; added a fresh-install
  scenario asserting the legacy default directory is auto-detected when OPENDJ
  is not given, plus uninstall/service cleanup checks.
- ADNotificationRequestControl: escape && in the javadoc code sample
  (javadoc "invalid input: '&'" warnings).
…aces)

_script-util.bat appends -Djava.io.tmpdir=%OPENDJ_TMP_DIR% to OPENDJ_JAVA_ARGS
without quotes, so in any install directory containing spaces (for example
C:\Program Files (x86)\OpenDJ) the CheckJVMVersion probe - and with it setup
and every other command-line tool - fails with "The detected Java version
could not be used with the set of Java arguments".

Quote the value: -Djava.io.tmpdir="%OPENDJ_TMP_DIR%".
…cp.bat)

_script-util.bat passed unquoted paths to setcp.bat, and setcp.bat compared
arguments with if ""%1""=="""". The argument-joining hack survived spaces, but
a parenthesis in the path (C:\Program Files (x86)\OpenDJ - the default MSI
directory) breaks the cmd parser with "... was unexpected at this time", so
setup.bat and every tool exit with 255.

Quote the setcp.bat arguments at the three call sites and switch setcp.bat to
%~1 with quoted comparisons.
…e "%VAR%" == "" comparisons

After -Djava.io.tmpdir="%OPENDJ_TMP_DIR%" is appended, OPENDJ_JAVA_ARGS
contains embedded quotes, and the subsequent if "%OPENDJ_JAVA_ARGS%" == ""
checks blow up the cmd parser ('...\tmp"" was unexpected at this time', every
tool exits 255) regardless of whether the install path has spaces. Compare
with "if defined", which does not expand the value.
…h quoting

- The <Files> harvest ships files only (heat had -ke), so the empty instance
  directories (bak, changelogDb, classes, db, import-tmp, ldif, locks, logs,
  tmp and the template ones) were missing from the installed tree and the
  server could not create its lock/pid files - setup aborted with "error
  stopping server". Create them with explicit CreateFolder components.
- [OPENDJ] ends with a backslash which escaped the closing quote in the
  service ImagePath ('start "C:\opendj\"' -> broken argv), so the MSI-managed
  service failed to start (NET HELPMSG 2186). Append an extra backslash so
  \\" parses as backslash + closing quote.
Components whose KeyPath is a directory cannot use auto-generated GUIDs, so the empty-instance-directory components broke wix build with WIX0230. Assign fixed GUIDs.
The tmp-cleanup block expands %OPENDJ_TMP_DIR% unquoted inside a ( ) compound statement, so a parenthesis in the install path - C:\Program Files (x86)\OpenDJ - terminates the block at parse time and start-ds fails; the Windows service then dies with a service-specific error -1 while setup (which never runs this block) succeeds. Quote the three path expansions.
opendj_service.exe located its own service entry by comparing the raw
SCM ImagePath with the exact string it builds itself, so a service
registered by the MSI (exe unquoted, instance dir with a trailing
backslash) was never found and "net start" failed with error 2186.
getServiceName now compares the executable path, the subcommand and the
normalized instance dir token by token instead. The MSI ServiceInstall
argument becomes 'start "[OPENDJ]."' and the sc-config workaround is
not needed.
@vharseko vharseko added the packaging deb/rpm/MSI, distribution layout, config.ldif label Jul 6, 2026
@vharseko vharseko added the CI label Jul 6, 2026
vharseko added 2 commits July 6, 2026 18:55
Resolve build.yml conflict: keep master's build-docker-alpine benchmark
steps and append the test-msi and test-msi-upgrade CI jobs. The build-maven
matrix and WiX setup step merged automatically.
Replace opendj_service.exe, winlauncher.exe and launcher_administrator.exe
with the CI-built artifacts from the fixed service.c (tolerant ImagePath
matching, commit 262f98f). Verified by green test-msi and
test-msi-upgrade — the Windows service now starts (no more error 2186).
…stribution-new

# Conflicts:
#	.gitattributes
#	.github/workflows/build.yml
#	opendj-core/src/main/java/org/forgerock/opendj/ldap/controls/ADNotificationRequestControl.java
#	opendj-doc-generated-ref/src/main/asciidoc/install-guide/chap-install.adoc
#	opendj-doc-generated-ref/src/main/asciidoc/install-guide/chap-uninstall.adoc
#	opendj-doc-generated-ref/src/main/asciidoc/install-guide/chap-upgrade.adoc
Comment thread .github/workflows/build.yml Fixed
Comment thread .github/workflows/release.yml Fixed
@vharseko
vharseko requested a review from maximthomas August 6, 2026 10:49

@maximthomas maximthomas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The WiX v5 / x64 / service-registration rework is the right direction and the CI you added is far beyond what the old MSI had. service.c is clean (I checked the three new functions for bounds, leaks and legacy-ImagePath compatibility — nothing), and I verified the committed opendj_service.exe really does contain normalizeInstanceDir.

My earlier approval predates 11 commits; please treat this as replacing it. Blocking items below are mostly one- to few-line fixes.

release-msi cannot attach the MSI — missing contents: write (blocker)

.github/workflows/release.yml:33 sets permissions: contents: read at workflow level. release-maven (:41) and release-docker (:225) override it; release-msi (:163) does not, so softprops/action-gh-release (:213) 403s. A permissive repo default cannot rescue this — the workflow-level block is applied after it.

  release-msi:
    runs-on: 'windows-latest'
    continue-on-error: true
+   permissions:
+     contents: write

There are now three independent ways the MSI silently vanishes from a release, all new here: this 403; the job's continue-on-error: true (which also defeats fail_on_unmatched_files: true — job fails, workflow run still concludes success); and release.yml:94-100 uploading release-server-zip with continue-on-error + default if-no-files-found: warn. Meanwhile the MSI was dropped from the main Release on GitHub file list (:110). Worth one deliberate decision about which of these should actually be fatal.

deploy.yml cannot download the MSI — missing actions: read (blocker)

.github/workflows/deploy.yml:29 declares permissions: contents: write. A permissions: block sets every unlisted scope to none, and contents does not imply actions. Cross-run download needs actions: read:

 permissions:
   contents: write
+  actions: read

On a public repo the artifact list call is exempt, but GET /actions/artifacts/{id}/zip is not — so the download itself fails. With continue-on-error on both steps and upload-artifact's default warn, no artifact is created at all and Package/Deploy stays fully green.

opendj-msi-standard disappears from Maven Central (blocker, unless intended)

opendj-packages/pom.xml:53-59,69-72 removes opendj-msi from distribution-unix and distribution-mac. Both mvn deploy jobs run on ubuntu-latest, so the module leaves the reactor there. release-msi runs package only — it never deploys.

This is a live coordinate, not a theoretical one: Central lists 38 versions, 4.4.12 → 5.1.2, and opendj-msi-standard-5.1.2.msi is a real 90 MB MSI. Pre-PR it built fine on Linux — the module was gated on <file><exists>/usr/bin/wine</exists></file> and the workflows installed winehq. The parent opendj-msi pom artifact stops publishing too.

If dropping the Maven coordinate is intentional, please say so in the PR description; otherwise release-msi needs a deploy step.

RemoveLegacyService: SYSTEM code execution, wrong-instance deletion, silent no-service installs (high)

opendj-packages/opendj-msi/opendj-msi-standard/resources/msi/package.wxs:64-68:

<CustomAction Id="RemoveLegacyService" Directory="OPENDJ"
              ExeCommand="cmd.exe /c (net stop &quot;OpenDJ Server&quot; &amp; sc delete &quot;OpenDJ Server&quot;)"
              Execute="deferred" Impersonate="no" Return="ignore"/>

Three problems in one action:

  1. LPE to SYSTEM. A type-34 CA runs with [OPENDJ] as its working directory. cmd.exe itself is safe (it binds from System32, the calling process's image dir), but NeedCurrentDirectoryForExePath returns TRUE by default, so cmd resolves net and sc from .;%PATH%. being [OPENDJ]. Any install outside Program Files — e.g. OPENDJ=C:\opendj, which your own build.yml uses — sits in a directory where BUILTIN\Users can add files (the C:\ root DACL grants AddSubDir/AddFile, and a user-created dir makes them CREATOR OWNER). A planted C:\opendj\net.com executes as LocalSystem the next time an admin installs or upgrades. (The MSI SDK also requires the Target column to be "the full path and name to the executable file", so the unqualified names are already out of contract.)
  2. Wrong instance. createServiceName produces OpenDJ Server, OpenDJ Server-2, … Upgrading instance B (OPENDJ=C:\opendj-B, service OpenDJ Server-2) runs sc delete "OpenDJ Server" and destroys instance A's service. There is no rollback action, so a later failure does not restore it.
  3. Silent no-service install. sc delete on a service with an open handle only marks it for deletion; the following CreateService then fails with ERROR_DUPLICATE_SERVICE_NAME. Vital="no" + Return="ignore" means the MSI reports success with no service registered.

All three go away by deleting the custom action and using MSI's own mechanism — ServiceControl/@Name explicitly "can be used to control a service that is not installed", and it runs at StopServices/DeleteServices (1900/2000) instead of 5799:

<ServiceControl Id="RemoveLegacyOpenDJServer" Name="OpenDJ Server"
                Stop="install" Remove="install" Wait="yes"/>

Separately: the legacy display name is not a constant. ConfigureWindowsService.java:321 resolves it through Utils.getCustomizedObject, and INFO_WINDOWS_SERVICE_NAME_825 is literally OpenDS in tool_ko.properties:644 and tool_zh_TW.properties:644 — so on those locales the hardcoded string matches nothing and the collision this action exists to prevent still happens.

The GUI installer shows a Lorem ipsum EULA (high)

package.wxs:166-167:

<!-- WixUI_InstallDir; the license is shown only by setup, not by this installer. -->
<ui:WixUI Id="WixUI_InstallDir" InstallDirectory="OPENDJ"/>

The comment is no longer true. The old wxs skipped the dialog with <Publish Dialog="WelcomeDlg" Control="Next" Event="NewDialog" Value="InstallDirDlg">NOT Installed</Publish>; that went with the rest of the <UI> block. Stock WixUI_InstallDir is Welcome → LicenseAgreement → InstallDir → VerifyReady, and LicenseAgreementDlg binds !(wix.WixUILicenseRtf=$(var.licenseRtf)) — a build-time default, so omitting the variable is not an error. It falls back to WiX's own License.rtf, whose body is Lorem ipsum dolor sit amet…, and the Next button's DisableCondition makes accepting it mandatory. Only silent installs run in CI, so nothing catches it.

Either set WixUILicenseRtf to the real CDDL text, or re-add the <Publish> override.

The Java launch condition blocks supported installs — and upgrades (high)

package.wxs:58. _script-util.bat:113-155 resolves Java in this order:

OPENDJ_JAVA_BIN → OPENDJ_JAVA_HOME → <script>.java-home → default.java-home
  → java.exe on PATH → JAVA_BIN → JAVA_HOME

The condition checks only the last entry, plus a substring guess at the fifth. Consequences:

  • OPENDJ_JAVA_HOME — the variable the product's own "no Java found" message tells users to set (_script-util.bat:160) — is never consulted.
  • default.java-home in config/java.properties is how a configured instance pins its JVM. On a major upgrade Installed is false, so a working OpenDJ with no machine-scope JAVA_HOME cannot be upgraded at all — that breaks the PR's headline feature.
  • LaunchConditions is in both sequence tables, so an interactive user with a user-scoped JAVA_HOME clicks through the entire wizard and is refused at the end (the Execute sequence runs as LocalSystem and sees only the machine environment).
  • Path="[%JAVA_HOME]" has a null DrLocator parent, so when JAVA_HOME is unset — precisely the case this search exists for — MSI scans every fixed drive (bounded by Depth="0").
  • The ~>< substring test passes on C:\Users\javier\bin or any JavaScript toolchain path.

#664 records that a JAVA_HOME condition was added and reverted before for this reason. A RegistrySearch on HKLM\SOFTWARE\JavaSoft\* / Adoptium keys, plus OPENDJ_JAVA_HOME/OPENDJ_JAVA_BIN, is the signal you want — not %ENV.

Service registered Start="auto" before the instance exists (medium-high)

package.wxs:134-137. Nothing happens at install time (correct — ServiceControl has no Start event). But Start="auto" is SERVICE_AUTO_START, so at the next boot the SCM runs opendj_service.exebat\start-ds.bat against a tree with no config\config.ldif (the package ships template/config/** only). With ErrorControl="normal" that logs an error on every boot until someone runs setup. Start="demand", promoted to auto by setup/windows-service.bat, is the right shape.

windows-service.bat --disableService now deletes the MSI-managed service (medium-high)

Before this PR getServiceName() could not match the MSI ImagePath, so --disableService left the MSI service alone. The new serviceCmdsMatch makes that lookup succeed — so --disableService now DeleteServices key OpenDJ, and a following --enableService recreates it as key=display=OpenDJ Server. msiexec /x then targets key OpenDJ, which no longer exists, leaving an orphaned auto-start service pointing at a deleted directory.

That sequence is still what chap-upgrade.adoc:73-81 and :139-144 instruct, even though :272 says the MSI path needs no manual disable/enable. chap-uninstall.adoc:165 ("the uninstaller stops and removes the OpenDJ Server service") is false in that state. Also note this PR removes the only CI coverage of those two commands — master's test-msi ran --enableService/--disableService; the new one just starts the service.

The release pipeline never rebuilds the native .exes (medium-high)

release-maven runs on ubuntu-latest and builds the server zip from the committed opendj-server-legacy/lib/*.exe; the nmake all + xcopy step (build.yml:81-86) is Windows-only. release-msi just install:install-files that zip. build.yml prints git status after the xcopy but never asserts on it.

So a future service.c fix that isn't re-committed as a blob leaves all CI green (CI builds a fresh exe) while the tagged release ships the old wrapper. One line closes it:

- run: git diff --exit-code -- opendj-server-legacy/lib

FWIW the binary diff here is also mostly unrelated: winlauncher.exe and launcher_administrator.exe changed with zero source change — linker 14.44 → 14.51, +3150 bytes .text each.

wix is an unguarded build prerequisite (medium)

opendj-packages/pom.xml:74-85 activates distribution-windows on <os><family>windows</family></os> and unconditionally pulls in opendj-msiopendj-msi-standardexec:exec on wix, with no skip property, no guard, no failOnError. Any Windows contributor running mvn install at the root now gets Cannot run program "wix" mid-reactor; pre-PR the toolset was downloaded automatically.

The prerequisite is also three commands, not one — DOTNET_ROLL_FORWARD=Major, dotnet tool install --global wix --version 5.0.2, and wix extension add -g WixToolset.UI.wixext/5.0.2 (needed because the pom passes -ext, opendj-msi-standard/pom.xml:141). The module <description> mentions only the first, unpinned, without the extension — following it still fails. Please add a skip property and put the real incantation in a contributor-facing doc (grep -ri wix over *.md/*.adoc currently returns nothing).

Upgrade-location detection works, but by undefined behavior (medium)

package.wxs:34-45. To be clear about what is not wrong: the unconditional SetProperty is correct — that is the Remember Property pattern, and gating it on NOT Installed would send uninstall/repair of a custom-directory install to the default path. And the registry search does win today.

But it wins only because MSI orders AppSearch rows by string-pool ID, which follows authoring order — and MS documents text-column sort order as undefined ("the sort sequence of a text column is undefined"). Splitting the searches makes the precedence explicit and lets you gate just the legacy-directory fallback:

<Property Id="OPENDJ_REG" Secure="yes">
  <RegistrySearch Id="InstallDirRegistrySearch" Root="HKLM" Key="SOFTWARE\$(var.name)"
                  Name="InstallDir" Type="directory"/>
</Property>
<Property Id="OPENDJ_LEGACY" Secure="yes">
  <DirectorySearch Id="LegacyX86InstallDir" Path="[ProgramFilesFolder]$(var.name)"/>
</Property>
<!-- legacy dir: fresh installs only -->
<SetProperty Id="OPENDJ" Action="SetOpendjFromLegacyDir" After="AppSearch" Sequence="both"
             Value="[OPENDJ_LEGACY]" Condition="NOT Installed AND OPENDJ_LEGACY AND NOT OPENDJ"/>
<!-- registry: unconditional, so maintenance resolves the real location -->
<SetProperty Id="OPENDJ" Action="SetOpendjFromRegistry" After="SetOpendjFromLegacyDir" Sequence="both"
             Value="[OPENDJ_REG]" Condition="OPENDJ_REG"/>

The registry branch also has zero CI coverage: build.yml:735 passes OPENDJ= explicitly (so NOT OPENDJ is false), and :768 uninstalls first, which removes the registry value.

Nits

  • MSI-only installs have no event-log source: createRegistryKey() (service.c:167-332) writes EventMessageFile/TypesSupported/CategoryCount and is called only from windows-service.bat --enableService, which the new docs tell users not to run. Every service event then renders as "The description for Event ID (n) … cannot be found." The MSI needs the same three RegistryValues.
  • 8.3 paths break service lookup: serviceCmdsMatch normalizes only the dir token; the exe path uses plain _stricmp and GetFullPathName doesn't expand short names. OPENDJ="C:\PROGRA~1\OpenDJ" → lookup fails → net start fails with 2186. Run both tokens through GetLongPathName.
  • ImagePath quoting is asserted but never tested: the whole [OPENDJ]. hack rests on how msiexec quotes the exe path, and the default path has spaces. Add a CI assertion on HKLM\SYSTEM\CurrentControlSet\Services\OpenDJ\ImagePath (unquoted + spaces would be CWE-428).
  • attach-artifact loses a fail-fast check: opendj-msi-standard/pom.xml:53-70 is declared first, so it attaches ${msi.file} before wix build creates it. install/deploy do catch a missing file, but the pre-PR antrun <attachartifact> had a File.exists()BuildException that fired at the right moment. Residual risk is a stale MSI: target/ isn't cleaned, so a skipped wix build silently attaches the previous run's output. Move it after the exec execution. (attach-msi-and-bundle also names a bundle that doesn't exist.)
  • exec-maven-plugin has no version (opendj-msi-standard/pom.xml:124-126) and nothing manages it in the chain — the root pom has no parent, and help:effective-pom shows zero occurrences. Maven warns and resolves the newest release (3.6.3 today).
  • No-JRE test accepts any failure: build.yml:645 asserts only ExitCode -ne 0, so 1619/1620/1601 pass as "refused without a JRE" — and Stop-Service msiserver two lines earlier makes 1601 plausible. The Select-String … "requires Java" result is printed, never checked. Assert -Quiet on it and on exit code 1603.
  • sc.exe getkeyname parsing (build.yml:749): .Matches[0] on no match throws "Cannot index into a null array" instead of the intended message, and sc.exe output is localized.
  • External download on every PR run: build.yml:718 fetches releases/download/5.1.2/opendj-5.1.2.msi with no retry or checksum; an asset rename breaks unrelated PRs.
  • MSI built twice: build.yml:92 gates WiX setup on runner.os == 'Windows', and the module activates on OS family, so windows-latest-26 does a full staging copy and cab build that nothing consumes. Payload doesn't depend on the build JDK (maven.compiler.source/target = 11, no bundled JRE).
  • No AllowSameVersionUpgrades="yes": a rebuilt MSI at the same 3-part version isn't detected as an upgrade — two ARP entries and a refcount mess on any hotfix re-release.
  • No ARPINSTALLLOCATION: ARP/winget/SCCM can't report the install location. <SetProperty Id="ARPINSTALLLOCATION" Value="[OPENDJ]" After="CostFinalize"/>.
  • MajorUpgrade has no Schedule, so it defaults to afterInstallValidate. Safe today only because the harvest covers template/**; Schedule="afterInstallExecute" is the safer default.
  • OPENDJ isn't Secure="yes" (only OPENDJ_DETECTED is), yet the docs tell users to pass it on the command line — worth testing a non-admin-triggered elevated install.
  • Stale comments: opendj-msi/pom.xml:35-37 ("built on every OS") and opendj-msi-standard/pom.xml:33 ("cross-platform; no wine") contradict opendj-packages/pom.xml:80-81 and your own PR description. That comment also misattributes the cause: WIX0389 is PayloadMustBeRelativeToCache ("not a relative path"); the actual blocker is WiX's [DllImport("msi.dll")] P/Invokes.
  • chap-install.adoc:663 says the x64 package installs under C:\Program Files\OpenDJ "by default" — false whenever the legacy dir or registry value exists, which your own autodetect CI step asserts.
  • .gitattributes pins *.wxi text eol=lf but there is no .wxi file in the repo; meanwhile *.exe — now actively-maintained tracked binaries — gets no binary attribute.

…ion detection

- release.yml: grant release-msi contents:write so the MSI attaches to the release
- deploy.yml: grant actions:read so the cross-run MSI artifact download works
- package.wxs: replace the cmd.exe/net/sc legacy-service custom action with
  ServiceControl; drop the Java launch condition; skip the placeholder EULA dialog;
  split the install-location searches with explicit precedence; add
  AllowSameVersionUpgrades, ARPINSTALLLOCATION, Secure OPENDJ and the event-log source
- service.c: refuse to remove the MSI-managed service from --disableService and
  expand 8.3 paths in serviceCmdsMatch (launcher binaries to be refreshed from CI)
- Gate the MSI module on an installed wix tool (distribution-windows-msi profile),
  pin exec-maven-plugin, attach the MSI only after the wix build produces it
- CI: assert committed launcher binaries match the sources, assert a quoted
  ImagePath, cover the registry install-location branch and the --disableService
  guard, retry the 5.1.2 download, parse sc.exe output null-safely, build the MSI
  on the java 11 job only
- Docs: scope --disableService advice to zip installs, fix the default-directory
  and Java wording
@vharseko vharseko added security Security fixes / CodeQL code-scanning alerts docs labels Aug 6, 2026
@vharseko

vharseko commented Aug 6, 2026

Copy link
Copy Markdown
Member Author

Thanks for the thorough review — almost everything reproduced exactly as described. Addressed in 3aa24c5 unless noted otherwise.

Blockers

  • release-msi now has permissions: contents: write, and deploy.yml grants actions: read. On the "three independent ways the MSI silently vanishes" point, the deliberate decision is: an MSI failure must not block a release (the Windows runner + WiX are the least reliable part of the pipeline), so continue-on-error stays on the job — but with the 403 fixed, the remaining failure modes are visible red jobs rather than silent successes.
  • Dropping opendj-msi-standard from Maven Central is intentional — now stated in the PR description. The MSI is distributed via GitHub Releases and the Package/Deploy artifact; the coordinate stays at 5.1.2 as its last published version.

RemoveLegacyService — replaced with exactly the ServiceControl you suggested (Stop="install" Remove="install" Wait="yes"). Note this deliberately tightens semantics: a legacy service that fails to stop now fails the install instead of silently proceeding. The localized display-name gap (OpenDS on ko/zh_TW) applies to the ServiceControl row just as much; left as a documented limitation in the wxs comment.

EULA — restored the <Publish> overrides (Welcome → InstallDir); the license is shown by setup, as before the rewrite.

Java launch condition — dropped entirely rather than extended: given the #664 history, and that every install-time signal (registry keys included) can false-negative on a machine where the server would run fine (OPENDJ_JAVA_HOME, default.java-home), the installer now installs unconditionally and the docs state that setup/the server require Java 11+. The no-JRE CI test went with it.

--disableServiceopendj_service.exe now refuses to remove the MSI-managed key OpenDJ (guard in removeService()), CI asserts it against an MSI install, and "Before You Upgrade" scopes the disable/enable advice to zip installs. One correction though: the PR does not remove the only CI coverage of those two commands — the zip-package Windows test still runs --enableService/--disableService; what was missing was coverage against an MSI install, which is now added.

Start="auto" — keeping auto deliberately: nothing would promote a demand-start service back to auto after setup (windows-service.bat manages its own service, not the MSI one), so demand means the server never comes back after a reboot unless the user runs sc config by hand. The boot-time error events on an unconfigured tree until setup runs are the accepted cost.

Release never rebuilds the exes — added your git diff --exit-code -- opendj-server-legacy/lib, with the exe artifact upload moved just before it so a failing run already carries the refreshed binaries for the fix-up commit. Heads-up: the next Build run will be red on that step by design — service.c changed in this round and the committed launchers need a refresh from the windows-exe-11 artifact.

wix guard — new distribution-windows-msi profile activates only when %USERPROFILE%\.dotnet\tools\wix.exe exists (plain mvn install no longer breaks mid-reactor); the full three-command toolchain is in the module description; the MSI now builds on the java-11 job only, which also removes the double build.

AppSearch — split into OPENDJ_REG/OPENDJ_LEGACY with explicit SetProperty ordering as suggested (legacy gated on fresh installs; registry unconditional but still yielding to an explicit OPENDJ), and the registry branch got a CI scenario: planted InstallDir plus a legacy directory, asserting the registry wins.

Nits — applied: event-log source registered by the MSI itself, GetLongPathName on both tokens in serviceCmdsMatch, ImagePath quoting asserted in CI, attach-artifact moved after the wix build and renamed, exec-maven-plugin pinned to 3.6.3, null-safe sc.exe getkeyname parsing, retry on the 5.1.2 download, stale comments/docs/.gitattributes fixed. Two exceptions:

  • MajorUpgrade/@Schedule: kept the afterInstallValidate default. Late RemoveExistingProducts requires matching component GUIDs for matching paths, and the auto-generated GUIDs of the WiX3 x86 package vs this WiX5 x64 one cannot guarantee that — a late REP would delete freshly installed files. Early removal is safe here precisely because instance data is outside any component set; rationale is now a comment in the wxs. (AllowSameVersionUpgrades is applied.)
  • Two tiny factual notes: C:\Users\javier does not actually match ~>< "java" ("java" is not a substring of "javier" — the JavaScript-toolchain example is the real one), and sc delete followed by CreateService yields ERROR_SERVICE_MARKED_FOR_DELETE (1072) rather than ERROR_DUPLICATE_SERVICE_NAME. Neither changes your conclusions.

@vharseko
vharseko requested a review from maximthomas August 6, 2026 14:46
…(/Brepro)

lib/*.exe are taken from the windows-exe-11 artifact of Build run 31112196230:
opendj_service.exe now refuses to remove the MSI-managed service and expands
8.3 paths in serviceCmdsMatch; the launchers pick up only PE-timestamp changes.

/Brepro is added to cl and link so the PE TimeDateStamp becomes a content hash:
without it every build differs in the timestamp fields and the
binaries-match-sources CI guard could never pass. These binaries still predate
the flag, so the guard stays red for one more run; the follow-up refresh from
the first /Brepro build is final.
lib/*.exe are taken from the windows-exe-11 artifact of Build run 31114568111 -
the first build with /Brepro, so the PE timestamp fields are now content hashes.
No source change: this only realigns the committed bytes with the reproducible
toolchain output, after which the binaries-match-sources CI guard stays green.
The registry install-location action must run before the legacy-directory
fallback: both are guarded by NOT OPENDJ, so with the legacy action first an
existing Program Files (x86)\OpenDJ directory won over the InstallDir registry
value. Precedence is now explicit OPENDJ -> registry -> legacy directory.

The --disableService guard test failed on the expected non-zero exit code of
windows-service.bat (the pwsh wrapper turns the last exit code into the step
result): capture it, assert the refusal, and finish with an explicit exit 0.

@maximthomas maximthomas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid work — the WiX 5 rewrite is coherent, the rationale comments are accurate, and the two new end-to-end MSI jobs cover ground that was untested. Everything below comes from moving service ownership into the installer: three regressions that the new CI cannot see, plus a few things worth hardening. The binaries are already refreshed, so the "next Build run is red by design" note no longer applies.

Corrections to my previous round: the custom-directory upgrade is not data loss and not a regression (5.1.2 had no location detection and the same default schedule, and config/db/logs belong to no component); ServiceControl has no Return attribute in any WiX version, so aborting on a failed stop is exactly the deliberate choice you described; the expandLongPath cost is noise next to the QueryServiceConfig calls already in that loop; and exec-maven-plugin genuinely cannot be version-less, since the root pom does not manage it.

uninstall.bat fails on every MSI install (blocker)

The refusal returns 3, which is already SERVICE_DISABLE_ERROR:

// opendj-server-legacy/src/build-tools/windows/service.c:2661
if (_stricmp(serviceName, MSI_SERVICE_NAME) == 0) { ...; returnCode = 3; }

serviceCmdsMatch now makes serviceState() resolve the MSI service, so Uninstaller runs the disable step and throws on the result:

// opendj-server-legacy/src/main/java/org/opends/quicksetup/uninstaller/Uninstaller.java:1375
default: throw new ApplicationException(ReturnCode.WINDOWS_SERVICE_ERROR, errorMessage, null);

deleteExternalDatabaseFiles0() / deleteInstallationFiles0() never run — the server is stopped and nothing is deleted. uninstall.bat is in the payload (opendj-packages/opendj-msi/opendj-msi-standard/pom.xml excludes the extension-less uninstall, not uninstall.bat), and since msiexec /x deliberately leaves config/db/logs, it is the natural next step for a user. The displayed message then tells them to run windows-service.bat -d, which can never succeed.

Give the refusal its own return code and treat it as a skip in Uninstaller.disableWindowsService() and InstallerHelper.disableWindowsService(). That also fixes the swallowed message (see Nits).

A second instance can never register its service again (blocker)

<!-- opendj-packages/opendj-msi/opendj-msi-standard/resources/msi/package.wxs:135 -->
<ServiceInstall Id="OpenDJService" Name="OpenDJ" DisplayName="OpenDJ Server" .../>

The uniqueness loop that produces OpenDJ Server-2 compares key names only:

// opendj-server-legacy/src/build-tools/windows/service.c:2101
if (strcmp (serviceName, curService.serviceName) == 0)

Key OpenDJ Server is free, so createServiceName stops at i == 1 and CreateService(scm, "OpenDJ Server", "OpenDJ Server", ...) fails with ERROR_DUPLICATE_SERVICE_NAME against the MSI's display name. There is no retry (service.c:2247 maps it straight to DUPLICATED_SERVICE_NAME), and the name is not user-selectable — so on a host with the MSI product installed, windows-service.bat --enableService in any zip instance fails permanently. That is the normal multi-instance replication setup. DisplayName="OpenDJ", or comparing displayName in serviceNameInUse(), closes it.

Vital="no" hides a failed service registration (blocker)

<!-- opendj-packages/opendj-msi/opendj-msi-standard/resources/msi/package.wxs:138 -->
Arguments="start &quot;[OPENDJ].&quot;" Vital="no"/>

If CreateService fails — e.g. the legacy service still has an open handle, so DeleteServices only marked it for deletion and InstallServices hits ERROR_SERVICE_MARKED_FOR_DELETE — MSI logs error 1923 and continues. msiexec exits 0, ARP shows the product installed, and there is no service. It is the only unexplained attribute in the file; dropping it restores the yes default.

RemoveExistingProducts runs before StopServices (should fix)

The comment at package.wxs:145 claims the ServiceControl row gives "right instance semantics", but with the default afterInstallValidate schedule RemoveExistingProducts is at ~1401 while StopServices is at 1900. Upgrading a running 5.1.x without stopping it first — which the PR advertises as no longer needing a manual --disableService — deletes lib\*.jar and lib\opendj_service.exe while the JVM holds them, so they get queued with MOVEFILE_DELAY_UNTIL_REBOOT and can remove the freshly installed binaries at the next boot. test-msi-upgrade runs net stop "OpenDJ Server" first, so it never sees this. Either stop the legacy service earlier (a Before="RemoveExistingProducts" custom action, or accept a custom action here) or document that the service must be stopped first.

Repair stops a running server and never restarts it (should fix)

<!-- opendj-packages/opendj-msi/opendj-msi-standard/resources/msi/package.wxs:139 -->
<ServiceControl Id="OpenDJServiceControl" Name="OpenDJ" Stop="both" Remove="uninstall" Wait="yes"/>

Stop="both" fires on install, reinstall and repair; there is no Start, so msiexec /f (or Repair in Apps & Features) silently takes a production directory server offline with no message. Stop="uninstall" covers both the uninstall and the RemoveExistingProducts-driven upgrade, so both buys nothing here.

Legacy-service removal is not instance-aware (should fix)

<!-- opendj-packages/opendj-msi/opendj-msi-standard/resources/msi/package.wxs:147 -->
<ServiceControl Id="RemoveLegacyOpenDJServer" Name="OpenDJ Server" Stop="install" Remove="install" Wait="yes"/>

MSI has no way to check which instance the service points at, so this deletes whichever instance happens to hold the bare name. On a host running an unrelated zip instance at D:\opendj, a fresh MSI install into C:\Program Files\OpenDJ stops and removes that instance's service — and Wait="yes" means a slow shutdown there fails this install. It also re-fires on every repair. The trade-off may be acceptable, but chap-install.adoc says nothing about it (only chap-upgrade.adoc does), and the localized case (OpenDS) silently matches nothing, leaving two auto-start services on one directory.

A release can publish with no MSI and stay green (should fix)

.github/workflows/release.yml:95 uploads release-server-zip with continue-on-error: true and the default if-no-files-found: warn; release-msi then fails at its download-artifact, and the job-level continue-on-error: true (:166) keeps the workflow conclusion green. Release on GitHub (:110-119) no longer lists the MSI, so it reaches the release only through release-msi. Net effect: a tagged release ships without an MSI and nothing is red. Same shape in .github/workflows/deploy.yml:116,124, where both the download and the upload are continue-on-error. Keeping the job non-blocking is fine — but set if-no-files-found: error on the uploads so the loss is visible.

The launcher-binary guard is a tripwire (should fix)

# .github/workflows/build.yml:100
run: git diff --exit-code -- opendj-server-legacy/lib

/Brepro replaces build-time timestamps with a content hash; it does not make codegen or the linked CRT toolchain-independent. This PR demonstrates it: between be03df284c and ac4eeedf19 — identical sources, both post-/Brepro — the three exes differ by ~3 KB each, because the runner's linker moved 14.44 → 14.51 mid-PR. The next runner-image bump therefore reds both Windows jobs at a step that runs before Build with Maven, so no windows-latest-11 artifact is produced, test-msi* and build-docker* skip, and on master deploy.yml does not run at all — until someone with a Windows box re-commits three binaries.

Worse, the guard cannot catch the case it exists for. The preceding step is shell: cmd and ends with git status, so its exit code is always 0:

# .github/workflows/build.yml:80-85
shell: cmd
run: |
  cd opendj-server-legacy\src\build-tools\windows
  nmake all
  xcopy /Y *.exe ..\..\..\lib\
  git status

A broken nmake all leaves lib/ untouched, so git diff --exit-code passes. Adding || exit /b 1 after nmake/xcopy fixes that; pinning toolset: on ilammy/msvc-dev-cmd, or downgrading the comparison to a warning, removes the tripwire.

A custom-directory upgrade still relocates (should fix)

Not the blocker I called it last round — instance data survives and 5.1.2 behaved the same way — but for a 5.1.x install outside [ProgramFilesFolder]OpenDJ, OPENDJ_REG and OPENDJ_LEGACY are both empty (5.1.2 writes no registry value), so a /quiet upgrade without OPENDJ installs to C:\Program Files\OpenDJ while RemoveExistingProducts empties the old tree. chap-upgrade.adoc:266 does warn, and there is no CI coverage because both upgrade steps pass OPENDJ=C:\opendj. A third search on the legacy service's ImagePath (HKLM\SYSTEM\CurrentControlSet\Services\OpenDJ Server\ImagePath embeds the old root), or a launch condition when WIX_UPGRADE_DETECTED is set but OPENDJ resolved to the untouched default, would close it.

Nits

  • cleanup bypasses the guard: the MSI_SERVICE_NAME check is only in removeService(); windows-service.bat --cleanupService OpenDJ reaches removeServiceWithServiceName() (opendj-server-legacy/src/build-tools/windows/service.c:2600) and deletes the key plus its event-log key anyway. Moving the check one level down makes the guard consistent.
  • The refusal message is never printed: service.c:2666 writes it to stdout, but ConfigureWindowsService.java:474 is Runtime.getRuntime().exec(cmd).waitFor() with no stream drained. Users see only "An unexpected error occurred trying to disable the server as a Windows service. Check that you have administrator rights" — resolved for free by the dedicated return code above.
  • Start="auto" before setup: package.wxs:137 plus an unconfigured tree means the SCM logs a start failure on every boot until setup.bat runs. Worth a sentence in chap-install.adoc, which currently only says the service is not started during installation.
  • The 5.1.2 CI comment is wrong: .github/workflows/build.yml:725 says the released 5.1.2 scripts cannot run from a directory with spaces, but the 5.1.2 tag (2026-07-17) post-dates #671 (merged 2026-07-08) — the scripts are byte-identical to this branch. Because of that the old product is installed into C:\opendj, so the headline path (real 5.1.2 at its own default, upgraded with no OPENDJ) is never tested end to end.
  • release.yml:215 relies on implicit profile activation: mvn ... package -pl :opendj-msi-standard never passes -Pdistribution-windows-msi, so any drift in USERPROFILE or the dotnet tool path yields "Could not find the selected project in the reactor" — swallowed by the job's continue-on-error. Adding the flag costs nothing.
  • Custom install dirs and LocalSystem: the service runs as LocalSystem with no Account, and a directory created directly under C:\ (as OPENDJ=C:\opendj in the docs and CI) inherits Authenticated Users:(OI)(CI)(IO)(M). Any standard user can then replace bat\start-ds.bat and get SYSTEM. A doc warning, or an ACL reset on [OPENDJ], would help.
  • Upgrades discard service hardening: because the old product is uninstalled first, sc config OpenDJ obj= DOMAIN\svc-opendj, recovery actions and dependencies are lost on every upgrade and the service silently returns as LocalSystem.
  • No "run as Administrator" note: chap-install.adoc:665,678 tell users to run setup.bat and net start from C:\Program Files\OpenDJ; a 64-bit JVM is never file-virtualized, so both fail without elevation. Pre-existing, but the x64 move makes the protected directory the default.
  • exec-maven-plugin versions diverge: 3.6.3 in opendj-packages/opendj-msi/opendj-msi-standard/pom.xml:112 vs 1.3.2 in opendj-server-legacy/pom.xml:1406, neither centrally managed. Root pluginManagement would let both converge.
  • GUI path is untested: every CI install uses /quiet /qn, so the restored license <Publish> overrides and the InstallDirDlg pre-population from a detected OPENDJ are unverified. One manual run before merge would be worth it.

- opendj_service.exe refuses to remove the MSI-managed key with a dedicated
  code (4): uninstall.bat and windows-service.bat now treat it as an
  informational skip instead of failing, and --cleanupService is covered too
- Name the MSI service 'OpenDJ' (key = display name): the windows-service.bat
  naming loop checks key names only, so any other display name made every
  additional zip instance's --enableService fail with a duplicate name
- Drop Vital="no": a failed service registration fails the install instead of
  reporting success with no service; Stop="uninstall" keeps repair from
  silently stopping a production server
- Stop a running legacy service before RemoveExistingProducts (immediate
  action, full net.exe path) and remove it only when an existing OpenDJ was
  detected - a fresh install must not touch an unrelated instance's service
- Refuse a silent upgrade when the existing location cannot be determined
  (5.1.x wrote no registry value) instead of relocating to the default
- CI: upgrade from 5.1.2 at its real x86 default without OPENDJ, uninstall.bat
  and refusal scenarios, if-no-files-found: error on the MSI-chain uploads,
  nmake/xcopy error propagation, binaries guard downgraded to a warning
- Manage exec-maven-plugin centrally (3.6.3), document the new behaviors
…stribution-new

# Conflicts:
#	.github/workflows/build.yml
@vharseko vharseko added the java Pull requests that update java code label Aug 7, 2026
@vharseko

vharseko commented Aug 7, 2026

Copy link
Copy Markdown
Member Author

Thanks for the second pass — the service-ownership consequences were exactly the right place to look, and all three blockers reproduced. Addressed in af7d80f (plus a master merge in f0c9a11); notes below follow your headings.

uninstall.bat — the refusal now has its own wrapper return code (4). removeServiceWithServiceName carries the guard, so --cleanupService is covered too; ConfigureWindowsService maps it to a new SERVICE_MSI_MANAGED constant with a proper message (INFO_WINDOWS_SERVICE_MSI_MANAGED), Uninstaller.disableWindowsService() treats it as a skip, and InstallerHelper needed no change (it only throws on SERVICE_DISABLE_ERROR). That also resolves the swallowed-stdout nit — the message now comes from the Java side. CI runs uninstall.bat --cli --remove-all against an MSI install and asserts the instance files are gone while the service survives for msiexec /x.

Second instance — fixed by naming the service OpenDJ outright (key = display name), so the createServiceName loop's first candidate OpenDJ Server no longer collides with anything and no C change was needed. CI and docs now use net start OpenDJ; the upgrade test asserts the display name and that no OpenDJ Server service remains.

Vital="no" — dropped; with the default a failed CreateService (e.g. ERROR_SERVICE_MARKED_FOR_DELETE) fails the install with rollback instead of reporting success with no service.

REP before StopServices — one note: chap-upgrade.adoc step 1 has always required stopping the server (net stop) before installing the newer package, so the documented path was safe. But the PR does advertise no-manual-steps upgrades, so a running legacy service is now stopped explicitly: an immediate "[System64Folder]net.exe" stop "OpenDJ Server" action (full path — no working-directory resolution) sequenced before RemoveExistingProducts, WIX_UPGRADE_DETECTED only, Return="ignore". Deferred was not an option — with the afterInstallValidate schedule REP sits before InstallInitialize.

RepairStop="uninstall"; agreed that both bought nothing except taking a production server down on msiexec /f.

Instance-awareness — the legacy ServiceControl moved into its own component, conditioned on WIX_UPGRADE_DETECTED OR OPENDJ_LEGACY OR OPENDJ_REG: a fresh install on a host running an unrelated zip instance elsewhere no longer touches that instance's service. chap-install.adoc now states when the replacement happens (and the boot-time start-failure note until setup runs, plus the localized OpenDS gap staying documented in the wxs).

Release visibilityif-no-files-found: error on both the release-server-zip upload and the deploy.yml MSI upload; the jobs stay non-blocking per the earlier decision, but a missing MSI is now a red step instead of a warn. release.yml also passes -Pdistribution-windows-msi explicitly.

The guard — you're right about the cascade and about the swallowed nmake failure: nmake/xcopy now || exit /b 1, and the comparison is warning-only (continue-on-error + a ::warning annotation naming the artifact to refresh from), so a toolchain bump can no longer kill test-msi*/deploy. One correction to the evidence, not the conclusion: the ~3 KB delta between be03df2 and ac4eeed is the /Brepro flag being introduced (be03df2's committed binaries were still a pre-flag build), not linker drift — the 14.44→14.51 observation from your first round is the real precedent.

Custom-directory upgrade — a type-19 error action now refuses when WIX_UPGRADE_DETECTED AND NOT OPENDJ (execute sequence only: in the wizard the user sees and confirms the directory in InstallDirDlg, which sets OPENDJ). There are no false positives — x64 installs write the registry value and the legacy default is detected. CI covers it: 5.1.2 at C:\opendj-custom, silent upgrade without OPENDJ must exit non-zero with the guidance message and leave the original tree untouched.

5.1.2 comment — good catch, and it unlocked the honest headline test: test-msi-upgrade now installs 5.1.2 at its real Program Files (x86)\OpenDJ default (verified against the 5.1.2 tag's wxs) and upgrades with no OPENDJ at all; the explicit-OPENDJ path stays covered by the registry-detection scenario.

Remaining nitsexec-maven-plugin converged on 3.6.3 via root pluginManagement (the 1.3.2 pin in opendj-server-legacy is gone; the full-matrix build exercises the MIB generation with it). Docs gained the LocalSystem/ACL warning for root-level custom directories, the run-as-Administrator note, and the service-hardening-lost-on-upgrade caveat. The GUI wizard run stays manual — planned before merge.

The committed launcher binaries are pending one more refresh (the service.c guard moved and gained a return code), which now surfaces as the warning annotation rather than a red job.

Taken from the windows-exe-11 artifact of Build run 31191804954. Only the
service wrapper changed (the MSI-managed guard moved into
removeServiceWithServiceName and gained the dedicated return code 4);
winlauncher.exe and launcher_administrator.exe came out byte-identical to the
committed ones - the first live confirmation that the /Brepro build is
reproducible.
@vharseko
vharseko requested a review from maximthomas August 7, 2026 16:48

@maximthomas maximthomas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round-3 review of af7d80f9da + 4bf7927489. The service-ownership rework reads well and I could not fault service.c — the tokenizer bounds, normalizeInstanceDir edge cases and the round-trip against the MSI's ImagePath ("<root>\lib\opendj_service.exe" start "<root>\.") all check out, as do the message wiring and .gitattributes.

Two blockers below, both outside the paths CI exercises. Also note the MSI jobs have not run on round 2 yet — build-maven (ubuntu-latest, 11) is still in flight, so test-msi/test-msi-upgrade last passed on c287e5b3cf. Nothing is gated; they just need a green build.

Two things from my round-2 comments I withdraw after checking: dropping Vital="no" is a no-op (WiX 5.0.2 defaults Vital to noCompiler_Package.cs:3531), and the missing WixUI_ErrorProgressText is harmless (Windows Installer falls back to built-in localized ActionText/Error strings).

Released MSI ships the slim server zip (blocker)

.github/workflows/release.yml:100 uploads both zips:

path: target/checkout/opendj-server-legacy/target/package/*.zip

and .github/workflows/release.yml:210 picks the first one:

ZIP=$(ls server-zip/*.zip | head -1)

opendj-<ver>-slim.zip sorts before opendj-<ver>.zip (- = 0x2D < . = 0x2E, in both C and en_US). The slim zip excludes the backend drivers (opendj-server-legacy/pom.xml:1130):

lib/org.apache.cassandra.*,lib/com.oracle.database.jdbc*,lib/com.mysql.mysql-connector*,
lib/com.microsoft.sqlserver*,lib/org.postgresql.*

dependency:unpack expands it without error (same opendj/ prefix), so every released MSI silently lacks JDBC/Cassandra support while the same release's .zip has it. test-msi builds through the normal reactor with the full zip, so only the release path is affected.

ZIP=$(ls server-zip/*.zip | grep -v -- '-slim\.zip$' | head -1)

Legacy-service guard defeats itself after the first install (blocker)

opendj-packages/opendj-msi/opendj-msi-standard/resources/msi/package.wxs:194:

<Component Id="RemoveLegacyServiceControl" Directory="OPENDJ"
           Condition="WIX_UPGRADE_DETECTED OR OPENDJ_LEGACY OR OPENDJ_REG">
  <ServiceControl Id="RemoveLegacyOpenDJServer" Name="OpenDJ Server"
                  Stop="install" Remove="install" Wait="yes"/>

OPENDJ_REG reads HKLM\SOFTWARE\OpenDJ\InstallDir (line 53) — the value this package's own InstallDirRegistry component writes (line 204). Same root, key, name and registry view. So the condition is false only on a clean first install; from then on it is permanently true on that host, and a repair (msiexec /f, or ARP → Repair via MaintenanceTypeDlg) transitions the component Absent→Install and deletes an unrelated zip instance's OpenDJ Server service — the exact case the comment on line 192 says must not be touched. Recovery needs windows-service.bat --enableService.

WIX_UPGRADE_DETECTED is a second path to true, so removing the OPENDJ_REG disjunct alone does not close it. Gating on something that proves this product owns the legacy service would.

Custom-directory upgrade guard cannot fire in the wizard (major)

package.wxs:90 schedules the guard in InstallExecuteSequence only:

<Custom Action="RequireDirOnCustomUpgrade" After="SetOpendjFromLegacyDir"
        Condition="WIX_UPGRADE_DETECTED AND NOT OPENDJ"/>

In a full-UI install the UI sequence runs first and its CostFinalize (1000) resolves the OPENDJ Directory row to C:\Program Files\OpenDJ\ before ExecuteAction. NOT OPENDJ is therefore false at ~403 and the guard never fires. Upgrading a 5.1.x install at, say, C:\opendj-custom (no registry value) installs to the default while RemoveExistingProducts empties the old tree.

The comment on line 75 says the user "sees and confirms the directory in InstallDirDlg" — but what InstallDirDlg shows is the default, not the detected install, and nothing tells them an existing install was found elsewhere. Comparing the resolved OPENDJ against the upgrade's recorded location, or duplicating the guard into InstallUISequence after the dialogs, would cover it.

StopLegacyServiceBeforeUpgrade runs unelevated (major)

package.wxs:86 has no Execute, so it is immediate (type 34 + Continue = 98):

<CustomAction Id="StopLegacyServiceBeforeUpgrade" Directory="TARGETDIR"
              ExeCommand="&quot;[System64Folder]net.exe&quot; stop &quot;OpenDJ Server&quot;"
              Return="ignore"/>

Immediate custom actions always run in the msiexec client process under the launching user's token — position relative to InstallInitialize is not the deciding factor. On a UAC double-click net stop returns access denied and Return="ignore" swallows it, so the legacy service keeps holding lib\*.jar through RemoveExistingProducts and the delayed-delete hazard the comment describes is unmitigated. The ServiceControl on line 198 only cleans up later, at StopServices (1900). CI cannot see this: hosted runners have a full admin token, and .github/workflows/build.yml:815 stops the service by hand first.

Return="ignore" is load-bearing for the service-absent case, so it cannot simply be dropped.

sustaining/4.10.x deploys stop publishing the MSI (major)

.github/workflows/deploy.yml is workflow_run-triggered, so GitHub always runs master's copy, but the job checks out ${{ github.event.workflow_run.head_branch }}. That branch's opendj-packages/pom.xml still lists <module>opendj-msi</module> under the /usr/bin/wine profiles. With wine no longer installed the profile stays inactive, no MSI is produced, and the upload step fails into continue-on-error: true — a yellow step and no MSI.

Native-exe staleness gate is continue-on-error (minor)

.github/workflows/build.yml:103. CI copies freshly compiled exes over opendj-server-legacy/lib/ before the Maven build, so test-msi validates the compiled wrapper, while releases ship the committed one. A PR that changes service.c without re-committing lib/*.exe goes green with a yellow warning and ships the old wrapper — which would happily delete the MSI-managed service. Worth a job that fails rather than a step that warns. (The binaries are in sync today.)

Nits

  • lib\extensions no longer created: <Files> harvests files only and OpenDJEmptyDirs (package.wxs:128) recreates the root and template/* dirs but not lib/extensions, so WARN_ADMIN_NO_EXTENSIONS_DIR logs on every startup and there is nowhere to drop a custom extension jar.
  • WindowsServicePanel not updated for code 4: opendj-server-legacy/src/main/java/org/opends/guitools/controlpanel/ui/WindowsServicePanel.java:337 still treats SERVICE_MSI_MANAGED as FINISHED_WITH_ERROR; Uninstaller.java:1374 treats it as a skip.
  • Vital comment is wrong: package.wxs:163 says "Vital defaults to yes". It defaults to no in WiX 4/5 (the XSD prose says otherwise — wixtoolset/issues#5974), so the fail-fast the comment promises needs an explicit Vital="yes". Separately, new-MSI → new-MSI upgrade is untested; the DeleteService/CreateService 1072 race yields a silently missing service.
  • SERVICE_MSI_MANAGED in the wrong constant block: ConfigureWindowsService.java:101 declares it among the disable codes but returns it from cleanupService, whose codes are deliberately renumbered and whose javadoc lists only SERVICE_CLEANUP_*. A SERVICE_CLEANUP_MSI_MANAGED would fit.
  • Duplicate refusal message: service.c:2615 prints the refusal and the Java side prints the localized INFO_WINDOWS_SERVICE_MSI_MANAGED. On non-UAC hosts the user sees both; the native one can drop to debug().
  • --cleanupService has no escape hatch: the guard keys purely on the name "OpenDJ", so an orphaned service (rolled-back install, hand-run create) can never be cleaned and needs sc delete. Checking actual MSI ownership would keep the escape hatch.
  • uninstall.bat ordering: opendj-doc-generated-ref/src/main/asciidoc/install-guide/chap-uninstall.adoc:175 recommends uninstall.bat --remove-all before msiexec /x, which deletes lib\opendj_service.exe while leaving the auto-start service registered — SCM 7000/7009 every boot until the package is removed. Worth a warning.
  • opendj-msi* no longer on Maven Central: dropped from distribution-unix/distribution-mac in opendj-packages/pom.xml, so the modules leave the Linux mvn deploy reactor. Probably intended, but it silently changes published coordinates.
  • wix lookup mismatch: opendj-packages/pom.xml:92 activates on %USERPROFILE%\.dotnet\tools\wix.exe but the build invokes bare wix via PATH. Same split in build.yml:116 / release.yml:198, which push the MSYS path /c/Users/runneradmin/.dotnet/tools into $GITHUB_PATH from a shell: bash step — it works only because the runner image already exposes the directory.
  • WixUI_ErrorProgressText parity: harmless (built-in fallback), but re-adding <UIRef Id="WixUI_ErrorProgressText"/> restores the pre-PR authoring at zero risk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build CI docs enhancement java Pull requests that update java code packaging deb/rpm/MSI, distribution layout, config.ldif security Security fixes / CodeQL code-scanning alerts Windows

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants