hvanbakel / CsprojToVs2017

Tooling for converting pre 2017 project to the new Visual Studio 2017 format.
MIT License
1.08k stars 120 forks source link

Multiple AssemblyInfo should be OK #263

Open jzabroski opened 5 years ago

jzabroski commented 5 years ago

What exactly are you trying to guard against here?

In my use case, I have a Linked GlobalAssemblyInfo.cs which contains things like AssemblyCopyright, AssemblyTrademark, AssemblyVersion, AssemblyFileVersion, AssemblyInformationalVersion, AssemblyCulture, AssemblyDescription, etc.

The only things I have in my "normal" AssemblyInfo is AssemblyTitleAttribute, ComVisibleAttribute, and GuidAttribute.

https://github.com/hvanbakel/CsprojToVs2017/blob/d7040f870e304e9191e878cdd1b440bc684d1a27/Project2015To2017.Core/Reading/AssemblyInfoReader.cs#L71-L79

jzabroski commented 5 years ago

This functionality also doesn't work right.

My newly generated dotnet SDK project file generates a <ApplicationVersion>1.0.0.0</ApplicationVersion> despite also having <GenerateAssemblyInfo>false</GenerateAssemblyInfo>

I think if you disable GenerateAssemblyInfo, you shouldnt automatically choose ApplicationVersion.

jzabroski commented 5 years ago

In general, I think this feature more or less works "correctly" (if in a hacky way), in the sense that if you DO have more than one AssemblyInfo file, chances are you are doing what I am doing. The only escaped defect is introducing ApplicationVersion element.

Would it be possible to use Roslyn to load all AssemblyInfo files and make sure they're mutually exclusive?

I'm just not sure why you would drop out if you encountered more than one file.

andrew-boyarshin commented 5 years ago

https://github.com/hvanbakel/CsprojToVs2017/pull/257#issuecomment-517119810

And regarding the ApplicationVersion — I'll look into it.

andrew-boyarshin commented 5 years ago

@jzabroski ApplicationVersion is not related to AssemblyInfo files in CPS (although, of course, it can be specified in such files). The behaviour is intended, no bug here, although it can be a feature request: simplify away this property if default value of 1.0.0.0 is specified (and its variations, like 1.0). This issue is to be closed and a new one opened for feature request, unless, of course, I've got your comments wrong.

For future reference: The full list of automatically-generated AssemblyInfo properties:

AssemblyCompany
AssemblyConfiguration
AssemblyCopyright
AssemblyDescription
AssemblyFileVersion
AssemblyInformationalVersion
AssemblyProduct
AssemblyTitle
AssemblyVersion
NeutralResourcesLanguage
jzabroski commented 5 years ago

I think you got it wrong.

ApplicationVersion is a SemVer helper and orthogonal to Assembly Info for GREENFIELD projects.

For existing projects, I think Assembly Version Info is correct

andrew-boyarshin commented 5 years ago

@jzabroski sorry, I still don't follow.

My newly generated dotnet SDK project file generates a <ApplicationVersion>1.0.0.0</ApplicationVersion> despite also having <GenerateAssemblyInfo>false</GenerateAssemblyInfo>

Generated by whom? .NET Core SDK CLI (dotnet new) or dotnet-migrate-201X?

ApplicationVersion is a SemVer helper and orthogonal to Assembly Info for GREENFIELD projects.

I assume GREENFIELD is some non-open-source project/organization/company? Well, .NET Core SDK 5.0 (nightly) only has one mention of that property in the feature sector I'm not well informed of.

And I'm still not sure what exactly the issue is with ApplicationVersion property. Why are you mentioning AssemblyInfo files while they are orthogonal to the property in question?

jzabroski commented 5 years ago

Sorry, I was not clear. Hope you can forgive me. Take 2:

Greenfield is a term for building brand new projects (e.g., dotnet new).

Brownfield is a term for maintaining and upgrading existing projects (dotnet-migrate201x helps with this).

For an application that is on Version 2.0, it makes little sense for the output to generate <ApplicationVersion>1.0.0.0</ApplicationVerison>

I think, if you're manually moving from MSBuild file format to .NET Core Common Project System format, then you're safest bet is to either omit ApplicationVersion or use AssemblyVersionAttribute metadata (especially in the case of an executable target vs. a library).

I don't think dotnet-migrate-201x should just throw in ApplicationVersion because that's what dotnet new does. Do you agree or disagree?

mungojam commented 5 years ago

I think I implemented this and I did it after establishing the different treatment of versions in Assembly info vs. SDK project file.

unfortunately I don't have time to look into it more at the moment but I expect it is to do with assembly info falling back on another assembly version property when one is missing, while project SDK may default to 1.0 or 0.0, so to do an accurate migration I had to add the previously missing property unless the default matched already

On Wednesday, 18 September 2019, John Zabroski notifications@github.com wrote:

Sorry, I was not clear. Hope you can forgive me. Take 2:

Greenfield is a term for building brand new projects (e.g., dotnet new).

Brownfield is a term for maintaining and upgrading existing projects ( dotnet-migrate201x helps with this).

For an application that is on Version 2.0, it makes little sense for the output to generate 1.0.0.0

I think, if you're manually moving from MSBuild file format to .NET Core Common Project System format, then you're safest bet is to either omit ApplicationVersion or use AssemblyVersionAttribute metadata (especially in the case of an executable target vs. a library).

I don't think dotnet-migrate-201x should just throw in ApplicationVersion because that's what dotnet new does. Do you agree or disagree?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/hvanbakel/CsprojToVs2017/issues/263?email_source=notifications&email_token=AAYCFSZEWTYKI57HDZ7BX7TQKI6LFA5CNFSM4IXUMM5KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7ALGFA#issuecomment-532722452, or mute the thread https://github.com/notifications/unsubscribe-auth/AAYCFS2IVQZYJFE27N3RTVLQKI6LFANCNFSM4IXUMM5A .

-- Sent from my mobile

jzabroski commented 5 years ago

So, I searched GitHub to see how people might use my concept of GlobalAssemblyInfo (because I learned it from other developers and assume such practices spread like a virus/meme). See here: https://github.com/search?p=4&q=globalassemblyinfo&type=Code

It's interesting that a large number of projects put their "Global Assembly Info" data in a special static class and then reference it in their AssemblyInfo.cs. I don't think this approach is as good as mine because there is overhead associated with loading the second assembly, just for the sake of loading assembly metadata. In this scenario, @mungojam 's logic doesn't work, either, though. The irony is that a meta-circular upgrade test would have the same problem: This csproj project has AssemblyInfoReader.cs and AssemblyInfo.cs