Joseph's dotnet6 - With some adjustments #238

jasonleenaylor · 2022-07-07T21:21:40Z

This change is

ermshiperete · 2022-07-08T08:43:37Z

src/SIL.LCModel.Build.Tasks/SIL.LCModel.Build.Tasks.csproj

  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="GitVersion.MsBuild" Version="5.6.10" PrivateAssets="All" />
    <PackageReference Include="Microsoft.SourceLink.GitHub" Version="1.0.0" PrivateAssets="All" />
    <PackageReference Include="NVelocity" Version="1.2.0" PrivateAssets="All" />
-    <PackageReference Include="SIL.IdlImporter" Version="2.0.0" PrivateAssets="All" />
+    <PackageReference Include="SIL.IdlImporter" Version="2.0.1-beta0009" />


I just created a release for SIL.IdlImporter, so we should depend on that. Do we need to have the dependency on SIL.IdlImporter in the nuget package, or should PrivateAssets=All be re-added?

ermshiperete · 2022-07-08T08:46:44Z

src/SIL.LCModel.Core/GenerateKernelCs.proj

@@ -23,15 +23,17 @@

 		<UsingNamespaces Include="SIL.LCModel.Utils" />
 	</ItemGroup>
-	<Target Name="GenerateKernelCs">
+	<Target Name="GenerateKernelCs" Inputs="@(KernelIdhFiles>" Outputs="$(OutDir)KernelInterfaces/FwKernelTlb.json">
+		<Copy SourceFiles="C:/Windows/Microsoft.NET/Framework64/v4.0.30319/Microsoft.Build.Utilities.v4.0.dll"


That's not a good idea to have an absolute path in the .proj file. It has the potential to break on other machines and isn't cross-platform compatible.

@josephmyers @papeh
Oh yes, I forgot about this little hack. And we didn't discuss it in our meeting this morning. We need a different solution to this before we can merge this PR. 😄

@jasonleenaylor Where do we need it? Can we update the component that needs it?

jasonleenaylor · 2022-07-08T14:43:19Z

src/SIL.LCModel.Build.Tasks/SIL.LCModel.Build.Tasks.csproj line 17 at r8 (raw file):

Previously, ermshiperete (Eberhard Beilharz) wrote…

I just created a release for SIL.IdlImporter, so we should depend on that. Do we need to have the dependency on SIL.IdlImporter in the nuget package, or should PrivateAssets=All be re-added?

We can probably re-add the PrivateAssets=All, this was an experiment to try and solve the issue resolving Microsoft.Build.Utilities before I gave up all hope and added the hack of directly copying it. Now that we know this build works on all windows developer scenarios I hope to find a more principled solution to that problem.

jasonleenaylor · 2022-07-08T14:57:37Z

src/SIL.LCModel.Core/GenerateKernelCs.proj line 27 at r8 (raw file):

Previously, ermshiperete (Eberhard Beilharz) wrote…

@jasonleenaylor Where do we need it? Can we update the component that needs it?

The IdlImp build task needs it. My best guess as to why this fails is that when we start multi-targeting we lose assembly loading from the GAC during the build.

jasonleenaylor · 2022-07-08T15:03:21Z

src/SIL.LCModel.Core/GenerateKernelCs.proj line 27 at r8 (raw file):

Previously, jasonleenaylor (Jason Naylor) wrote…

The IdlImp build task needs it. My best guess as to why this fails is that when we start multi-targeting we lose assembly loading from the GAC during the build.

Without that copy you get this, and nothing else I tried could get me around it.

error MSB4062: The "IdlImp" task could not be loaded from the assembly C:\Repositories\liblcm\/artifacts/Debug/net461\SIL.LCModel.Build.Tasks.dll. Could not load file or assembly 'Microsoft.Build.Utilities.v4.0, Version=4.0.0.0

I tried adding a specific reference to get it to copy the file and it found a reference only assembly that failed at runtime. Any and all ideas are welcome.

papeh

Reviewed 2 of 18 files at r1, 8 of 22 files at r7.
Reviewable status: 10 of 31 files reviewed, 2 unresolved discussions (waiting on @ermshiperete, @josephmyers, and @papeh)

src/SIL.LCModel.Core/GenerateKernelCs.proj line 27 at r8 (raw file):

Previously, jasonleenaylor (Jason Naylor) wrote…

Without that copy you get this, and nothing else I tried could get me around it.
error MSB4062: The "IdlImp" task could not be loaded from the assembly C:\Repositories\liblcm\/artifacts/Debug/net461\SIL.LCModel.Build.Tasks.dll. Could not load file or assembly 'Microsoft.Build.Utilities.v4.0, Version=4.0.0.0
I tried adding a specific reference to get it to copy the file and it found a reference only assembly that failed at runtime. Any and all ideas are welcome.

Check the .dll in to our repository?
Condition="'$(OS)'=='Windows_NT'"?

papeh

Reviewed 5 of 18 files at r1, 1 of 1 files at r2, 7 of 22 files at r7, 1 of 1 files at r8, 2 of 2 files at r9.
Reviewable status: 26 of 31 files reviewed, 2 unresolved discussions (waiting on @ermshiperete, @josephmyers, and @papeh)

src/SIL.LCModel.Core/SIL.LCModel.Core.csproj line 137 at r9 (raw file):

  <Target Name="GenerateIcuData" AfterTargets="CoreBuild" Inputs="@(IcuDataInputs)" Outputs="@(IcuDataOutputs)" >
    <!-- The following two errors will highlight broken build configuration situations -->

This count could be omitted. Having the text and condition in the same order for all errors would improve readability.

Code quote (i):

two

Code snippet (ii):

three

papeh

Reviewable status: 26 of 31 files reviewed, 2 unresolved discussions (waiting on @ermshiperete, @josephmyers, and @papeh)

src/SIL.LCModel.Utils/SIL.LCModel.Utils.csproj line 15 at r9 (raw file):

    <PackageReference Include="SIL.Core" Version="10.0.0-*" />
    <PackageReference Include="SIL.ReleaseTasks" Version="2.5.0" PrivateAssets="All" />
    <PackageReference Include="Newtonsoft.Json" Version="13.0.1" />

¿¿¿should FW also be updated to Netwonsoft.Json 13???

jasonleenaylor · 2022-07-08T17:47:53Z

src/SIL.LCModel.Utils/SIL.LCModel.Utils.csproj line 15 at r9 (raw file):

Previously, papeh wrote…

¿¿¿should FW also be updated to Netwonsoft.Json 13???

Hmm.
Probably.

ermshiperete · 2022-07-08T17:56:10Z

src/SIL.LCModel.Core/GenerateKernelCs.proj line 27 at r8 (raw file):
Previously, jasonleenaylor (Jason Naylor) wrote…

Without that copy you get this, and nothing else I tried could get me around it.
error MSB4062: The "IdlImp" task could not be loaded from the assembly C:\Repositories\liblcm\/artifacts/Debug/net461\SIL.LCModel.Build.Tasks.dll. Could not load file or assembly 'Microsoft.Build.Utilities.v4.0, Version=4.0.0.0
I tried adding a specific reference to get it to copy the file and it found a reference only assembly that failed at runtime. Any and all ideas are welcome.

Do we have to build SIL.LCModel.Build.Tasks for net461 or would netstandard2.0 be sufficient? (If we remove net461 we'll also have to change SIL.LCModel.Build.Tasks.props - that mentions net461 as well)

jasonleenaylor · 2022-07-08T18:44:23Z

src/SIL.LCModel.Core/GenerateKernelCs.proj line 27 at r8 (raw file):

Previously, papeh wrote…

Check the .dll in to our repository?
Condition="'$(OS)'=='Windows_NT'"?

Thanks to Eberhard's suggestion about using the netstandard2.0 version we have a new hack to avoid the copy that should also work on Linux.

jasonleenaylor · 2022-07-08T18:46:34Z

Yes we do, I'm going to have to play around with this in a Flex build today but your question led me to a much improved hack.

jasonleenaylor · 2022-07-08T18:48:12Z

src/SIL.LCModel.Build.Tasks/SIL.LCModel.Build.Tasks.csproj line 17 at r8 (raw file):

Previously, jasonleenaylor (Jason Naylor) wrote…

We can probably re-add the PrivateAssets=All, this was an experiment to try and solve the issue resolving Microsoft.Build.Utilities before I gave up all hope and added the hack of directly copying it. Now that we know this build works on all windows developer scenarios I hope to find a more principled solution to that problem.

Done.

papeh

when angle bracket is rounded

Reviewed 1 of 8 files at r6, 3 of 22 files at r7, 1 of 1 files at r10, 2 of 2 files at r11.
Reviewable status: all files reviewed (commit messages unreviewed), 3 unresolved discussions (waiting on @ermshiperete, @jasonleenaylor, and @josephmyers)

src/SIL.LCModel.Core/GenerateKernelCs.proj line 26 at r11 (raw file):

		<UsingNamespaces Include="SIL.LCModel.Utils" />
	</ItemGroup>
	<Target Name="GenerateKernelCs" Inputs="@(KernelIdhFiles>" Outputs="$(OutDir)KernelInterfaces/FwKernelTlb.json">

Code quote (i):

@(KernelIdhFiles>

Code snippet (ii):

@(KernelIdhFiles)

jasonleenaylor

Dismissed @ermshiperete from a discussion.
Reviewable status: 30 of 31 files reviewed, 2 unresolved discussions (waiting on @josephmyers and @papeh)

src/SIL.LCModel.Core/GenerateKernelCs.proj line 26 at r11 (raw file):

		<UsingNamespaces Include="SIL.LCModel.Utils" />
	</ItemGroup>
	<Target Name="GenerateKernelCs" Inputs="@(KernelIdhFiles>" Outputs="$(OutDir)KernelInterfaces/FwKernelTlb.json">

Done.

papeh

Reviewed 1 of 1 files at r12.
Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @josephmyers)

josephmyers

Reviewed 6 of 18 files at r1, 1 of 1 files at r2, 18 of 22 files at r7, 1 of 1 files at r8, 1 of 2 files at r9, 2 of 2 files at r11, 1 of 1 files at r12, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ermshiperete)

papeh

Reviewed 5 of 7 files at r13, 2 of 2 files at r14, 1 of 1 files at r15, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ermshiperete and @jasonleenaylor)

.github/workflows/ci-cd.yml line 38 at r14 (raw file):

    - name: Install NUnit.ConsoleRunner
      run: nuget install NUnit.ConsoleRunner -Version 3.13.0 -DirectDownload -OutputDirectory .

nit: trailing whitespace

tests/SIL.LCModel.Utils.Tests/SIL.LCModel.Utils.Tests.csproj line 21 at r15 (raw file):

  </ItemGroup>
  <ItemGroup Condition="'$(TargetFramework)' == 'net461'">
    <PackageReference Include="Mono.Posix" Version="5.4.0.201" PrivateAssets="All" />

DRY code makes me happy 😄

papeh

Reviewed 24 of 28 files at r21, 2 of 3 files at r22, 1 of 2 files at r24, 1 of 5 files at r31, 4 of 4 files at r32, all commit messages.
Dismissed @ermshiperete from 3 discussions.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ermshiperete and @jasonleenaylor)

.github/workflows/ci-cd.yml line 16 at r19 (raw file):

Previously, ermshiperete (Eberhard Beilharz) wrote…

So we don't want to build on Linux?

The current revision builds on both platforms, as desired.

.github/workflows/ci-cd.yml line 44 at r19 (raw file):

Previously, ermshiperete (Eberhard Beilharz) wrote…

Would dotnet test work here?

Done

.github/workflows/ci-cd.yml line 38 at r25 (raw file):

Previously, ermshiperete (Eberhard Beilharz) wrote…

We'll have to add llso for icu-fw to be available. That can be done with:

wget -qO - http://linux.lsdev.sil.org/downloads/sil-testing.gpg | sudo tee /etc/apt/trusted.gpg.d/linux-lsdev-sil-org.asc
sudo add-apt-repository "deb http://linux.lsdev.sil.org/ubuntu $(lsb_release -sc)-experimental main"

Done. Thanks.

.github/workflows/ci-cd.yml line 54 at r25 (raw file):

Previously, ermshiperete (Eberhard Beilharz) wrote…

Shouldn't this be artifacts/*.nupkg ?

      run: dotnet nuget push artifacts/*.nupkg -s https://api.nuget.org/v3/index.json -k ${{secrets.SILLSDEV_PUBLISH_NUGET_ORG}} --skip-duplicate

Done

.github/workflows/ci-cd.yml line 66 at r31 (raw file):

        name: NugetPackages
        path: artifacts/*.nupkg
      if: github.event_name == 'pull_request'

Does this publish artifacts from pull requests? It could use a comment explaining what it does and why it is needed.

ermshiperete

We should remove appveyor.yml file

ermshiperete · 2022-07-18T16:17:44Z

.github/workflows/ci-cd.yml

+
+    - name: Publish to Nuget
+      run: dotnet nuget push artifacts/*.nupkg -s https://api.nuget.org/v3/index.json -k ${{secrets.SILLSDEV_PUBLISH_NUGET_ORG}} --skip-duplicate
+      if: github.event_name == 'push' && matrix.os == 'ubuntu-latest'


Do we have to check the branch name as well so that we don't publish to nuget on a push to a feature branch?

ermshiperete · 2022-07-18T16:20:49Z

src/SIL.LCModel.Utils/MiscUtils.cs

+					               || appPath.IndexOf("jetbrains", StringComparison.Ordinal) != -1
+					               || appPath.IndexOf("testhost", StringComparison.Ordinal) != -1;
+
+					Console.WriteLine($"RUNNINGTESTS: {commandLine} - {appPath}");


Is this line intentional or a remnant of getting things to build? If it is intentional we should probably change it to Debug.WriteLine.

papeh

Reviewable status: 36 of 38 files reviewed, 5 unresolved discussions (waiting on @ermshiperete, @jasonleenaylor, @josephmyers, and @papeh)

.github/workflows/ci-cd.yml line 59 at r32 (raw file):

Previously, ermshiperete (Eberhard Beilharz) wrote…

Do we have to check the branch name as well so that we don't publish to nuget on a push to a feature branch?

That's checked on lines 5-8

papeh

Dismissed @ermshiperete from a discussion.
Reviewable status: 36 of 38 files reviewed, 4 unresolved discussions (waiting on @ermshiperete, @jasonleenaylor, @josephmyers, and @papeh)

src/SIL.LCModel.Utils/MiscUtils.cs line 845 at r32 (raw file):

Previously, ermshiperete (Eberhard Beilharz) wrote…

Is this line intentional or a remnant of getting things to build? If it is intentional we should probably change it to Debug.WriteLine.

gone

ermshiperete

Reviewed 2 of 2 files at r33, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jasonleenaylor)

papeh

Reviewed 2 of 2 files at r33, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jasonleenaylor)

ermshiperete requested changes Jul 8, 2022

View reviewed changes

papeh reviewed Jul 8, 2022

View reviewed changes

jasonleenaylor commented Jul 8, 2022

View reviewed changes

ermshiperete approved these changes Jul 8, 2022

View reviewed changes

papeh approved these changes Jul 8, 2022

View reviewed changes

josephmyers approved these changes Jul 11, 2022

View reviewed changes

jasonleenaylor force-pushed the net6fix-jn branch 4 times, most recently from e89820b to bae1b9e Compare July 12, 2022 19:06

papeh approved these changes Jul 12, 2022

View reviewed changes

jasonleenaylor force-pushed the net6fix-jn branch 8 times, most recently from 53a9e0b to d279e29 Compare July 12, 2022 22:08

papeh force-pushed the net6fix-jn branch from fe44046 to e44297a Compare July 15, 2022 18:46

test output

3a2523a

papeh force-pushed the net6fix-jn branch from 5fd8a4f to 3a2523a Compare July 15, 2022 19:16

Rigorous Tests

e7b2a5c

papeh force-pushed the net6fix-jn branch from e96d37a to 1a11245 Compare July 15, 2022 20:05

More helpful test output, clean up TestHelper

2de34e6

papeh force-pushed the net6fix-jn branch 7 times, most recently from 5577f45 to ec3e8e3 Compare July 18, 2022 15:11

Permanent test for lurking ICU DLL's

1b2f4af

papeh force-pushed the net6fix-jn branch from ec3e8e3 to 1b2f4af Compare July 18, 2022 15:19

Fix CustomIcuFallbackTests

15e848d

papeh force-pushed the net6fix-jn branch from b3aada7 to 15e848d Compare July 18, 2022 15:31

papeh added 2 commits July 18, 2022 10:54

clean up after myself

cff2173

clean up dead code

1532c43

papeh reviewed Jul 18, 2022

View reviewed changes

ermshiperete requested changes Jul 18, 2022

View reviewed changes

more cleanup

e397ef0

papeh reviewed Jul 18, 2022

View reviewed changes

ermshiperete approved these changes Jul 18, 2022

View reviewed changes

papeh approved these changes Jul 18, 2022

View reviewed changes

papeh merged commit e8dceaa into develop Jul 18, 2022

papeh deleted the net6fix-jn branch July 18, 2022 18:57

papeh mentioned this pull request Jul 27, 2022

Re-Add support for netstandard2.0 framework #236

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Joseph's dotnet6 - With some adjustments #238

Joseph's dotnet6 - With some adjustments #238

jasonleenaylor commented Jul 7, 2022 •

edited by ddaspit

Loading

ermshiperete Jul 8, 2022

ermshiperete Jul 8, 2022

jasonleenaylor Jul 8, 2022

ermshiperete Jul 8, 2022

jasonleenaylor commented Jul 8, 2022

jasonleenaylor commented Jul 8, 2022

jasonleenaylor commented Jul 8, 2022

papeh left a comment

papeh left a comment

papeh left a comment

jasonleenaylor commented Jul 8, 2022

ermshiperete commented Jul 8, 2022 •

edited

Loading

jasonleenaylor commented Jul 8, 2022

jasonleenaylor commented Jul 8, 2022

jasonleenaylor commented Jul 8, 2022

papeh left a comment

jasonleenaylor left a comment

papeh left a comment

josephmyers left a comment

papeh left a comment

papeh left a comment

ermshiperete left a comment

ermshiperete Jul 18, 2022

ermshiperete Jul 18, 2022

papeh left a comment

papeh left a comment

ermshiperete left a comment

papeh left a comment

Joseph's dotnet6 - With some adjustments #238

Joseph's dotnet6 - With some adjustments #238

Conversation

jasonleenaylor commented Jul 7, 2022 • edited by ddaspit Loading

ermshiperete Jul 8, 2022

Choose a reason for hiding this comment

ermshiperete Jul 8, 2022

Choose a reason for hiding this comment

jasonleenaylor Jul 8, 2022

Choose a reason for hiding this comment

ermshiperete Jul 8, 2022

Choose a reason for hiding this comment

jasonleenaylor commented Jul 8, 2022

jasonleenaylor commented Jul 8, 2022

jasonleenaylor commented Jul 8, 2022

papeh left a comment

Choose a reason for hiding this comment

papeh left a comment

Choose a reason for hiding this comment

papeh left a comment

Choose a reason for hiding this comment

jasonleenaylor commented Jul 8, 2022

ermshiperete commented Jul 8, 2022 • edited Loading

jasonleenaylor commented Jul 8, 2022

jasonleenaylor commented Jul 8, 2022

jasonleenaylor commented Jul 8, 2022

papeh left a comment

Choose a reason for hiding this comment

jasonleenaylor left a comment

Choose a reason for hiding this comment

papeh left a comment

Choose a reason for hiding this comment

josephmyers left a comment

Choose a reason for hiding this comment

papeh left a comment

Choose a reason for hiding this comment

papeh left a comment

Choose a reason for hiding this comment

ermshiperete left a comment

Choose a reason for hiding this comment

ermshiperete Jul 18, 2022

Choose a reason for hiding this comment

ermshiperete Jul 18, 2022

Choose a reason for hiding this comment

papeh left a comment

Choose a reason for hiding this comment

papeh left a comment

Choose a reason for hiding this comment

ermshiperete left a comment

Choose a reason for hiding this comment

papeh left a comment

Choose a reason for hiding this comment

jasonleenaylor commented Jul 7, 2022 •

edited by ddaspit

Loading

ermshiperete commented Jul 8, 2022 •

edited

Loading