Skip to content

Remove obsolete MSBuild wrapper scripts in favor of dotnet build - #364

Open
jasonleenaylor wants to merge 2 commits into
masterfrom
chore/updateAgentDocs
Open

Remove obsolete MSBuild wrapper scripts in favor of dotnet build#364
jasonleenaylor wants to merge 2 commits into
masterfrom
chore/updateAgentDocs

Conversation

@jasonleenaylor

@jasonleenaylor jasonleenaylor commented Feb 9, 2026

Copy link
Copy Markdown
Contributor

Removes build.cmd, build.sh and build/LCM.proj. These wrapped MSBuild against
LCM.sln; CI has used dotnet build/test/pack directly for some time and nothing else in
the repo referenced them, so they only offered a second, diverging way to build.
README.md and AGENTS.md now name dotnet build instead. The AGENTS.md edits are
deletions plus one added note - the rewrite is a separate PR.

Drops the premature Wrapper.Init() call in InitializeIcuAttribute. It ran before PATH
held the lib directories and before ICU_DATA was set, so the first native ICU load had
nothing to find and the environment had to be set by hand. CustomIcu.InitIcuDataDir,
called a few lines later, does both. The catch message named Wrapper.Init() while
guarding InitIcuDataDir(); corrected.

Relaxes global.json from rollForward: latestFeature to latestMajor. The 8.0.100 pin
refuses any newer SDK, blocking work on machines with no 8.0.x SDK installed. CI installs
8.0.x explicitly via setup-dotnet, so the pin was redundant there.

Documents -m:1 for cold-start builds. The removed scripts called msbuild without /m,
so they were implicitly serial; dotnet build is parallel by default. On a tree with no
generated sources, GenerateModel and GenerateKernelCs can be invoked concurrently
against the same non-per-framework outputs, and then fail as LcmGenerate / IdlImp
"returned false but did not log an error". It is intermittent, which is why CI has not
tripped over it. That behaviour predates this PR and is unchanged by it; the docs now name
the command that works.

Verification

Solution builds clean on SDK 10.0.201 and 9.0.316, both exercised against this commit in
clean worktrees. All three assemblies that declare InitializeIcu pass on net462 and
net8.0:

Assembly Result
SIL.LCModel.Core.Tests 792/792
SIL.LCModel.FixData.Tests 21/21
SIL.LCModel.Tests 1709/1727, 18 skipped

ICU verified with ICU_DATA unset, no ICU entries on PATH, no
Software\SIL\Icu70DataDir registry value and no %ProgramData%\SIL\Icu70 directory.


This change is Reviewable

@johnml1135

Copy link
Copy Markdown
Contributor

AGENTS.md line 30 at r1 (raw file):

### Prerequisites
- .NET SDK 8.x

So, this can be compiled in any of the 3 frameworks? What version of mono? Why 3? One sentence would be helpful here to understand.

@johnml1135

Copy link
Copy Markdown
Contributor

AGENTS.md line 21 at r1 (raw file):

2. **Model changes require a version bump and migration.** Almost every change to `MasterLCModel.xml` requires incrementing the `version` attribute and writing a data migration class. The ONLY exceptions are: editing `<comment>`/`<notes>` elements, editing XML comments, or adding attributes that only affect the code generator. Read the warnings at the top of `MasterLCModel.xml` carefully.

3. **All data changes must occur within a UnitOfWork.** Use `UndoableUnitOfWorkHelper` for user actions or `NonUndoableUnitOfWorkHelper` for system operations. Changes outside a UOW will throw or silently fail.

Can this be enforced? Can the write commands only be accessible through these two commands?

@johnml1135

Copy link
Copy Markdown
Contributor

AGENTS.md line 23 at r1 (raw file):

3. **All data changes must occur within a UnitOfWork.** Use `UndoableUnitOfWorkHelper` for user actions or `NonUndoableUnitOfWorkHelper` for system operations. Changes outside a UOW will throw or silently fail.

4. **No references to `System.Windows.Forms`.** The build enforces this via the `CheckWinForms` target.

Why would Windows Forms be in here at all? Why reference it if it is not in the repo? Does this need one more sentence or clause of explanation?

@johnml1135

Copy link
Copy Markdown
Contributor

AGENTS.md line 33 at r1 (raw file):

- Windows: .NET Framework 4.6.1 targeting pack
- Linux: mono-devel, icu-fw packages
- Full git history (GitVersion.MsBuild requires `git fetch --unshallow` or `fetch-depth: 0`)

Is this something specific to this repo? Why is it best recorded here?

@johnml1135

Copy link
Copy Markdown
Contributor

AGENTS.md line 37 at r1 (raw file):

### CI Commands (GitHub Actions, `.github/workflows/ci-cd.yml`)

dotnet build --configuration Release

Does this section add value? We probably need the test script called out (if there is one). Or make a testing.instructions.md file.

@johnml1135

Copy link
Copy Markdown
Contributor

AGENTS.md line 48 at r1 (raw file):

### Known Issues
- GitVersion.MsBuild requires full git metadata. Shallow clones will fail.

Can we fix this programmatically? When we start a build (and need it), can we ensure that we are not in a shallow clone in a quick way? A fix is better than a known issue...

@johnml1135

Copy link
Copy Markdown
Contributor

AGENTS.md line 49 at r1 (raw file):

### Known Issues
- GitVersion.MsBuild requires full git metadata. Shallow clones will fail.
- `NU1701` warnings are expected; treat as warnings unless the build breaks.

This should probably be in a comment in a project file rather than here. These should just be ignored programmatically with a rationale (if needed).

@johnml1135

Copy link
Copy Markdown
Contributor

AGENTS.md line 61 at r1 (raw file):

3. Processes NVelocity templates (`LcmGenerate/*.vm.cs`) to produce the 9 generated C# files

The generated code provides: class ID/field ID constants, interfaces, concrete implementations, factory interfaces and implementations, repository interfaces and implementations, the backend provider's `ModelVersion` constant, and StructureMap DI bootstrapping.

Can one more sentence be added as to why this architecture was chosen? It is unobvious to me. Is there an article that explains this paradigm? Do newer versions of DotNet make this irrelevant, but we want to still support mono? Is this a good long term strategy? If you feel we need a few paragraphs to explain, can you reference a document or wiki, etc?

@johnml1135

Copy link
Copy Markdown
Contributor

AGENTS.md line 173 at r1 (raw file):

When performing any of the following tasks, read the linked guide first:

- **Adding a property to an existing class**: [docs/agents/adding-a-property.md](docs/agents/adding-a-property.md)

Is it best practice to put these here? Should they be grouped with the actual relevant data?

@johnml1135

Copy link
Copy Markdown
Contributor

docs/agents/adding-a-property.md line 2 at r1 (raw file):

# Task: Adding a Property to an Existing Class

Are these files generated from a user guide that was fed into Claude? How have they been validated? I don't know enough to know if they are right.

@johnml1135

Copy link
Copy Markdown
Contributor

docs/agents/adding-a-property.md line 75 at r1 (raw file):

Create: `src/SIL.LCModel/DomainServices/DataMigration/DataMigration7000073.cs`

**For new optional properties with safe defaults (most common case)**, existing data doesn't need modification. But you still need the migration class:

Is this a duplication of data migration? You can ask Claude "Check for duplicated code here" "Refactor to have an appropriate amount of context for a broad scope of work".

@johnml1135

Copy link
Copy Markdown
Contributor

docs/agents/adding-a-new-class.md line 1 at r1 (raw file):

# Task: Adding a New Class to the Model

Do these show up in VSCode Copilot extension? If we want new agents, they should show up for GitHub and for claude. We should follow "https://docs.github.com/en/copilot/reference/custom-agents-configuration". These are custom agents (specific general tasks that need to follow a workflow and have certain things they can an cannot do. They may also need a bit of reformatting (head matter, etc.).

@johnml1135

Copy link
Copy Markdown
Contributor

docs/agents/adding-a-new-class.md line 3 at r1 (raw file):

# Task: Adding a New Class to the Model

This guide covers adding an entirely new class to `MasterLCModel.xml`. This is less common than adding properties and more involved -- it touches the model, requires a migration, may need an owner property on an existing class, and may need hand-written partial class logic.

You may also want a impact-assessment subagent (or similar). Sending off sub-agents to research the codebase and then come back is often effective. YOu can create a custom agents and then call it out in other agents to use as appropriate.

@johnml1135

Copy link
Copy Markdown
Contributor

docs/agents/writing-a-data-migration.md line 3 at r1 (raw file):

# Task: Writing a Data Migration

Data migrations transform existing persisted data when the model version changes. They operate on raw XML via `DomainObjectDTO` objects -- live `CmObject` instances are NOT available during migration.

This sounds very formal. Will this become the formal documentation or is it written somewhere else? Can we reference it or can this become it?

@johnml1135

Copy link
Copy Markdown
Contributor

docs/agents/writing-tests.md line 2 at r1 (raw file):

# Task: Writing Tests

Writing tests are better as skills. Skills are reusable. We should also have a debugging skill (with specific debugging paradigms for this repo).

You can have a custom agents be for writing tests, but I would put most of the logic in a skill.

Migrations should also be a skill - it's kind of cross cutting concerns.

Does the distinction make sense?

https://github.com/orgs/community/discussions/183962
https://danielmiessler.com/blog/when-to-use-skills-vs-commands-vs-agents

@johnml1135 johnml1135 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.

@johnml1135 reviewed 6 files and all commit messages.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @jasonleenaylor).

@jasonleenaylor jasonleenaylor left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@jasonleenaylor made 7 comments.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @johnml1135).


AGENTS.md line 21 at r1 (raw file):

Previously, johnml1135 (John Lambert) wrote…

Can this be enforced? Can the write commands only be accessible through these two commands?

It is enforced actually, this is just making sure the agent understands that. Actually there is better guidance I can give here. The silently fail is not accurate.


AGENTS.md line 23 at r1 (raw file):

Previously, johnml1135 (John Lambert) wrote…

Why would Windows Forms be in here at all? Why reference it if it is not in the repo? Does this need one more sentence or clause of explanation?

We specifically guard against developers who tend to add things like message boxes or other things from System.Windows.Forms (there is a build target that guards against it). The AI picked up on that, which seems fine.


AGENTS.md line 48 at r1 (raw file):

Previously, johnml1135 (John Lambert) wrote…

Can we fix this programmatically? When we start a build (and need it), can we ensure that we are not in a shallow clone in a quick way? A fix is better than a known issue...

It is a known GitVersion issue I believe, not an issue of ours.


AGENTS.md line 61 at r1 (raw file):

Previously, johnml1135 (John Lambert) wrote…

Can one more sentence be added as to why this architecture was chosen? It is unobvious to me. Is there an article that explains this paradigm? Do newer versions of DotNet make this irrelevant, but we want to still support mono? Is this a good long term strategy? If you feel we need a few paragraphs to explain, can you reference a document or wiki, etc?

If you are referring to the class id, field id constants that was to provide fast reflection and support data migration capabilities which didn't require instantiating C# classes. It has nothing to do with mono.
This markdown is specific to the agents, so I don't know that adding paragraphs to explain it is valuable.


AGENTS.md line 173 at r1 (raw file):

Previously, johnml1135 (John Lambert) wrote…

Is it best practice to put these here? Should they be grouped with the actual relevant data?

I'm not sure if this is best practice. this is what I got when I asked it to keep the context which an agent starts with as small as reasonable but to have instructions for specific common tasks with best practices. I was wondering about this and thinking they should really be 'skills' that live somewhere. I'm not sure on the best practice if there is one for vendor unlocked skills files.


docs/agents/adding-a-property.md line 2 at r1 (raw file):

Previously, johnml1135 (John Lambert) wrote…

Are these files generated from a user guide that was fed into Claude? How have they been validated? I don't know enough to know if they are right.

Yes, these files came from a combination of a model description pdf and specific code file guidance that I gave. I've mostly validated them but I was actually executing a test model change to validate some of this.


docs/agents/writing-a-data-migration.md line 3 at r1 (raw file):

Previously, johnml1135 (John Lambert) wrote…

This sounds very formal. Will this become the formal documentation or is it written somewhere else? Can we reference it or can this become it?

