Skip to content

Conversation

@johnml1135
Copy link
Contributor

@johnml1135 johnml1135 commented Dec 3, 2025

Summary

Remove the chocolatey WiX downgrade workaround from CI workflows. The \windows-latest\ GitHub runner now has WiX 3.14.x pre-installed, making the downgrade step unnecessary.

Changes

  • Remove \Downgrade Wix Toolset\ step from .github/workflows/base-installer-cd.yml\
  • Remove \Downgrade Wix Toolset\ step from .github/workflows/patch-installer-cd.yml\
  • Update .NET 461\ targeting pack to .NET 481\ for compatibility
  • Modernize build commands to use \msbuild Build/Orchestrator.proj\ targets
  • Add \microsoft/setup-msbuild@v2\ action for reliable MSBuild availability
  • Add NETFX 4.8.1 tools to PATH for \sn.exe\ and other utilities

Benefits

  • ~30-60 seconds faster CI: Eliminates chocolatey install/uninstall overhead
  • Simpler workflow: Uses runner's pre-installed WiX instead of managing versions
  • Future-proof: No longer tied to specific WiX 3.11.x version

Validation Required

  • PR workflow runs successfully (this PR)
  • Base installer workflow succeeds (workflow_dispatch after merge)
  • Patch installer workflow succeeds (workflow_dispatch after merge)
  • Installer installs correctly on Windows 10/11

Related

  • Spec: \specs/007-wix-314-installer/\
  • Research confirms WiX 3.14.x is backward compatible with WiX 3.11.x-built bases

This change is Reviewable

Copilot AI review requested due to automatic review settings December 3, 2025 22:33
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR removes the obsolete WiX downgrade workaround from CI workflows and modernizes the build configuration. With WiX 3.14.x now pre-installed on windows-latest runners, the downgrade step is no longer necessary, resulting in faster builds. The changes also upgrade the .NET targeting pack from 4.6.1 to 4.8.1 for compatibility and modernize build commands to use MSBuild directly with the Orchestrator.proj targets.

Key Changes:

  • Remove chocolatey-based WiX downgrade steps from installer workflows
  • Upgrade .NET targeting pack from 4.6.1 to 4.8.1
  • Add microsoft/setup-msbuild@v2 action for reliable MSBuild availability
  • Modernize build commands to use MSBuild traversal targets directly
  • Add NETFX 4.8.1 tools to PATH for sn.exe and related utilities

Reviewed changes

Copilot reviewed 119 out of 1067 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
.github/workflows/base-installer-cd.yml Remove WiX downgrade, upgrade to .NET 4.8.1, modernize build commands using Orchestrator.proj
.github/workflows/patch-installer-cd.yml Remove WiX downgrade, upgrade to .NET 4.8.1, modernize build commands using Orchestrator.proj
.github/workflows/CI.yml Upgrade .NET targeting pack to 4.8.1, add NETFX tools to PATH, modernize build command, add COM activation smoke test
.github/workflows/copilot-setup-steps.yml New reusable workflow documenting pre-installed dependencies on windows-latest
BUFFERING_FIX.md New documentation for VwDrawRootBuffered GDI resource management fix
AGENTS.md New documentation for GitHub Copilot coding agent guidance
.vscode/tasks.json Expanded VS Code task definitions for build/test/environment operations
.vscode/settings.json New VS Code settings for terminal, C# dev kit, and chat tool auto-approval
.vscode/mcp.json New MCP server configuration for GitHub and Serena integrations
.vscode/launch.json Simplified Windows-focused debug configurations
Multiple .specify/ files New speckit workflow templates and scripts for feature development
Multiple .serena/ files New Serena MCP memory files for project context
Multiple .github/workflows/ files New validation and documentation workflows
Multiple .github/prompts/ files New AI agent prompt templates for feature development
Multiple .github/instructions/ files New concise instruction files extracted from COPILOT.md content
.github/src-catalog.md New comprehensive catalog of Src/ folder descriptions
Multiple .github/spec-templates/ files New specification templates for feature planning
Multiple .github/ Python scripts New tools for COPILOT.md scaffolding and validation
.github/pull_request_template.md New PR template with CI-ready checklist
.github/memory.md New curated agent memory for common patterns

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

fetch-depth: 0
path: "Localizations/LCM"

- name: Download .NET 481 targeting pack
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

Corrected version string from '481' to '4.8.1' for consistency with the official .NET Framework versioning scheme.

Copilot uses AI. Check for mistakes.
path: "Localizations/LCM"

- name: Download .NET 461 targeting pack
- name: Download .NET 481 targeting pack
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

Corrected version string from '481' to '4.8.1' for consistency with the official .NET Framework versioning scheme.

Copilot uses AI. Check for mistakes.
shell: cmd
working-directory: public
run: NDP461-DevPack-KB3105179-ENU.exe /q
- name: Download 481 targeting pack
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

Corrected version string from '481' to '4.8.1' for consistency with the official .NET Framework versioning scheme.

Suggested change
- name: Download 481 targeting pack
- name: Download 4.8.1 targeting pack

Copilot uses AI. Check for mistakes.
$ErrorActionPreference = 'Stop'
# Build base installer (uses traversal build internally via Installer.targets)
msbuild Build/Orchestrator.proj /t:RestorePackages /p:Configuration=Debug /p:Platform=x64
msbuild Build/Orchestrator.proj /t:BuildBaseInstaller /p:Configuration=Debug /p:Platform=x64 /p:config=release /p:action=test /p:desktopNotAvailable=true /m /v:d /bl | Tee-Object -FilePath Build/build.log
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

The configuration property appears twice with different values (Configuration=Debug and config=release), which is confusing. Clarify the intended configuration or use consistent property naming.

Suggested change
msbuild Build/Orchestrator.proj /t:BuildBaseInstaller /p:Configuration=Debug /p:Platform=x64 /p:config=release /p:action=test /p:desktopNotAvailable=true /m /v:d /bl | Tee-Object -FilePath Build/build.log
msbuild Build/Orchestrator.proj /t:BuildBaseInstaller /p:Configuration=Debug /p:Platform=x64 /p:action=test /p:desktopNotAvailable=true /m /v:d /bl | Tee-Object -FilePath Build/build.log

Copilot uses AI. Check for mistakes.
cd BuildDir
# Build patch installer (uses traversal build internally via Installer.targets)
msbuild Build/Orchestrator.proj /t:RestorePackages /p:Configuration=Debug /p:Platform=x64
msbuild Build/Orchestrator.proj /t:BuildPatchInstaller /p:Configuration=Debug /p:Platform=x64 /p:config=release /p:action=test /p:desktopNotAvailable=true /m /v:d /bl | Tee-Object -FilePath Build/build.log
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

The configuration property appears twice with different values (Configuration=Debug and config=release), which is confusing. Clarify the intended configuration or use consistent property naming.

Suggested change
msbuild Build/Orchestrator.proj /t:BuildPatchInstaller /p:Configuration=Debug /p:Platform=x64 /p:config=release /p:action=test /p:desktopNotAvailable=true /m /v:d /bl | Tee-Object -FilePath Build/build.log
msbuild Build/Orchestrator.proj /t:BuildPatchInstaller /p:Configuration=Debug /p:Platform=x64 /p:action=test /p:desktopNotAvailable=true /m /v:d /bl | Tee-Object -FilePath Build/build.log

Copilot uses AI. Check for mistakes.
@johnml1135 johnml1135 force-pushed the 007-wix-314-installer branch 3 times, most recently from 84617d8 to e8d5a9e Compare December 4, 2025 20:28
jasonleenaylor and others added 9 commits December 4, 2025 15:44
- Created convertToSDK script in Build folder
- Updated mkall.targets RestoreNuGet to use dotnet restore
- Update mkall.targets to use dotnet restore instead of old NuGet restore
- Update build scripts to use RestorePackages target

Implement and execute improved convertToSDK.py

* Use mkall.targets-based NuGet detection
* Fix test package references causing build failures
* Add PrivateAssets to test packages to exclude transitive deps

  SDK-style PackageReferences automatically include transitive
  dependencies. The SIL.LCModel.*.Tests packages depend on
  TestHelper, which causes NU1102 errors. Adding PrivateAssets="All"
  prevents transitive dependencies from flowing to consuming
  projects

