Create nuget packages #786
Conversation
This is now done by GitVersion.
This will set the ReleaseNotes property in the created nuget package.
Corrected confusing localization comment
Apps can now override the default password if needed and also specify the support page URL.
This change replaces the use of CultureAndRegionInfoBuilder, which doesn't exist in Mono, with a call to LanguageLookup. This should make the code also a bit more robust in the case of some neutral cultures that it looks like we didn't properly handle previously, e.g. "en". This change also adds unit tests for the `WritingSystemFromWindowsLocaleProvider.GetRegion` method which verify that the method still works in the same way as before (with the few cases that behave differently mentioned in the comments in the test).
|
The irrKlang stuff looks okay, but it doesn't really seem to belong in this PR |
Palaso.sln
Outdated
| {5DE33CD7-60CB-4B9F-A123-A83C1C686E47}.DebugStrongName|x64.Build.0 = DebugStrongName|x64 | ||
| {5DE33CD7-60CB-4B9F-A123-A83C1C686E47}.DebugStrongName|x86.ActiveCfg = DebugStrongName|x86 | ||
| {5DE33CD7-60CB-4B9F-A123-A83C1C686E47}.DebugStrongName|x86.Build.0 = DebugStrongName|x86 | ||
| {5DE33CD7-60CB-4B9F-A123-A83C1C686E47}.Debug|x64.ActiveCfg = Debug|Any CPU |
There was a problem hiding this comment.
If I understand this correctly, we're essentially "flattening" this by making all these different configurations resolve to the same thing so that the only distinction is between Debug/Release. If that is correct, why not just delete all the redundant configurations? Is that just to keep from breaking stuff on TC (and maybe individual developers' processes)? In the long run, all these configurations are going to lend themselves to more confusion.
There was a problem hiding this comment.
yes, eventually I'll delete those. Just haven't gotten that far yet. I should have applied the WIP label on this PR 😄
|
|
||
| if (!string.IsNullOrEmpty(withApplicationDirectory) && File.Exists(withApplicationDirectory)) | ||
| return withApplicationDirectory; | ||
| if (!string.IsNullOrEmpty(withApplicationDirectory) && File.Exists(withApplicationDirectory)) |
There was a problem hiding this comment.
Just below this line is a comment: "nb: this is sensitive to whether we are compiled against win32 or not, not just the host OS, as you might guess.
Is that (still) true? If so, what happens now that we are not doing conditional compilation?
There was a problem hiding this comment.
Ahh, thanks. I didn't see that comment.
I don't understand this comment - it doesn't seem to match what the code is doing. Depending on whether we run as 32- or 64-bit process we get a different value for ProgramFiles. But then we look in both "Program Files" and "Program Files (x86)" for ffmpeg.exe and take whatever we find. I don't think this change changed anything how the method works. Correct me if I miss something.
Yes, it doesn't really fit in this PR, but it requires an interface change with an increment in version number, which I don't want to do separately. |
Now that I've had a chance to digest this whole thing, I guess I kind of see why/how the irrKlang stuff had to be included as part of the whole removal of conditional compilation for Mono/Windows. |
…Url a publicly settable property instead of getting it indirectly from config file.
Improved the password notice in the settings protection dialog.
This change replaces the use of CultureAndRegionInfoBuilder, which doesn't exist in Mono, with a call to LanguageLookup. This should make the code also a bit more robust in the case of some neutral cultures that it looks like we didn't properly handle previously, e.g. "en". This change also adds unit tests for the `WritingSystemFromWindowsLocaleProvider.GetRegion` method which verify that the method still works in the same way as before (with the few cases that behave differently mentioned in the comments in the test).
The namespace got changed in the big refactoring, but some mentions in strings were missed...
These files are no longer needed now that we use alltags.json.
Also remove some files that were not included in the old .csproj files (in SIL.Archiving) - I don't know if they are still needed, but since they were not included in the old .csproj files they were not available previously, and nobody seems to have missed them, so I decided to remove them.
Allow to compile Windows- and Linux-only code on both platforms. +semver: major
The namespace got changed in the big refactoring, but some mentions in strings were missed...
|
One other thing that needs to be adjusted is the default configuration in the |
There was a problem hiding this comment.
Reviewed 13 of 21 files at r20.
Reviewable status: 185 of 298 files reviewed, 6 unresolved discussions (waiting on @ermshiperete, @jasonleenaylor, and @tombogle)
build/platform.targets, line 4 at r20 (raw file):
<!-- Include this file at the end of a csproj file (just before the include of Microsoft.CSharp.targets) to add system-specific defines
It seems we no longer need this file - The constants it defines are used only in Tests, and are used to run certain tests only on Mac. Seems we should use PlatformAttribute instead to run certain tests only on certain platforms.
SIL.Core/Progress/ProgressState.cs, line 7 at r20 (raw file):
//NOTE: this "ProgressState" system is deprecated. Newer clients use the extensive SIL.Progress.IProgress classes, often along with the logbox. namespace SIL.Progress
This comment makes me wonder if we should add an Obsolete attribute to this class.
Move IAudioPlayer, IAudioRecorder, IRecordingDevice, RecordingState to SIL.Media namespace.
This allows to successfully build the solution on Ubuntu.
This is needed for Chorus tests.
Some commented-out code still used the old Palaso namespaces, and would cause compilation errors if it was ever uncommented and used.
We no longer have StrongName builds nor separate Mono builds. This change modifies the scripts to build the Debug or Release configuration instead.
There was a problem hiding this comment.
One other thing that needs to be adjusted is the default configuration in the build/TestBuild.sh script
Done. Thanks, @rmunn !
Reviewable status: 172 of 301 files reviewed, 6 unresolved discussions (waiting on @ermshiperete, @jasonleenaylor, and @tombogle)
There was a problem hiding this comment.
I haven't finished reviewing the whole thing, but I do have some concerns that need discussion. See my comments on GitVersion.yml for details.
Reviewed 1 of 233 files at r4, 1 of 220 files at r6, 1 of 5 files at r8, 52 of 246 files at r14, 2 of 5 files at r17, 18 of 24 files at r19, 1 of 22 files at r21, 18 of 21 files at r22, 3 of 3 files at r23.
Reviewable status: 264 of 301 files reviewed, 7 unresolved discussions (waiting on @ermshiperete, @jasonleenaylor, and @tombogle)
GitVersion.yml, line 23 at r23 (raw file):
tag: alpha # Tell gitversion to use a version number >= next-version next-version: 6.1.0
How are we planning to handle versioning in the NuGet packages? With multiple NuGet packages being built from one repo, GitVersion is going to end up assigning the same version number to all of them, right? Which means that the libpalaso "family" of NuGet packages would have version numbers that increase in lockstep. But my memory of the discussion we had in February is that we did NOT want lockstep package numbering, but rather package numbering that increases on a per-package basis. I.e., the first release of all the packages will have version 6.1.0; fine. But after that, when SIL.Media gets a 6.2.0 release because it added a feature requiring a minor version bump, we don't want SIL.Core to end up with a 6.2.0 release at the same time, where SIL.Core 6.2.0 would be the same as SIL.Core 6.1.0. As I recall, that was really hard with GitVersion.
https://medium.com/@gparlakov/gitversion-in-a-multi-project-repo-ac23edff6e2e has a report of using GitVersionTfsTask to build multiple projects from one repo, but that relies on an environment variable that's apparently checked by GitVersionTfsTask but NOT by GitVersionTask. (Note how https://github.com/GitTools/GitVersion/blob/00b99518f8625930a6f6420d515acd36c4dc5c9a/src/GitVersionTfsTask/GitVersion.ts contains code that checks Build.SourcesDirectory but https://github.com/GitTools/GitVersion/blob/00b99518f8625930a6f6420d515acd36c4dc5c9a/src/GitVersionTask/GetVersion.cs does not). The only solutions I have found so far for getting different version numbers out of a single repo involve other tools than GitVersion, sadly.
Unfortunately, I think this is a blocker until we can be sure that we get the versioning story right.
|
@rmunn It's true that we decided:
But we also said:
(see end of Libpalaso Breakup document). IMO it makes sense to merge the changes to create nuget packages, and then make it right in a later change. Lock-step versions is no worse than what we currently have. |
There was a problem hiding this comment.
I'll remove my "blocking" status since I'm about to disappear for a couple of weeks and I don't want to block this getting merged while I'm away. I'm willing to accept using GitVersion for the initial release, but I really want a solution that can do per-project versioning really soon after initial release (as in, we'd either teach GitVersion to do per-project versioning, or else use something like https://github.com/AArnott/Nerdbank.GitVersioning/ which is what Microsoft is using for versioning their .Net Core packages). BTW, see dotnet/Nerdbank.GitVersioning#160 which will be highly relevant to our situation with LibPalaso if we decide to use that project. I don't have time to write more about it at the moment, but I'll be happy to discuss this idea further once I'm back online in mid-July.
Reviewable status: 264 of 301 files reviewed, 6 unresolved discussions (waiting on @ermshiperete, @jasonleenaylor, and @tombogle)
GitVersion.yml, line 23 at r23 (raw file):
Previously, rmunn (Robin Munn) wrote…
How are we planning to handle versioning in the NuGet packages? With multiple NuGet packages being built from one repo, GitVersion is going to end up assigning the same version number to all of them, right? Which means that the libpalaso "family" of NuGet packages would have version numbers that increase in lockstep. But my memory of the discussion we had in February is that we did NOT want lockstep package numbering, but rather package numbering that increases on a per-package basis. I.e., the first release of all the packages will have version 6.1.0; fine. But after that, when SIL.Media gets a 6.2.0 release because it added a feature requiring a minor version bump, we don't want SIL.Core to end up with a 6.2.0 release at the same time, where SIL.Core 6.2.0 would be the same as SIL.Core 6.1.0. As I recall, that was really hard with GitVersion.
https://medium.com/@gparlakov/gitversion-in-a-multi-project-repo-ac23edff6e2e has a report of using GitVersionTfsTask to build multiple projects from one repo, but that relies on an environment variable that's apparently checked by GitVersionTfsTask but NOT by GitVersionTask. (Note how https://github.com/GitTools/GitVersion/blob/00b99518f8625930a6f6420d515acd36c4dc5c9a/src/GitVersionTfsTask/GitVersion.ts contains code that checks
Build.SourcesDirectorybut https://github.com/GitTools/GitVersion/blob/00b99518f8625930a6f6420d515acd36c4dc5c9a/src/GitVersionTask/GetVersion.cs does not). The only solutions I have found so far for getting different version numbers out of a single repo involve other tools than GitVersion, sadly.Unfortunately, I think this is a blocker until we can be sure that we get the versioning story right.
Retracting my "blocking" status so I don't prevent this being merged while I'm offline. See https://github.com/AArnott/Nerdbank.GitVersioning for another way to do this, which I think I like better than GitVersion since it seems to allow for multiple projects in the same repo (but see dotnet/Nerdbank.GitVersioning#160 for a feature request which I'd really like to see accepted before we used Nerdbank.GitVersioning).
|
Update to my earlier: dotnet/Nerdbank.GitVersioning#102 makes me not exactly a fan of how GitVersioning does things either (I strongly disagree with his opinion re: what the SemVer spec requires here). If we do use GitVersioning, we'd probably want to fork it to change that design decision. Again, I don't have time to discuss this in detail right now, unfortunately. |
There was a problem hiding this comment.
Reviewed 1 of 21 files at r22.
Reviewable status: 265 of 301 files reviewed, 6 unresolved discussions (waiting on @ermshiperete, @jasonleenaylor, and @tombogle)
There was a problem hiding this comment.
Reviewed 1 of 220 files at r6, 1 of 221 files at r7, 4 of 5 files at r8, 2 of 4 files at r9, 3 of 3 files at r13, 15 of 246 files at r14, 2 of 3 files at r18, 6 of 24 files at r19, 2 of 21 files at r22.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ermshiperete and @tombogle)
build/platform.targets, line 4 at r20 (raw file):
Previously, ermshiperete (Eberhard Beilharz) wrote…
It seems we no longer need this file - The constants it defines are used only in Tests, and are used to run certain tests only on Mac. Seems we should use
PlatformAttributeinstead to run certain tests only on certain platforms.
That seems plausible. I'm guessing Neil and team added this when they tried to get one of our tools to run on a Mac?
SIL.Core/Progress/ProgressState.cs, line 7 at r20 (raw file):
Previously, ermshiperete (Eberhard Beilharz) wrote…
This comment makes me wonder if we should add an
Obsoleteattribute to this class.
Since this is a breaking API change anyhow, now would be the time to do it.
SIL.Media/SIL.Media.targets, line 9 at r16 (raw file):
Previously, ermshiperete (Eberhard Beilharz) wrote…
I didn't find a way because the copying is done by some msbuild magic.
If it is copied by way of the ItemGroup above you can just add an Exclude attribute in the None element right? (If not this is no problem)
SIL.WritingSystems/LanguageLookup.cs, line 16 at r23 (raw file):
/// <summary> /// Lets you find a language using data from the Ethnologue, IANA subtag repository and the SLDR. /// It obtains the data from alltags.json.
Hmm, langtags.json is the current correct filename. Is this still wrong on master?
There was a problem hiding this comment.
Reviewed 6 of 252 files at r14.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @jasonleenaylor and @tombogle)
build/platform.targets, line 4 at r20 (raw file):
Previously, jasonleenaylor (Jason Naylor) wrote…
That seems plausible. I'm guessing Neil and team added this when they tried to get one of our tools to run on a Mac?
I'll add this as a separate PR once this is merged.
SIL.Core/Progress/ProgressState.cs, line 7 at r20 (raw file):
Previously, jasonleenaylor (Jason Naylor) wrote…
Since this is a breaking API change anyhow, now would be the time to do it.
Turns out that ProgressState is used quite a bit in different libpalaso assemblies, so to completely deprecate it means a lot of changes in all kinds of APIs. At least it should be a separate PR.
SIL.Media/SIL.Media.targets, line 9 at r16 (raw file):
Previously, jasonleenaylor (Jason Naylor) wrote…
If it is copied by way of the ItemGroup above you can just add an Exclude attribute in the None element right? (If not this is no problem)
Of course! Thanks!
SIL.WritingSystems/LanguageLookup.cs, line 16 at r23 (raw file):
Previously, jasonleenaylor (Jason Naylor) wrote…
Hmm, langtags.json is the current correct filename. Is this still wrong on master?
No, it's correct in master. Once we merge master into feature/nuget this will be fixed (he said optimistically 😃 )
There was a problem hiding this comment.
Reviewed 1 of 1 files at r24.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jasonleenaylor and @tombogle)
|
SIL.Archiving/Generic/AccessProtocol/ELARAccessProtocol.cs, line 1 at r14 (raw file): Previously, jasonleenaylor (Jason Naylor) wrote…
Pretty sure these were added as some kind of a false start. They would have been for SayMore and SayMore has no references to them, nor to AccessProtocolBase or IAccessProtocol (neither of which seem to exist anywhere). Should be safe to delete them. |
There was a problem hiding this comment.
Reviewed 5 of 220 files at r6, 9 of 221 files at r7, 5 of 5 files at r8, 2 of 4 files at r9, 6 of 6 files at r10, 5 of 9 files at r12, 3 of 3 files at r13, 213 of 246 files at r14, 2 of 2 files at r15, 2 of 5 files at r17, 2 of 3 files at r18, 23 of 24 files at r19, 1 of 22 files at r21, 21 of 21 files at r22, 3 of 3 files at r23, 1 of 1 files at r24.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jasonleenaylor)
*-Designer.csprojfiles for projects that implement dialogs or custom controls so that it’s possible to edit the forms/controls in Visual Studio’s designer#if MONOstatements in the code; same binary will work cross-platform.AnyCPUIRecordingDeviceinterface whichIAudioRecorder.SelectedDevicenow returns. This interface is implemented in theRecordingDeviceclass of both NAudio and AlsaAudio. Since this is a breaking change the libpalaso version number went up to 7.0.CHANGELOG.mdfile. Relevant changes and bugfixes should be documented in here. What gets added will show up as release notes on the nuget packages on nuget.org. What should go in this document is:See the Google doc for a more detailed discussion of the changes and the reasons why things were done a certain way.
This change is