This is a description of how the datamigration code works and it is giving the agent instructions on how to successfully add one (I'm currently testing if these instructions actually result in successful agent action)

@jasonleenaylor
jasonleenaylor marked this pull request as ready for review February 12, 2026 21:17
@jasonleenaylor

Copy link
Copy Markdown
Contributor Author

Converted the task focused documents to skills. Thinking of this as an alpha version of agent support in the repository.

@johnml1135

Copy link
Copy Markdown
Contributor

tests/SIL.LCModel.Core.Tests/Attributes/InitializeIcuAttribute.cs line 6 at r2 (raw file):

using System;
using System.Collections.Generic;

What are these other changes for? the removal of the proj file? I am assuming you moved from the script to dotnet build. Is that all? Can you add a comment for these changes?

@johnml1135

Copy link
Copy Markdown
Contributor

tests/SIL.LCModel.Core.Tests/Attributes/InitializeIcuAttribute.cs line 6 at r2 (raw file):

Previously, johnml1135 (John Lambert) wrote…

What are these other changes for? the removal of the proj file? I am assuming you moved from the script to dotnet build. Is that all? Can you add a comment for these changes?

I see the git comment - does that match your understanding as to what happened?

@hahn-kev

hahn-kev commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

this is super helpful for human agents as well 😄

build.cmd, build.sh and build/LCM.proj wrapped MSBuild against LCM.sln. CI has
used dotnet build/test/pack directly for some time and nothing else in the repo
referenced these files, so they only offered a second, diverging way to build.
README.md and AGENTS.md now name dotnet build instead.

Drop the premature Wrapper.Init() call in InitializeIcuAttribute. It ran before
PATH held the lib directories and before ICU_DATA was set, so the first native
ICU load had nothing to find and the environment had to be set by hand.
CustomIcu.InitIcuDataDir, called a few lines later, does both. The catch message
named Wrapper.Init() while guarding InitIcuDataDir; corrected.

Relax global.json from rollForward latestFeature to latestMajor. The 8.0.100 pin
refuses any newer SDK, blocking work on machines with no 8.0.x SDK installed. CI
installs 8.0.x explicitly via setup-dotnet, so the pin was redundant there.

Verified: solution builds clean on SDK 10.0.201, and previously on 9.0.316.
SIL.LCModel.Core.Tests 792/792, SIL.LCModel.FixData.Tests 21/21 and
SIL.LCModel.Tests 1716/1716 pass on net462 and net8.0, with ICU_DATA unset, no
ICU entries on PATH, no Software\SIL\Icu70DataDir registry value and no
%ProgramData%\SIL\Icu70 directory.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown

LCM Tests

    16 files  ±0      16 suites  ±0   2m 9s ⏱️ +13s
 2 880 tests ±0   2 860 ✅ ±0   20 💤 ±0  0 ❌ ±0 
11 468 runs  ±0  11 300 ✅ ±0  168 💤 ±0  0 ❌ ±0 

Results for commit af8c5c9. ± Comparison against base commit 28e3487.

♻️ This comment has been updated with latest results.

@jasonleenaylor jasonleenaylor changed the title Rewrite AGENTS.md with architecture guide and add task docs for agents Remove obsolete MSBuild wrapper scripts in favor of dotnet build Aug 28, 2026
@jasonleenaylor

Copy link
Copy Markdown
Contributor Author

@johnml1135 — answering your two comments on InitializeIcuAttribute.cs at r2. Sorry for the long delay.

Good questions, and the answer turned out to be "no, that wasn't all" — thanks for pushing on it.

Three things were happening in that file, not one. The new SetIcuEnvironmentFromBuildOutputIfPresent helper set ICU_DATA and prepended lib paths; Wrapper.Init() was removed; and ConfineIcuVersions moved after initialization. Only the second one was load-bearing.

EnsureIcuDataEnvironmentVariableIsSet already resolves the same directory, because every test assembly declares [assembly: InitializeIcu(IcuDataPath = "IcuData")] and that takes the relative-path branch against the build output. And CustomIcu.InitIcuDataDir already prepends the same four lib directories. So the helper was doing nothing that wasn't done immediately after it — I've deleted it, and the reordering with it.

The actual defect: Wrapper.Init() ran before PATH had the lib directories and before ICU_DATA was set, so the first native ICU load had nothing to find. That's what made agents set the environment by hand. Removing that call is the whole fix — the diff against master is now +2/-10, and I've corrected the catch message that still said Wrapper.Init() while guarding InitIcuDataDir().

Verified with ICU_DATA unset, no ICU entries on PATH, no Software\SIL\Icu70DataDir registry value and no %ProgramData%\SIL\Icu70: SIL.LCModel.Core.Tests 792/792, SIL.LCModel.FixData.Tests 21/21 and SIL.LCModel.Tests 1716/1716, all on net462 and net8.0.

On the commit message — the third bullet was accurate, but it under-described the file, and the subject named the AGENTS.md trim when most of the commit was deleting build.cmd, build.sh and build/LCM.proj. That's fair criticism.

So I've split this PR. This one now keeps only the build-script removal, the ICU fix, and one global.json change; it's rebased onto current master and retitled. The AGENTS.md rewrite and the task skills will come separately, rewritten against today's tree rather than February's.

The global.json change is new since you last looked, so flagging it explicitly: rollForward goes from latestFeature to latestMajor. The 8.0.100 pin added in #394 refuses any newer SDK, which blocks work on a machine with no 8.0.x SDK installed. CI installs 8.0.x explicitly via setup-dotnet, so the pin was redundant there. Verified the solution builds clean and the suites pass on both 9.0.316 and 10.0.201.

The removed wrapper scripts called msbuild without /m, so they were
implicitly serial. dotnet build is parallel by default, and on a tree with
no generated sources yet the GenerateModel and GenerateKernelCs targets can
run concurrently against the same non-per-framework outputs, then fail as
LcmGenerate or IdlImp "returned false but did not log an error".

The failure is intermittent, so the error is easy to misread as a broken
change. README.md and AGENTS.md now name -m:1 for a cold-start build.

Correct the Windows prerequisite to Visual Studio 2022. net8.0 targets were
added in #385 and VS 2019 cannot build them. The C++ build tools are needed
for any Windows build rather than only for building in the IDE, because code
generation preprocesses the IDL with cl.exe.

Drop the AGENTS.md root-file inventory, which duplicated the repository root
and went stale with this change, and ask anyone whose run contradicts the
file to report it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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