Co-authored-by: jasonleenaylor <2295227+jasonleenaylor@users.noreply.github.com>

Convert DesktopAnalytics and IPCFramework to PackageReferences

Converted regular References to PackageReferences for NuGet packages:
- SIL.DesktopAnalytics (version 4.0.0) in 6 projects
- SIL.FLExBridge.IPCFramework (version 1.1.1-beta0001) in FwUtils
- Updated package versions to resolve NU1605 downgrade errors:
- Moq: 4.17.2 → 4.20.70 in XMLViewsTests.csproj
- TagLibSharp: 2.2.0 → 2.3.0 in xWorks.csproj

Co-authored-by: jasonleenaylor <2295227+jasonleenaylor@users.noreply.github.com>

Fix bare References and update convertToSDK.py script

* Fixed bare Reference elements in FieldWorks.csproj and
  XMLViews.csproj that should have been PackageReferences:
- Geckofx60.32/64 packages (provide Geckofx-Core, Geckofx-Winforms)
- SharpZipLib (provides ICSharpCode.SharpZipLib)
- SIL.ParatextShared (provides ParatextShared)
- FwControls.csproj: ParatextShared → SIL.ParatextShared
- ITextDll.csproj: Geckofx, SharpZipLib, ParatextShared → packages
- FwParatextLexiconPlugin.csproj: Paratext.LexicalContracts → ParatextData
- ScriptureUtilsTests.csproj: ParatextShared → SIL.ParatextShared
- Paratext8Plugin.csproj: Paratext.LexicalContracts → removed (provided by ParatextData)
- FwParatextLexiconPluginTests.csproj: Paratext.LexicalContracts* → ParatextData
- ParatextImportTests.csproj: ParatextShared → SIL.ParatextShared

Co-authored-by: jasonleenaylor <2295227+jasonleenaylor@users.noreply.github.com>

Fix Geckofx version and DotNetZip warnings

Updated Geckofx60.32/64 from 60.0.50/51 to 60.0.52 (only
version available on NuGet). This resolves NU1603 warnings
about missing package version 60.0.51.

Updated SharpZipLib in ITextDll.csproj from 1.3.3 to 1.4.0
to avoid downgrade warning (SIL.LCModel requires >= 1.4.0).

Suppressed DotNetZip NU1903 security warning in xWorks.csproj
and xWorksTests.csproj (already suppressed globally in
Directory.Build.props, but some projects need local suppression).

All 115 projects now restore successfully without errors.

Co-authored-by: jasonleenaylor <2295227+jasonleenaylor@users.noreply.github.com>

Fix post .csproj conversion build issues

* Add excludes for test subdirectories
* Fix several references that should have been PackageReferences
* Fix Resource ambiguity
* Add c++ projects to the solution

Delete some obsolete files and clean-up converted .csproj

* Fix more encoding converter and geckofx refs
* Delete obsolete projects
* Delete obsoleted test fixture

Copilot assisted NUnit3 to NUnit4 migration

* Also removed some obsolete tests and clean up some incomplete
  reference conversions

Update palaso dependencies and remove GeckoFx 32bit

* The conditional 32/64 bit dependency was causing issues
  and wasn't necessary since we aren't shipping 32 bit anymore

Fix broken test projects by adding needed external dependencies

* Mark as test projects and include test adapter
* Add .config file and DependencyModel package if needed
* Add AssemblyInfoForTests.cs link if needed
* Also fix issues caused by a stricter compiler in net48

Update FieldWorks.cs to use latest dependencies

* Update L10nSharp calls
* Specify the LCModel BackupProjectSettings
* Add CommonAsssemblyInfo.cs link lost in conversion
* Set Deterministic builds to false for now (evaluate later)

Spec kit and AI docs, tasks and instructions

Refine AI onboarding and workflows:
* Update copilot-instructions.md with agentic workflow links and
clearer pointers to src-catalog and per-folder guidance (COPILOT.md).
* Tune native and installer instructions for mixed C++/CLI, WiX, and build
nuances (interop, versioning, upgrade behavior, build gotchas).

