Migration to .NET Standard#221
Conversation
…cts. This was necessary in VS2022, since there is currently no NUnit extension.
This will allow it to target frameworks other than .NET Framework, like .NET Core. This is the first and most isolated project in this solution -- the others will follow.
This will allow it to target frameworks other than .NET Framework, like .NET Core. This required a new version of SIL.IdlImporter, and it now compiles with a warning (from IdlImporter) regarding an inner reference to a custom AntLr package that still targets 4.6.1. At this point, it's just a hope that everything stills works.
This will allow it to target frameworks other than .NET Framework, like .NET Core. This is contrast to the other projects at Standard 2.0, as this one required a few attributes for the auto-generated Kernel.cs file. I'm not sure yet what this means for the dependencies still on Framework (there are a slew of build warnings).
This will allow it to target frameworks other than .NET Framework, like .NET Core. This is a dual target with .NET Framework now. Because of the inner dependency by LCModel.Core on 2.1, all projects that reference it must use the same dual target. This compiles with multiple warnings, and it's hard to tell if any need to be heeded.
This will allow it to target frameworks other than .NET Framework, like .NET Core. This is a dual target with .NET Framework now. Because of the inner dependency by LCModel.Core on 2.1, all projects that reference it must use the same dual target. This compiles with many warnings (for Framework dependencies) as they build up through the various referenced projects.
In trying to determine why IdlImporter and other references are no longer being copied to the artifacts folder, I noticed this flag was removed during the .NET Standard upgrade. Restoring it here, as it's one less difference between old and new.
This is necessary with .NET Standard now to copy over dependent assemblies (like IDLImporter.dll) to the output folder, as Framework did.
Oddly enough, had to keep the NHunspell reference, as well. With the .Patched only, the C# symbols were never able to be resolved, but the .Patched is still necessary to copy/provide the two x64/x86 DLL's to the output folder.
This involved: Upgrading Build.Tasks to the latest version of IdlImporter, hopefully for the last time. Re-adding Icu4c*.Lib package back, as it was pre-upgrade. Fixing OutDir property to be framework-agnostic. The last thing to fix (I believe) is the GenerateIcuData task, which is failing because the Icu4c*.Lib .props isn't being included for some reason.
…h project as needed. This was necessary to build in .NET Standard. With the global property, some .props files weren't being loaded (e.g. Icu4c.Win.Fw.Lib.props), causing build errors in Core. On removal, all .props load as expected, and three projects required a TargetFramework to build. The original comment mentions needing TargetFrameworks, but it's not clear if this was because of the three projects (which I fixed) or something else.
…to fix TestHelper build. Due to WarningsAsErrors in Directory.build.props
papeh
left a comment
papeh
left a comment
There was a problem hiding this comment.
seems reasonable so far.
Reviewed 1 of 15 files at r1, all commit messages.
Reviewable status: 1 of 15 files reviewed, 1 unresolved discussion (waiting on @josephmyers and @papeh)
src/SIL.LCModel/SIL.LCModel.csproj, line 87 at r1 (raw file):
<PropertyGroup> <OutDir Condition="'$(OutDir)' == ''">../../artifacts/$(Configuration)/$(TargetFramework)/</OutDir>
Nice. This will make things more maintainable. 😎
Code quote:
$(TargetFramework)This was necessary with the move to .NET Standard, because the libraries were being built to a different folder (netstandard2.0) than the executables (net461). This was messing up the various custom build tasks (mainly in .Core) that involved copying assemblies to the build folder. At this point all unit tests (Windows) are passing.
papeh
left a comment
papeh
left a comment
There was a problem hiding this comment.
Reviewed 1 of 4 files at r2, all commit messages.
Reviewable status: 1 of 16 files reviewed, 1 unresolved discussion (waiting on @josephmyers and @papeh)
src/SIL.LCModel/SIL.LCModel.csproj, line 87 at r1 (raw file):
Previously, papeh wrote…
Nice. This will make things more maintainable. 😎
Sorry, I meant that as a compliment. Using the variable instead of hard-coding means we won't need to update this later.
I don't see a need to remove the target framework from the path; it can be useful when different artifacts target different frameworks. Although if there is a good reason to remove it from the path, I'm open to it.
This makes all test projects dependent on .Core, since .Utils.Tests is used by all of them. This change was necessary to fix the build for test projects when .Core hasn't already been built. Beforehand, as projects built asynchronously, System.CodeDom for .NET Standard would be copied to the artifacts folder (by .Build.Tasks) initially, but as the build for .Core followed, which requires that lib for its GenerateKernelCs build task, the build for .Utils.Tests would overwrite the System.CodeDom lib with its .NET Framework 4.6.1 copy. This was causing GenerateKernelCs to fail, unless of course .Core was built separately from the Test lib's. It should be noted that upgrading the Test lib's to .NET 6 does not fix the issue -- it appears that GenerateKernelCs can only use the .NET Standard 2.0 lib. But isn't that the whole point of .NET Standard?
|
src/SIL.LCModel/SIL.LCModel.csproj, line 87 at r1 (raw file): Previously, papeh wrote…
Yeah, I agree. I identified this as an issue when running the unit tests, with which I eventually realized that, with the .NET Standard move, output lib's were being copied to two different folders. This was causing various missing DLL problems exposed at runtime. Take a look at the corresponding commit message for more info! |
This resolves the "first-time" custom build task exception, which was preventing builds from succeeding on the server.
I missed this, because running Clean Solution doesn't actually clean the solution. It leaves obj folders (including old, cached files) untouched.
Hopefully this fixes the build server error.
This had been removed by the .NET Standard migration tool for some reason.
|
I'm highlighting here to me the most likely (if any) things to cause issues:
|
There was a problem hiding this comment.
Reviewed 6 of 15 files at r1, 2 of 4 files at r2, 1 of 1 files at r3, 6 of 9 files at r6, all commit messages.
Reviewable status: 16 of 19 files reviewed, 1 unresolved discussion (waiting on @megahirt and @papeh)
src/SIL.LCModel/DomainServices/WritingSystemServices.cs, line 98 at r6 (raw file):
var tpb = TsStringUtils.MakePropsBldr(); tpb.SetIntPropValues((int)FwTextPropType.ktptForeColor, (int)FwTextPropVar.ktpvDefault, (int)ColorUtil.ConvertColorToBGR(Color.FromArgb(unchecked((int) 0xFF716F64))));
This is a curious change...
@josephmyers explained to me that KnownColor is not part of .Net Standard, so he needed to provide a KnownColor.ControlDarkDark equivalent.
|
Thanks Jason!
…On Wed, Mar 16, 2022, 11:01 AM Jason Naylor ***@***.***> wrote:
***@***.**** commented on this pull request.
I'm happy with this, we just need to do an integration test with
FieldWorks now.
Reviewed 1 of 1 files at r11, all commit messages.
*Reviewable
<https://reviewable.io/reviews/sillsdev/liblcm/221#-:-MyFsXzK619a698uVoYN:b-rd54jr>*
status: all files reviewed, 1 unresolved discussion (waiting on
@ermshiperete <https://github.com/ermshiperete>)
—
Reply to this email directly, view it on GitHub
<#221 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA2I6KN4CH5SYFTKPFP6SE3VAFMIPANCNFSM5PDEXAWA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
This appears to be unused -- CustomIcuFallbackTests still pass, and that was the only reference to it. This is one less dependency to worry about when upgrading things (outside LCM) to .NET Standard etc., but it's also good cleanup for LCM.
josephmyers
left a comment
josephmyers
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: 19 of 21 files reviewed, 1 unresolved discussion (waiting on @ermshiperete and @jasonleenaylor)
josephmyers
left a comment
josephmyers
left a comment
There was a problem hiding this comment.
Reviewed 5 of 15 files at r1, 3 of 4 files at r2, 1 of 1 files at r3, 4 of 9 files at r6, 1 of 1 files at r8, 1 of 1 files at r11, 2 of 2 files at r12.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ermshiperete and @jasonleenaylor)
josephmyers
dismissed
jasonleenaylor’s stale review
Problem fixed! Please re-review
only necessary while waiting on PR, but if it's useful we can also keep it
updating to latest upstream and adding GH workflow
Update to latest master
Per https://sil-language-software.slack.com/archives/CGDKZ7NTV/p1654076175738199, I was apparently able to "dotnet build" locally without this, but I don't see how. Could've been totally off that day. This change allows dev's to run "dotnet build -c Release src/SIL.LCModel/SIL.LCModel.csproj" from cmd, which is what the GHA workflow is doing. It's necessary, because a typical .NET SDK install doesn't have an actual msbuild.exe, so the custom steps to build GenerateKernelCs.proj and GenerateModel.proj would fail. This change technically assumes you've got "dotnet.exe" in your PATH.
The latter is cross-platform, where the former isn't. See dotnet/msbuild#3175 for more information. Note that this functionality requires VS 15.8 or later. This change allows "dotnet build -c Release" to pass from cmd (it is a little flaky, just like the IDE build, but re-running enough times will fix it).
The FieldWorks build is broken with this commit: rolling back temporarily to release 9.1.12, will re-apply and work out a fix in our next sprint. This reverts commit d22427c.
The FieldWorks build is broken with this commit: rolling back temporarily to release 9.1.12, will re-apply and work out a fix in our next sprint. This reverts commit d22427c.
The FieldWorks build is broken with this commit: rolling back temporarily to release 9.1.12, will re-apply and work out a fix in our next sprint. This reverts commit d22427c.
6 participants
This change is