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
Generate AssemblyInfo attributes during build #81
Conversation
NuGetVersion version; | ||
if (string.IsNullOrEmpty(SemanticVersion) || !NuGetVersion.TryParseStrict(SemanticVersion, out version)) | ||
{ | ||
Log.LogError($"Invalid semantic version: '{SemanticVersion}'"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this is the first custom error message. Thoughts on localization for this assembly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I'll add a TODO with reference to #33.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What errors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, $(Version) does not parse as a semantic version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, you mean the errors preventing loc. #33 has some details.
👍 This looks great. |
tag @dotnet/project-system |
@@ -130,6 +131,105 @@ Copyright (c) .NET Foundation. All rights reserved. | |||
</ItemGroup> | |||
</Target> | |||
|
|||
<!-- | |||
============================================================ | |||
GenerateAssemblyInfo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it would be worthwhile to split this into its own .targets file?
My thoughts are twofold:
- It organizes the logic well. Someone can easily see a file named "Microsoft.NETCore.GenerateAssemblyInfo.targets" and quickly understand what logic is in that file.
- It allows us to reuse this logic in other .targets files without importing the whole "Sdk.targets" file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be fine with that. I'll do it unless someone pushes back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only downside I can think of is that it makes it easier to accidentally double-import, and it makes debugging somewhat more difficult because you have to hop to another file.
I think we've gone way too far in the "put everything in one giant mass of spaghetti" direction historically, so I support separate files for logically separate things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking has been "if I was coding in C#, would I make a separate class for this logic? If yes, I use a separate file."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for non-giant-mass-of-spaghetti targets
Looks good, @nguerrera. Just some random comments and thoughts. when addressed. |
Hmm, WriteCodeFragment is generating superfluous ';'s that break compilation on the non-CodeDom implementation for .NET Core. :( Tests started failing. Lost as to how they passed before. I'm going to submit a PR to msbuild xplat branch, what's the flow to here like? |
We're (CPS, us, MSBuild) are all inserting into the same branch on the VS side now - so the flow on the VS side is okay. Not sure how we pick up the new MSBuild on the CLI side though, @eerhardt? |
To update CLI's MSBuild, should be something like dotnet/cli@5f60c2c. MSBuild's packages aren't auto-published, so you might have to ping us/me if we don't remember to trigger a build after the change is merged. |
First we need to take the new MSBuild drop into the CLI. Searching all project.json files in the CLI for MSBuild packages, and updating the versions. Then once we have a build of the CLI with the new MSBuild, we change the https://github.com/dotnet/sdk/blob/master/DotnetCLIVersion.txt to the new build of the CLI. |
@rainersigwald It's merged (thanks for that), please trigger a build. |
Duplicate comment but it's more relevant here: @nguerrera I built packages at version |
|
@eerhardt The CLI change is in. When will we get a new build? Is it nightly or is there a manual process? |
Neither. The official build is kicked off on each merge into the branch. So the official build is running now. It takes around an hour. See all the build definitions marked with The new build number is generated from the commit count. So your build will be version |
|
||
<PropertyGroup> | ||
<!-- We don't use any of MSBuild's resolution logic for resolving the framework, so just set these two | ||
properties to any folder that exists to skip the GetReferenceAssemblyPaths task (not target) and | ||
properties to any folder that exists to skip the GetReferenceAssemblyPaths task (not target) and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this line intentially duped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops.
Also, use $(AssemblyTitle) defaulted to $(AssemblyName) instead of using $(AssemblyName) directly for AssemblyTitleAttribute Finally, don't set Authors and Copyright in template since $registeredorg$ is often blank and it clutters the new project. Just let users set authors and copyright as they see fit. This seems more in keeping with the spirit of minimal boilerplate in the new .csproj files.
I really hope we can figure out getting the daily build into the $(FileVersion) by default, in the future. |
Agreed. FWIW, the exact $(Version) gets in to InformationalVersion and can be seen in explorer properties as "Product Version". So nothing is lost, but we should think about if we're putting the right numbers in the right field... |
Conflicts: src/Tasks/Microsoft.NETCore.Build.Tasks/Microsoft.NETCore.Build.Tasks.csproj
I think this should be moved to the Microsoft/MSBuild repo common targets. |
I would like to see some sort of decent line drawn between what is "MSBuild" functionality, and what is outside of MSBuild in the language/platform's "SDK". Currently that line is super blurry, if it exists at all. Technically, if something is ".NET project specific", it shouldn't really be a part of MSBuild since MSBuild can be used to build things that aren't .NET (like C++ projects). The point of this repo, IMO, is to become the SDK for .NET projects. That way .NET can innovate its projects without having to ship a new MSBuild, or even ship new common targets - which are installed with MSBuild. The .targets and .props files contained here ship in NuGet packages, which can be updated easily and allow for one project to stay on older targets, while other projects upgrade to newer versions. Some/most of these .targets and .props files should be able to be reused in other .NET projects, not just ".NET Core" projects. Of course we are just starting down this path, and we can't change everything all at once. But, from my view, this is the direction we are currently headed. |
With that goal in mind, I wholeheartedly agree :) I'd start by renaming all |
Generate AssemblyInfo attributes during build
Add DynamicDependentFile capability for nesting rules
Fix #57, which covers the generation portion of #2
@srivatsn @davkean @eerhardt @natidea