Skip to content

Conversation

@tombogle
Copy link
Contributor

@tombogle tombogle commented Mar 12, 2025

No longer referencing any beta packages (e.g., L10nSharp)
Upgraded MarkDig.Signed to meet dependency requirements
Updated other packages to latest bug fix patch version for current major/minor version


This change is Reviewable

Upgraded MarkDig.Signed to meet dependency requirements
Updated other packages to latest bug fix patch version for current major/minor version
@tombogle tombogle added the dependencies Pull requests that update a dependency file label Mar 12, 2025
@tombogle tombogle requested a review from ermshiperete March 12, 2025 19:40
@tombogle tombogle self-assigned this Mar 12, 2025
@github-actions
Copy link

github-actions bot commented Mar 12, 2025

Palaso Tests

     4 files  ±    0       4 suites  ±0   16m 9s ⏱️ + 4m 6s
 4 914 tests ±    0   4 686 ✅ ±    0  228 💤 ±  0  0 ❌ ±0 
15 994 runs  +4 078  15 310 ✅ +3 856  684 💤 +222  0 ❌ ±0 

Results for commit cc60efb. ± Comparison against base commit 35ab285.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@imnasnainaec imnasnainaec left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 41 files reviewed, 2 unresolved discussions (waiting on @ermshiperete)


a discussion (no related file):
What determines which .csproj files require Markdig.Signed and which don't?


SIL.Windows.Forms.Scripture.Tests/SIL.Windows.Forms.Scripture.Tests.csproj line 17 at r1 (raw file):

    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.0.2" />
    <PackageReference Include="GitVersion.MsBuild" Version="5.11.1" PrivateAssets="all" />
    <PackageReference Include="Moq" Version="4.18.4" />

Why add Moq to SIL.Windows.Forms.Scripture.Tests when it has no tests with using Moq?

Copy link
Contributor Author

@tombogle tombogle left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 41 files reviewed, 2 unresolved discussions (waiting on @ermshiperete and @imnasnainaec)


a discussion (no related file):

Previously, imnasnainaec (D. Ror.) wrote…

What determines which .csproj files require Markdig.Signed and which don't?

The only thing in libpalaso that uses it directly (i.e., has it as a Top-level dependency) is SIL.Windows.Forms. Most of the other SIL.Windows.Forms.* DLLs reference that, so they have it as a transitive dependency. I just noticed that SIL.Media.Tests also references it, but the thing that uses it really should be a Test App, not stuck inside the DLL with the unit tests. I think I'll change that in a separate PR.


SIL.Windows.Forms.Scripture.Tests/SIL.Windows.Forms.Scripture.Tests.csproj line 17 at r1 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

Why add Moq to SIL.Windows.Forms.Scripture.Tests when it has no tests with using Moq?

It is a transitive dependency there because SIL.Scripture.Tests references it, and SIL.Windows.Forms.Scripture.Tests\ScrPassageControlTests.cs uses TestScrVers, so it requires a reference to SIL.Scripture.Tests.

Copy link
Contributor Author

@tombogle tombogle left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 41 files reviewed, 2 unresolved discussions (waiting on @ermshiperete and @imnasnainaec)


a discussion (no related file):

Previously, tombogle (Tom Bogle) wrote…

The only thing in libpalaso that uses it directly (i.e., has it as a Top-level dependency) is SIL.Windows.Forms. Most of the other SIL.Windows.Forms.* DLLs reference that, so they have it as a transitive dependency. I just noticed that SIL.Media.Tests also references it, but the thing that uses it really should be a Test App, not stuck inside the DLL with the unit tests. I think I'll change that in a separate PR.

This is the PR: #1408

Copy link
Contributor

@imnasnainaec imnasnainaec left a comment

Choose a reason for hiding this comment

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

Reviewed 15 of 41 files at r1, all commit messages.
Reviewable status: 15 of 41 files reviewed, all discussions resolved (waiting on @ermshiperete)

@tombogle
Copy link
Contributor Author

@ermshiperete , Any idea why this is causing the Appveyor build to fail or what to do about it? The versions of System.Runtime.CompilerServices.Unsafe hasn't changed. I see there are some transitive dependencies on version 4.5.3 for some of our DLLs, though not for some that are experiencing test failures. I'm reticent to turn all these into top-level dependencies, even if I knew that would solve the problem. I could add binding redirects, but we have hitherto not needed any redirects for System.Runtime.CompilerServices.Unsafe and I'd really like to understand how this change could have suddenly introduce so many failures. all may package version changes were upgrades, so how could things suddenly want a much older version of System.Runtime.CompilerServices.Unsafe.

@ermshiperete
Copy link
Member

@tombogle No idea. Could it be related to updating some of our components/packages from net6 to net8?

@tombogle
Copy link
Contributor Author

Wouldn't think so, since that has all been done and merged to master.

@imnasnainaec
Copy link
Contributor

@tombogle You could try rolling back the "Updated other packages to latest bug fix patch version for current major/minor version" changes...

@tombogle
Copy link
Contributor Author

Yeah, I can troubleshoot it by making the changes incrementally. I was just hoping someone would see the obvious answer and save me that trouble.

@tombogle
Copy link
Contributor Author

Here's what I note:

  • L10nSharp is being referenced as Version="8.0.0-beta*", but since 8.00 has been released, the stable version is the version it picks up. So I can try removing the -beta* manually. Should still build and not make any difference.
  • If I try to upgrade SIL.ReleaseTasks to Version 3.1.0, it complains about a downgrade of Markdig.Signed. I think that stems from SIL.BuildTasks having a transitive dependency on v. 0.30.3, while SIL.ReleaseTasks has a top-level dependency on 0.37.0. So in order to upgrade that, it seems I have to upgrade Markdig.Signed explicitly. I have created a PR on SIL.BuildTasks to try to consolidate the version there: Consolidated SIL.BuildTasks to use same version of Markdig.Signed as SIL.ReleaseTasks SIL.BuildTasks#74
  • After making the above changes, I get two errors building: Could not copy the file "C:\Projects\libpalaso\output\Debug\net8.0\SIL.WritingSystems.Tests.runtimeconfig.json" because it was not found. and Could not copy the file "C:\Projects\libpalaso\output\Debug\net8.0\SIL.WritingSystems.Tests.deps.json" because it was not found. If I clean and then rebuild, those errors go away. I am vaguely suspicious that those errors might be related to the Appveyor test failures, but I can't draw any obvious connection.

…dency update of Markdig.Signed).

This is a test to see if upgrading just those will succeed on Appveyor build
@tombogle
Copy link
Contributor Author

#1410 demonstrates that removing -beta* for L10nsharp indeed is not the cause of the Appveyor problem. (That one can be merged independently.)

tombogle and others added 9 commits March 18, 2025 10:58
# Conflicts:
#	SIL.Windows.Forms.Scripture/SIL.Windows.Forms.Scripture.csproj
#	SIL.Windows.Forms.Tests/SIL.Windows.Forms.Tests.csproj
#	SIL.Windows.Forms/SIL.Windows.Forms.csproj
# Conflicts:
#	SIL.Windows.Forms.Scripture/SIL.Windows.Forms.Scripture.csproj
# Conflicts:
#	SIL.Core/SIL.Core.csproj
#	SIL.Media.Tests/SIL.Media.Tests.csproj
#	SIL.Windows.Forms.Scripture.Tests/SIL.Windows.Forms.Scripture.Tests.csproj
#	SIL.Windows.Forms.Scripture/SIL.Windows.Forms.Scripture.csproj
#	SIL.Windows.Forms.Tests/SIL.Windows.Forms.Tests.csproj
#	SIL.Windows.Forms.WritingSystems.Tests/SIL.Windows.Forms.WritingSystems.Tests.csproj
#	SIL.WritingSystems/SIL.WritingSystems.csproj
…ependencies

# Conflicts:
#	SIL.Archiving/SIL.Archiving.csproj
#	SIL.Core.Desktop/SIL.Core.Desktop.csproj
#	SIL.Core.Tests/SIL.Core.Tests.csproj
#	SIL.Core/SIL.Core.csproj
#	SIL.DblBundle.Tests/SIL.DblBundle.Tests.csproj
#	SIL.DblBundle/SIL.DblBundle.csproj
#	SIL.DictionaryServices/SIL.DictionaryServices.csproj
#	SIL.Lexicon/SIL.Lexicon.csproj
#	SIL.Lift/SIL.Lift.csproj
#	SIL.Linux.Logging/SIL.Linux.Logging.csproj
#	SIL.Media/SIL.Media.csproj
#	SIL.Scripture.Tests/SIL.Scripture.Tests.csproj
#	SIL.Scripture/SIL.Scripture.csproj
#	SIL.TestUtilities/SIL.TestUtilities.csproj
#	SIL.Windows.Forms.Archiving/SIL.Windows.Forms.Archiving.csproj
#	SIL.Windows.Forms.DblBundle/SIL.Windows.Forms.DblBundle.csproj
#	SIL.Windows.Forms.GeckoBrowserAdapter/SIL.Windows.Forms.GeckoBrowserAdapter.csproj
#	SIL.Windows.Forms.Keyboarding/SIL.Windows.Forms.Keyboarding.csproj
#	SIL.Windows.Forms.Scripture.Tests/SIL.Windows.Forms.Scripture.Tests.csproj
#	SIL.Windows.Forms.Scripture/SIL.Windows.Forms.Scripture.csproj
#	SIL.Windows.Forms.Tests/SIL.Windows.Forms.Tests.csproj
#	SIL.Windows.Forms.WritingSystems.Tests/SIL.Windows.Forms.WritingSystems.Tests.csproj
#	SIL.Windows.Forms.WritingSystems/SIL.Windows.Forms.WritingSystems.csproj
#	SIL.Windows.Forms/SIL.Windows.Forms.csproj
#	SIL.WritingSystems.Tests/SIL.WritingSystems.Tests.csproj
#	SIL.WritingSystems/SIL.WritingSystems.csproj
#	TestApps/SIL.Windows.Forms.TestApp/SIL.Windows.Forms.TestApp.csproj
#	build/Palaso.proj
@tombogle tombogle enabled auto-merge March 26, 2025 21:04
Copy link
Contributor

@imnasnainaec imnasnainaec left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 41 files at r1, 1 of 31 files at r3, 39 of 39 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ermshiperete)

@tombogle tombogle merged commit f309259 into master Mar 27, 2025
11 of 12 checks passed
@tombogle tombogle deleted the update-dependencies branch March 27, 2025 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Pull requests that update a dependency file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants