Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated most libraries to latest versions #193

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Conversation

tombogle
Copy link
Collaborator

@tombogle tombogle commented Oct 28, 2024

This especially addresses some issues with archiving as well as a vulnerability in System.Text.Json


This change is Reviewable

Lots of minor refactoring and code cleanup.
Fixed AppSmokeTests that have been failing for years.
Replaced work-around in FileType to deal with circulardependency error in autofac with use of Lazy<Func<...
Moved control instantiation and initialization into designer file for PersonContributionEditor
… with failing ElementGrid tests

# Conflicts:
#	SayMore.sln.DotSettings
#	src/SayMore/Model/Session.cs
#	src/SayMore/Model/SessionContribution.cs
#	src/SayMore/UI/ComponentEditors/PersonContributionEditor.cs
#	src/SayMoreTests/model/SessionTests.cs
@tombogle tombogle added the dependencies Pull requests that update a dependency file label Oct 28, 2024
@tombogle tombogle self-assigned this Oct 28, 2024
… of SILReleaseTasksProps to happen after installation of the package
…sure the package is present when setting those up
@tombogle tombogle requested a review from andrew-polk October 29, 2024 04:53
@tombogle
Copy link
Collaborator Author

I believe all the coding changes are done. Still a draft, pending release of libpalaso, so we can update to use an official release version.

Copy link
Contributor

@andrew-polk andrew-polk left a comment

Choose a reason for hiding this comment

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

Reviewed 84 of 86 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tombogle)


src/SayMore/UI/ComponentEditors/ContributorsEditor.cs line 68 at r2 (raw file):

			SetComponentFile(file);

			if (personInformant != null)

Was this removal of a null check supposed to get an elvis operator?

@tombogle
Copy link
Collaborator Author

tombogle commented Nov 4, 2024

src/SayMore/UI/ComponentEditors/ContributorsEditor.cs line 68 at r2 (raw file):

Previously, andrew-polk wrote…

Was this removal of a null check supposed to get an elvis operator?

No, because it already cannot be null. It is used several lines about with no null check.

Copy link
Contributor

@andrew-polk andrew-polk 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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @tombogle)


src/SayMore/UI/ComponentEditors/ContributorsEditor.cs line 68 at r2 (raw file):

Previously, tombogle (Tom Bogle) wrote…

No, because it already cannot be null. It is used several lines about with no null check.

Hm. Thought I had looked for that, but apparently I missed it.

@tombogle
Copy link
Collaborator Author

tombogle commented Jan 8, 2025

Note to self: Need to add System.IO.Compression.dll to the Installer.

Copy link
Contributor

@andrew-polk andrew-polk left a comment

Choose a reason for hiding this comment

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

Reviewed 10 of 10 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @tombogle)


src/SayMore/Model/Project.cs line 791 at r3 (raw file):

		public IEnumerable<string> GetSessionFilesToArchive(Type typeOfArchive, CancellationToken cancellationToken)
		{
			// RAMP packages must not be compressed or RAMP can't read them.

I think this comment goes with the subsequent line.


src/SayMore/Model/Project.cs line 792 at r3 (raw file):

		{
			// RAMP packages must not be compressed or RAMP can't read them.
			var tempZipFilePath = ChangeExtension(GetTempFileName(), "zip");

Is it necessary to change the extension?
I'm sure it doesn't hurt anything other than an extra IO call.


src/SayMoreTests/packages.config line 47 at r3 (raw file):

  <package id="System.Drawing.Common" version="6.0.0" targetFramework="net462" />
  <package id="System.IO.Compression" version="4.3.0" targetFramework="net462" />
  <package id="System.IO.Compression.ZipFile" version="4.3.0" targetFramework="net462" />

Based on the fact that you didn't have to make any related code changes in the Tests project, I'm guessing this isn't actually a dependency?


src/SayMore/packages.config line 9 at r1 (raw file):

  <package id="Coroutine.NET" version="1.4.0" targetFramework="net462" />
  <package id="DialogAdapters.Gtk2" version="0.1.11" targetFramework="net462" />
  <package id="DotNetZip" version="1.16.0" targetFramework="net462" />

Just verifying this isn't needed as a dependency of libpalaso?

Copy link
Collaborator 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: all files reviewed, 4 unresolved discussions (waiting on @andrew-polk)


src/SayMore/packages.config line 9 at r1 (raw file):

Previously, andrew-polk wrote…

Just verifying this isn't needed as a dependency of libpalaso?

Not anymore. (It is still used in the current released version of ParatextData, so programs that depend on that have to live with it a little longer.)


src/SayMore/Model/Project.cs line 791 at r3 (raw file):

Previously, andrew-polk wrote…

I think this comment goes with the subsequent line.

Done.


src/SayMore/Model/Project.cs line 792 at r3 (raw file):

Previously, andrew-polk wrote…

Is it necessary to change the extension?
I'm sure it doesn't hurt anything other than an extra IO call.

GetTempFileName actually creates the file, so we either have to change the extension or delete the file. CreateFromDirectory doesn't have a param to tell it to overwrite it. Maybe deleting it would be better, as we wouldn't leave a junk temp file laying around, but Windows takes care of cleaning them up.


src/SayMoreTests/packages.config line 47 at r3 (raw file):

Previously, andrew-polk wrote…

Based on the fact that you didn't have to make any related code changes in the Tests project, I'm guessing this isn't actually a dependency?

"Unable to uninstall 'System.IO.Compression.ZipFile.4.3.0' because 'SIL.Archiving.15.0.0' depends on it."

Copy link
Contributor

@andrew-polk andrew-polk left a comment

Choose a reason for hiding this comment

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

Reviewed 10 of 10 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @tombogle)

… upgrades to other DLLs

Fixed mocks in unit test corresponding to archiving model changes
@tombogle tombogle marked this pull request as ready for review January 17, 2025 19:14
Copy link
Contributor

@andrew-polk andrew-polk left a comment

Choose a reason for hiding this comment

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

Reviewed 10 of 10 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tombogle)


src/SayMore/Model/Project.cs line 792 at r3 (raw file):

Previously, tombogle (Tom Bogle) wrote…

GetTempFileName actually creates the file, so we either have to change the extension or delete the file. CreateFromDirectory doesn't have a param to tell it to overwrite it. Maybe deleting it would be better, as we wouldn't leave a junk temp file laying around, but Windows takes care of cleaning them up.

I wasn't sure you had understood my comment based on your response, but you changed the code the way I would have.
However, now you've changed it back, so I'll explain.

You're going to move the file to be named with .zip in the end, so it doesn't matter what the extension is in the meantime, right? Or does the zip library require that it is working with a file with the .zip extension?

@tombogle tombogle marked this pull request as draft January 17, 2025 19:54
Reverted back to Segment.Analytics.CSharp v. 2.4.2.0 (failure prevented SayMore from starting)
Updated copyright dates
@tombogle tombogle marked this pull request as ready for review January 17, 2025 21:18
Copy link
Contributor

@andrew-polk andrew-polk left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 8 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tombogle)

Copy link
Contributor

@andrew-polk andrew-polk left a comment

Choose a reason for hiding this comment

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

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

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.

2 participants