Spec kit improvements:
* Refresh spec.md and plan.md to align with the
feature-spec and bugfix agent workflows and FieldWorks conventions.
Inner-loop productivity:
* Extend tasks.json with quick checks for whitespace and commit
message linting to mirror CI and shorten feedback loops.

CI hardening for docs and future agent flows:
* Add lint-docs.yml to verify COPILOT.md presence per
Src/<Folder> and ensure folders are referenced in .github/src-catalog.md.
* Add agent-analysis-stub.yml (disabled-by-default) to
document how we will run prompts/test-failure analysis in CI later.

Locally run CI checks in Powershell
* Refactor scripts and add whitespace fixing algorithm
* Add system to keep track of changes needed to be reflected in
  COPILOT.md files.

Use FieldWorks.proj for main file
Add local mulit-agent capability
Remove LexTextExe.exe
* As well as for NativeBuild
* Fix LcmArtifactsDir in mkall.targets
Co-authored-by: johnml1135 <13733556+johnml1135@users.noreply.github.com>
- Fixed DrawTheRoot to properly manage GDI bitmap resources
  * Clean up previous cached bitmap/DC before creating new ones
  * Keep bitmap in m_hdcMem for ReDrawLastDraw support
  * Delete stock bitmap that comes with new DC (not the previous bitmap)

- Fixed DrawTheRootRotated to use local DC/bitmap
  * Uses local resources since rotation makes caching impractical
  * Properly cleanup bitmap and DC on exit and in exception paths

The previous code had a critical bug: it was deleting the OLD bitmap
(hbmpOld) immediately after selecting the new one, instead of keeping
it for restoration. This caused resource leaks and visual artifacts.

The new implementation follows the correct GDI resource management pattern:
1. Create compatible DC and bitmap
2. Select bitmap into DC (save old bitmap)
3. Delete the stock bitmap (not the old one!)
4. Draw to bitmap
5. Blit to screen
6. Keep bitmap/DC cached in m_hdcMem for ReDrawLastDraw (DrawTheRoot)
   OR cleanup local resources (DrawTheRootRotated, DrawTheRootAt)

Also, update COPILOT.md files to be shorter
* Remove all Any CPU mappings
* Ensure each project has a Debug and Release mapping to x64
* For any csproj file map bounds to Debug, but .vcxproj files
  still map to Bounds (for c++ memory debugging)
Infrastructure:
- Configure Serena MCP with OmniSharp (IPv6 workaround)
- Enable managed code builds in Docker containers
- Add unified intermediate output paths (host: Obj/, container: C:\Temp\Obj/)

CI/CD:
- Upgrade NETFX 4.6.1→4.8.1, WiX 3.11.2→3.14.1
- Add copilot-setup-steps.yml with testable agent scripts
- Add docker-build-publish.yml workflow

Testing:
- Fix VSTest execution (DependencyModel, System.Memory conflicts)
- Add Test.runsettings with InIsolation=true
- Resolve 500+ pre-existing test failures

Docs:
- Migrate FwDocumentation wiki to docs/ and .github/instructions/
- Add AGENTS.md, CONTRIBUTING.md, developer setup guides
Re-checkout all 261 test files from origin/release/9.3 and run
scripts/tests/convert_nunit.py with corrected comparison logic.

This fixes swapped argument order in Greater/Less assertions that
were introduced during the SDK migration rebase (43d1fd5).

The previous conversion incorrectly produced:
  Assert.That(0, Is.GreaterThan(value))  // WRONG - always false

Now correctly produces:
  Assert.That(value, Is.GreaterThan(0))  // CORRECT

Also restores 3 test files that were unintentionally deleted during
the SDK migration:
- Lib/src/ScrChecks/ScrChecksTests/SentenceFinalPunctCapitalizationCheckUnitTest.cs
- Src/Common/ViewsInterfaces/ViewsInterfacesTests/ExtraComInterfacesTests.cs
- Src/FwCoreDlgs/FwCoreDlgsTests/FwWritingSystemSetupDlgTests.cs
Re-apply intentional fixes from commits 575eaa0..9eff1d4 on top
of the clean NUnit conversion.

Changes applied:
- FieldWorksTests.cs: Use rooted temp paths for cross-platform compat
- IVwCacheDaTests.cs: Add COM cleanup in OneTimeTearDown and TearDown
- RespellingTests.cs: Use real Mediator instead of Mock (sealed class)
- SCTextEnumTests.cs: Migrate from Rhino.Mocks to Moq
- PUAInstallerTests.cs: Use DistFiles relative to SourceDirectory

These fixes address test failures discovered during VSTest migration
without reintroducing the swapped assertion arguments from the
original buggy conversion.
- Add spec-kit agent framework (.GitHub/agents/*.agent.md)
- Fix config-aware intermediate paths (Output/$(Configuration)/Common/)
  to prevent Debug/Release build conflicts
- Add Build/Agent/Setup-InstallerBuild.ps1 for installer prerequisites
- Enable TreatWarningsAsErrors across managed projects
- Remove debug code dumping FieldWorks.targets to console
- Add test.ps1 for unified test execution
@johnml1135 johnml1135 force-pushed the 007-wix-314-installer branch from e8d5a9e to 6a83eac Compare December 4, 2025 20:45
@github-actions
Copy link

github-actions bot commented Dec 4, 2025

Agent analysis is currently a stub and does not execute prompts.
Intended workflow: run .github/prompts/test-failure-debug.prompt.md in analysis-only mode and post findings.
See .github/option3-plan.md for enablement steps.

@github-actions
Copy link

github-actions bot commented Dec 4, 2025

⚠️ Commit Message Format Issues ⚠️
commit dce32b9ed3:
1: T1 Title exceeds max length (73>72): "fix(build): sync Configuration and config properties, update output paths"
10: B1 Line exceeds max length (81>80): " - Prevents accidental mismatches like /p:Configuration=Debug /p:config=release"

commit 33e714fdf6:
17: B1 Line exceeds max length (83>80): "- Lib/src/ScrChecks/ScrChecksTests/SentenceFinalPunctCapitalizationCheckUnitTest.cs"

commit 74336bbbf8:
2: B4 Second line is not empty: "* As well as for NativeBuild"

commit b6c86feed5:
44: B1 Line exceeds max length (88>80): "- Paratext8Plugin.csproj: Paratext.LexicalContracts → removed (provided by ParatextData)"

…aths

The build system has two configuration properties that must stay in sync:
- Configuration: MSBuild property for managed projects (Debug, Release)
- config: Legacy property for native C++ makefiles

This commit:

1. Auto-derive config from Configuration when not explicitly set
   - Prevents accidental mismatches like /p:Configuration=Debug /p:config=release
   - Validates they match when both are specified, with clear error message

2. Update vcxproj files to use configuration-specific paths
   - Kernel.vcxproj: Output\Common → Output\$(Configuration)\Common
   - views.vcxproj: same pattern
   - TestViews.vcxproj: same pattern
   - Fixes IntelliSense paths when switching Debug/Release

3. Fix regen_midl.cmd
   - Remove hardcoded worktree path
   - Accept optional Configuration parameter (default: Debug)
   - Auto-detect VS 2022 Community or BuildTools installation
- Suppress NU1503 warnings during NuGet restore using
  DisableWarnForInvalidRestoreProjects=true (vcxproj files don't use
  NuGet restore, which is expected behavior)
- Demote MSB3277/MSB3243 (package version conflicts) and MSB3568
  (duplicate .resx entries) to messages via MSBuildWarningsAsMessages
- Demote optional tool warnings to low-priority messages:
  - "xsltc.exe not found" (optional XSL precompilation)
  - "Missing Palaso/Chorus native files" (optional Windows interop DLLs)
- Add dotnet to conflicting process cleanup to prevent MSB3026/MSB3027
  file lock errors during parallel builds
- Add ProjectReference from ViewsInterfaces to NativeBuild.csproj to
  enforce build ordering even when building projects directly

Ref: https://stackoverflow.com/a/76173374
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants