Adding Framework 4.8 and .NET8 to targets.

[josephmyers]

This required platform-specific references in the project files. I also fixed a warning about Assembly.CodeBase and some .NET8 warnings about manually copying duplicate files into our nuget package folder when packing. These files appear to be duplicates of what is copied during the .NET8 packaging.

The only other change worth noting involves the "unknown language" detection in L10nCultureInfo. It appears, per the link listed in the comment at that point in the code, that our method of determining an unknown language no longer works, starting with Win10 (combined with .NET). The change I made should be compatible with older OS's, like Win7, but I don't have an easy way to confirm this.

---

This change is 

[github-actions]

Test Results

    3 files  +    2    84 suites  +56   16s ⏱️ +10s
165 tests ±    0  160 ✔️ ±    0    5 💤 ±  0  0 ❌ ±0 
495 runs  +330  480 ✔️ +320  15 💤 +10  0 ❌ ±0 

Results for commit d567f2e. ± Comparison against base commit 5545cb6.

♻️ This comment has been updated with latest results.

[megahirt]

so far so good, Joseph! It looks like the strategy we're using at this point is to have conditional dependencies based upon the target build.

Reviewed 7 of 7 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @josephmyers)

[josephmyers]

I need to lodge a version bump with semvar

[megahirt]

LGTM! Going forward, I am a proponent of not building L10NSharp on Ubuntu Mono since the purpose is for use in .Net Framework applications which are Windows only.

Reviewed 6 of 6 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @andrew-polk)

[andrew-polk]

Going forward, I am a proponent of not building L10NSharp on Ubuntu Mono since the purpose is for use in .Net Framework applications which are Windows only.

I'm certainly no expert, but doesn't mono work with .NET Framework?
@ermshiperete

Reviewed 6 of 7 files at r1, 5 of 6 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @josephmyers)

---

src/L10NSharp/L10NCultureInfo.cs line 39 at r3 (raw file):

			// Starting with Windows 10, unknown cultures no longer return a RawCultureInfo containing an "Unknown Language" indication.
			// The proper way to detect fully unknown cultures, for Windows 10 and prior, is to:

Something in this comment is confusing me.
Is it supposed to be "for Windows 10 and later"?

---

src/L10NSharp/L10NCultureInfo.cs line 43 at r3 (raw file):

			//    2. Check if the three-letter language name is set to default
			var isFullyUnknown = RawCultureInfo.CultureTypes.HasFlag(CultureTypes.UserCustomCulture) && RawCultureInfo.ThreeLetterWindowsLanguageName == "ZZZ";
			if (RawCultureInfo == null || isFullyUnknown)

To maintain backward compatibility, should we still include the "Unknown Language" check?
I guess maybe we don't care...

---

src/L10NSharp/L10NSharp.csproj line 13 at r3 (raw file):

    <Reference Include="System.Windows.Forms" />
  </ItemGroup>
  <ItemGroup Condition=" '$(TargetFramework)' == 'net461' Or '$(TargetFramework)' == 'net481' ">

Maybe this was the result of an automated change, but the result is that the four items in this group are duplicated in two different conditional groups. (When they could just be unconditional.)

---

src/L10NSharpTests/L10NCultureInfoTests.cs line 45 at r3 (raw file):

				Assert.AreEqual("pbu", pbuci.RawCultureInfo.Name);
				Assert.IsTrue(pbuci.RawCultureInfo.CultureTypes.HasFlag(CultureTypes.UserCustomCulture));
				Assert.AreEqual("ZZZ", pbuci.RawCultureInfo.ThreeLetterWindowsLanguageName);

What is pbuci.RawCultureInfo.EnglishName now? Still something reasonable?

[josephmyers]

Yes, Mono will only work with .NET Framework, and we're moving away from that, as discussed in the .NET8 migration Slack channel. Granted, we're not there yet, hence the three frameworks!

Reviewable status: 11 of 12 files reviewed, 4 unresolved discussions (waiting on @andrew-polk and @megahirt)

---

src/L10NSharp/L10NCultureInfo.cs line 39 at r3 (raw file):

Previously, andrew-polk wrote…

Something in this comment is confusing me.
Is it supposed to be "for Windows 10 and later"?

Windows 10 and prior is correct. I wanted to be backwards compatible, so I did some research (in lieu of testing on an old OS), with this as my primary finding:
https://stackoverflow.com/a/71388328/1964319

---

src/L10NSharp/L10NCultureInfo.cs line 43 at r3 (raw file):

Previously, andrew-polk wrote…

To maintain backward compatibility, should we still include the "Unknown Language" check?
I guess maybe we don't care...

Ah, see above comment. The new check theoretically covers both cases.

---

src/L10NSharp/L10NSharp.csproj line 13 at r3 (raw file):

Previously, andrew-polk wrote…

Maybe this was the result of an automated change, but the result is that the four items in this group are duplicated in two different conditional groups. (When they could just be unconditional.)

Thanks, that's a good catch!

---

src/L10NSharpTests/L10NCultureInfoTests.cs line 45 at r3 (raw file):

Previously, andrew-polk wrote…

What is pbuci.RawCultureInfo.EnglishName now? Still something reasonable?

It's not consistent between frameworks. For Framework, it's the original "Unknown Language (pbu)" value, and for .NET8, it's just "pbu." The check I'm using works the same between the two.

[andrew-polk]

Reviewed 1 of 6 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @josephmyers)

---

.github/workflows/CI-CD.yml line 25 at r4 (raw file):

    strategy:
      matrix:
        os: [windows-latest, ubuntu-latest]

Sorry; I'm not clear why we are dropping ubuntu at this time.
And I wouldn't be comfortable doing so without @ermshiperete or someone else in that realm approving it.

---

.github/workflows/CI-CD.yml line 48 at r4 (raw file):

    - name: Test (Windows)
      # specify path to test assembly as workaround for https://github.com/nunit/nunit3-vs-adapter/issues/1040
      run: dotnet test --configuration Release --no-build output\Release\net461\*Tests.dll -- NUnit.TestOutputXml=TestResults

For the 4.6.1 tests, I think with my change to master, we should be able to have those run again.
Does that mean you have to put back the output\Release\net461\*Tests.dll?

[andrew-polk]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @josephmyers)

---

.github/workflows/CI-CD.yml line 48 at r4 (raw file):

Previously, andrew-polk wrote…

For the 4.6.1 tests, I think with my change to master, we should be able to have those run again.
Does that mean you have to put back the output\Release\net461\*Tests.dll?

But that would have to be conditional, obviously...

[andrew-polk]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @josephmyers)

---

src/L10NSharp/L10NCultureInfo.cs line 39 at r3 (raw file):

Previously, josephmyers wrote…

Windows 10 and prior is correct. I wanted to be backwards compatible, so I did some research (in lieu of testing on an old OS), with this as my primary finding:
https://stackoverflow.com/a/71388328/1964319

Ok, I'll try to talk out my confusion, and you can help me see what I'm missing.

First, your comment states that "Starting with Win 10..." but then continues with what seems to be a "therefore, we're changing it to this other thing to handle Win10 and earlier."

Stating what I think is the same confusion but from the perspective of the code change,
it seems that we previously used "Unknown Language" for this check, and you've corrected it for older OSes. But now I'm not understanding how we are handling it for newer OSes.

[josephmyers]

Reviewable status: 11 of 12 files reviewed, 3 unresolved discussions (waiting on @andrew-polk, @ermshiperete, and @megahirt)

---

.github/workflows/CI-CD.yml line 25 at r4 (raw file):

Previously, andrew-polk wrote…

Sorry; I'm not clear why we are dropping ubuntu at this time.
And I wouldn't be comfortable doing so without @ermshiperete or someone else in that realm approving it.

@megahirt did approve it. Maybe I should be super clear here with him and confirm he's aware of this particular change?

Keep in mind that eberhard has mentioned previously (in Slack) that he's intentionally taking less of a role over these repo's, so I don't think we should rely on decisions from him. Of course his input is valued. Right now Chris is the authoritative word on this code, since he's taking the responsibility of maintaining it.

---

.github/workflows/CI-CD.yml line 48 at r4 (raw file):

Previously, andrew-polk wrote…

But that would have to be conditional, obviously...

Yes, thanks for that fix! I tried to get them all running again, but even with the downgrade it is proving nontrivial. Removing this allows the 4.8.1 and .NET8 tests to run, which @megahirt deemed acceptable. He confirmed again today.

I also messed around with the wildcard some, but without any luck. If you know of another path, I'm absolutely open to it. But it doesn't appear to be very highly valuable, given Framework 4.8.1 is working.

---

src/L10NSharp/L10NCultureInfo.cs line 39 at r3 (raw file):

Previously, andrew-polk wrote…

Ok, I'll try to talk out my confusion, and you can help me see what I'm missing.

First, your comment states that "Starting with Win 10..." but then continues with what seems to be a "therefore, we're changing it to this other thing to handle Win10 and earlier."

Stating what I think is the same confusion but from the perspective of the code change,
it seems that we previously used "Unknown Language" for this check, and you've corrected it for older OSes. But now I'm not understanding how we are handling it for newer OSes.

Sure, sorry that my comments are not clear enough.

So, the previous code was working for OS's older than Windows 10. Apparently, Windows 10 changed its behavior for CultureInfo, so we need to note that here. However, research showed that my fix is backwards compatible with older OS's and equivalent (superior) to the previous code, so I wanted to capture that as a comment here, as well.

Of course, that code could change (again) in future Windows versions, so I wouldn't want to put some sort of assumption/guarantee here that it won't. And if it does, I want whomever comes here to understand why I made this change.

Maybe the primary source of confusion arises from my use of "Starting with...," as if I know what's going to happen in the future? I updated the comment to hopefully be clearer.

The climactic argument this change makes is: "The previous way of doing things worked for Win7 and older, but the new way works for Win10 and older." Hopefully this all helps!

[andrew-polk]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ermshiperete, @josephmyers, and @megahirt)

---

.github/workflows/CI-CD.yml line 25 at r4 (raw file):

Previously, josephmyers wrote…

@megahirt did approve it. Maybe I should be super clear here with him and confirm he's aware of this particular change?

Keep in mind that eberhard has mentioned previously (in Slack) that he's intentionally taking less of a role over these repo's, so I don't think we should rely on decisions from him. Of course his input is valued. Right now Chris is the authoritative word on this code, since he's taking the responsibility of maintaining it.

See comment about 4.6.1 tests.

---

.github/workflows/CI-CD.yml line 48 at r4 (raw file):

Previously, josephmyers wrote…

Yes, thanks for that fix! I tried to get them all running again, but even with the downgrade it is proving nontrivial. Removing this allows the 4.8.1 and .NET8 tests to run, which @megahirt deemed acceptable. He confirmed again today.

I also messed around with the wildcard some, but without any luck. If you know of another path, I'm absolutely open to it. But it doesn't appear to be very highly valuable, given Framework 4.8.1 is working.

Sorry; I definitely don't want to be the bottleneck to you moving forward on this.
If @megahirt has thought through the implications and wants to take responsibility for any fallout, I don't think it is my place to stand in the way.
But I do think we have a philosophical difference. In my mind, we should either say we support 4.6.1 and mono or we don't. If we say we support it, I don't think we should be removing anything (within reason) which "proves" we support it.

Feel free to merge.

---

src/L10NSharp/L10NCultureInfo.cs line 39 at r3 (raw file):

Previously, josephmyers wrote…

Sure, sorry that my comments are not clear enough.

So, the previous code was working for OS's older than Windows 10. Apparently, Windows 10 changed its behavior for CultureInfo, so we need to note that here. However, research showed that my fix is backwards compatible with older OS's and equivalent (superior) to the previous code, so I wanted to capture that as a comment here, as well.

Of course, that code could change (again) in future Windows versions, so I wouldn't want to put some sort of assumption/guarantee here that it won't. And if it does, I want whomever comes here to understand why I made this change.

Maybe the primary source of confusion arises from my use of "Starting with...," as if I know what's going to happen in the future? I updated the comment to hopefully be clearer.

The climactic argument this change makes is: "The previous way of doing things worked for Win7 and older, but the new way works for Win10 and older." Hopefully this all helps!

I'm still not sure I get it, but I'm going to get out of the way.

If it is a helpful question, I think what I still don't get is, What does Win11 do?
Or are you considering Win11 to be part of Win10? If yes, that would definitely clear up the confusion.
Anyway, feel free to answer in the PR or a comment. Or neither.

[megahirt]

Reviewed all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ermshiperete and @josephmyers)

---

.github/workflows/CI-CD.yml line 48 at r4 (raw file):

Previously, andrew-polk wrote…

Sorry; I definitely don't want to be the bottleneck to you moving forward on this.
If @megahirt has thought through the implications and wants to take responsibility for any fallout, I don't think it is my place to stand in the way.
But I do think we have a philosophical difference. In my mind, we should either say we support 4.6.1 and mono or we don't. If we say we support it, I don't think we should be removing anything (within reason) which "proves" we support it.

Feel free to merge.

You've got a good point, @andrew-polk . When I surveyed all developers on #common-libraries, no one could say that we are still actively supporting/shipping software on linux/mono. Thus, it seems safe to me to remove complexity in our shared libraries like this one that has been added for linux/mono support. Reducing complexity will help a lot for any future effort to move toward .Net 8 Windows and .Net cross-platform.

@ermshiperete should have a voice here. Can you comment?

I can see how Framework 4.6.1 and Mono are tied together for you - we've done all the work, why shouldn't we continue to keep it alive? I guess for the reason above, I don't see a compelling reason to keep the extra complexity around.

As long as we have a branch that represents the 4.6.1 / mono shipping version, which can stick around forever IMO, we should feel free to move forward with just 4.8.1 on Windows only. This PR represents forward work toward the 4.8.1 future that is Windows only.

I will also look into making these changes as part of a new long-term branch for .net8

[rmunn]

I just checked the Microsoft support site, and Framework versions 4.7.1, 4.7.2, and even 4.8 all have offline installers available for Windows versions as old as Windows 7 SP1. E.g., https://support.microsoft.com/en-us/topic/microsoft-net-framework-4-8-offline-installer-for-windows-9d23f658-3b97-68ab-d013-aa3c3e7495e0 lists supported Windows versions as Windows 7 SP1, Windows 8.1, Windows 10 version 1607 or later, plus a lot of Windows Server versions. Microsoft's announcement of 4.8.1 claimed that it was supported on Windows 10 and up, not mentioning Windows 7. I have found references elsewhere to Framework 4.8.1 being available for Windows 7, but I haven't been able to prove it. (I may still have a Win7 VM image I can dig up and test that; I'll report my results later).

Therefore, I'd suggest that it might be worth adjusting this PR to target .Net Framework 4.8 rather than 4.8.1. This would still allow libraries to move to .Net Standard 2.0 and be easy to consume from both .Net Framework and .Net 8.0 apps, while still allowing us to produce releases for users who, for one reason or another, are still stuck on older versions of Windows: our software that needs .Net Framework can offer a version with the 4.8 offline installer packaged with it if necessary.

[josephmyers]

That's an easy change. Thanks for your input

Reviewable status: 5 of 12 files reviewed, 3 unresolved discussions (waiting on @andrew-polk, @ermshiperete, and @megahirt)

---

.github/workflows/CI-CD.yml line 25 at r4 (raw file):

Previously, andrew-polk wrote…

See comment about 4.6.1 tests.

Thanks

---

.github/workflows/CI-CD.yml line 48 at r4 (raw file):

Previously, megahirt (Christopher Hirt) wrote…

You've got a good point, @andrew-polk . When I surveyed all developers on #common-libraries, no one could say that we are still actively supporting/shipping software on linux/mono. Thus, it seems safe to me to remove complexity in our shared libraries like this one that has been added for linux/mono support. Reducing complexity will help a lot for any future effort to move toward .Net 8 Windows and .Net cross-platform.

@ermshiperete should have a voice here. Can you comment?

I can see how Framework 4.6.1 and Mono are tied together for you - we've done all the work, why shouldn't we continue to keep it alive? I guess for the reason above, I don't see a compelling reason to keep the extra complexity around.

As long as we have a branch that represents the 4.6.1 / mono shipping version, which can stick around forever IMO, we should feel free to move forward with just 4.8.1 on Windows only. This PR represents forward work toward the 4.8.1 future that is Windows only.

I will also look into making these changes as part of a new long-term branch for .net8

We have been discussing a branching strategy for libpalaso here. I was not planning on applying that to L10nSharp, since other than the 4.6.1 tests and Ubuntu, everything is living happily together. Are you proposing we implement the branching strategy for L10nSharp, as well? That will fundamentally change this PR, and future work on this repo specifically, but it would allow us to more explicitly support the theoretical 4.6.1/Mono userbase.

Maybe these changes would be more appealing if I were actually able to get the ubuntu build to work? This would be regardless of whether we want 4.6.1 on the master branch or an eventual framework branch. I will investigate some more.

(I'm not confident about getting all three sets of tests to run.)

---

src/L10NSharp/L10NCultureInfo.cs line 39 at r3 (raw file):

Previously, andrew-polk wrote…

I'm still not sure I get it, but I'm going to get out of the way.

If it is a helpful question, I think what I still don't get is, What does Win11 do?
Or are you considering Win11 to be part of Win10? If yes, that would definitely clear up the confusion.
Anyway, feel free to answer in the PR or a comment. Or neither.

Yes, thanks, I should include Win11 in this. I'll update the comment.

Another thing we could do, if we want to guarantee support for older OS's, is add specific versions of Windows to the CI. I can look into this, as well

[josephmyers]

Reviewable status: 4 of 12 files reviewed, 3 unresolved discussions (waiting on @andrew-polk, @ermshiperete, and @megahirt)

---

.github/workflows/CI-CD.yml line 48 at r4 (raw file):

Previously, josephmyers wrote…

We have been discussing a branching strategy for libpalaso here. I was not planning on applying that to L10nSharp, since other than the 4.6.1 tests and Ubuntu, everything is living happily together. Are you proposing we implement the branching strategy for L10nSharp, as well? That will fundamentally change this PR, and future work on this repo specifically, but it would allow us to more explicitly support the theoretical 4.6.1/Mono userbase.

Maybe these changes would be more appealing if I were actually able to get the ubuntu build to work? This would be regardless of whether we want 4.6.1 on the master branch or an eventual framework branch. I will investigate some more.

(I'm not confident about getting all three sets of tests to run.)

So I was able to get the Ubuntu build reinstated. I had to remove the tests for that OS, since they were failing (Framework 4.6.1 and 4.8), but on master those tests aren't doing anything anyway. They are "succeeding" in 2 seconds with empty output. So it's clearer to remove the build step. We can open a separate issue to add this feature, if desired.

[josephmyers]

Reviewable status: 4 of 12 files reviewed, 2 unresolved discussions (waiting on @andrew-polk, @ermshiperete, and @megahirt)

---

src/L10NSharp/L10NCultureInfo.cs line 39 at r3 (raw file):

Previously, josephmyers wrote…

Yes, thanks, I should include Win11 in this. I'll update the comment.

Another thing we could do, if we want to guarantee support for older OS's, is add specific versions of Windows to the CI. I can look into this, as well

After a little research, the oldest windows runner supported by GHA is windows-2019. Doesn't really seem super valuable to add that to the list on top of windows-latest, so I recommend we close this out.

[josephmyers]

Given the numerous changes here surrounding platforms and their tests, I made some super simple tables that lay out who all is running what.

Master:

	Windows	Ubuntu
Framework 4.6.1	Yes	-
Framework 4.8	-	-
.NET8	-	-

This PR:

	Windows	Ubuntu
Framework 4.6.1	Yes	-
Framework 4.8	Yes	-
.NET8	Yes	-

[hahn-kev]

just checked this out, ran the tests and it all seems to work correctly. Can we get this merged in?

[hahn-kev]

Reviewed 1 of 7 files at r1, 3 of 6 files at r3, 8 of 8 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ermshiperete and @megahirt)

[andrew-polk]

Reviewed 8 of 8 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ermshiperete)

[josephmyers]

@hahn-kev I don't think this will solve #116, but it might get you closer.