Remove obsolete MSBuild wrapper scripts in favor of dotnet build - #364
Remove obsolete MSBuild wrapper scripts in favor of dotnet build#364jasonleenaylor wants to merge 2 commits into
Conversation
|
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. |
|
Can this be enforced? Can the write commands only be accessible through these two commands? |
|
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? |
|
Is this something specific to this repo? Why is it best recorded here? |
|
Does this section add value? We probably need the test script called out (if there is one). Or make a testing.instructions.md file. |
|
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... |
|
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). |
|
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? |
|
Is it best practice to put these here? Should they be grouped with the actual relevant data? |
|
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. |
|
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". |
|
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.). |
|
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. |
|
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? |
|
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 |
johnml1135
left a comment
There was a problem hiding this comment.
@johnml1135 reviewed 6 files and all commit messages.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @jasonleenaylor).
jasonleenaylor
left a comment
There was a problem hiding this comment.
@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)
|
Converted the task focused documents to skills. Thinking of this as an alpha version of agent support in the repository. |
|
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? |
|
Previously, johnml1135 (John Lambert) wrote…
I see the git comment - does that match your understanding as to what happened? |
|
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>
a06c52f to
e27b8c3
Compare
|
@johnml1135 — answering your two comments on 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
The actual defect: Verified with 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 So I've split this PR. This one now keeps only the build-script removal, the ICU fix, and one The |
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>
Removes
build.cmd,build.shandbuild/LCM.proj. These wrapped MSBuild againstLCM.sln; CI has useddotnet build/test/packdirectly for some time and nothing else inthe repo referenced them, so they only offered a second, diverging way to build.
README.mdandAGENTS.mdnow namedotnet buildinstead. TheAGENTS.mdedits aredeletions plus one added note - the rewrite is a separate PR.
Drops the premature
Wrapper.Init()call inInitializeIcuAttribute. It ran beforePATHheld the
libdirectories and beforeICU_DATAwas set, so the first native ICU load hadnothing 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()whileguarding
InitIcuDataDir(); corrected.Relaxes
global.jsonfromrollForward: latestFeaturetolatestMajor. The8.0.100pinrefuses 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:1for cold-start builds. The removed scripts calledmsbuildwithout/m,so they were implicitly serial;
dotnet buildis parallel by default. On a tree with nogenerated sources,
GenerateModelandGenerateKernelCscan be invoked concurrentlyagainst 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
InitializeIcupass on net462 andnet8.0:
SIL.LCModel.Core.TestsSIL.LCModel.FixData.TestsSIL.LCModel.TestsICU verified with
ICU_DATAunset, no ICU entries onPATH, noSoftware\SIL\Icu70DataDirregistry value and no%ProgramData%\SIL\Icu70directory.This change is