Generate appconfig for full framework project #2447

wli3 · 2018-07-31T20:59:54Z

wli3 · 2018-07-31T21:42:08Z

src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenAGenerateAppConfig.cs

+        }
+
+        // intersection of https://docs.microsoft.com/en-us/nuget/reference/target-frameworks
+        // and https://docs.microsoft.com/en-us/dotnet/framework/configure-apps/file-schema/startup/supportedruntime-element#version


Use this map is good enough? Also, net30 is not a tfm in dotnet core. So, this is not a valid tfm

wli3 · 2018-07-31T21:42:39Z

src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenAGenerateAppConfig.cs

+        [InlineData("net30")]
+        [InlineData("net999")]
+        [InlineData("netstandard20")]
+        public void It_does_not_generate_version_and_sku_for_non_supported(string targetframework)


If it is not a valid TFM, no supportedRuntime is generated

wli3 · 2018-07-31T21:44:26Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.GenerateAppConfig.targets

+    </ItemGroup>
+
+    <!--Override the AppConfigWithTargetPath for downstream target-->
+    <ItemGroup>


This override I am not sure. PrepareForBuild will find and put AppConfigWithTargetPath. And GenerateBindingRedirects will read AppConfigWithTargetPath, and add binding redirect on top of it.

And this task will point AppConfigWithTargetPath to its intermediate result. So GenerateBindingRedirects will use the generated file.

dsplaisted

I didn't have time to review the whole thing, but I did leave some feedback.

dsplaisted · 2018-08-02T18:35:02Z

src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenAGenerateAppConfig.cs

+
+namespace Microsoft.NET.Build.Tasks.UnitTests
+{
+    public class GivenAGenerateAppConfig


Rename this class/file to GivenAGeneratedAppConfig (it's missing a "d").

Ah, forget to run through a round of spell checker

dsplaisted · 2018-08-02T18:35:29Z

src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenAGenerateAppConfig.cs

+    public class GivenAGenerateAppConfig
+    {
+        [Fact]
+        public void It_creates_startup_and_supportedRuntime_nod_when_there_is_not_any()


Typo in method name: nod -> node

dsplaisted · 2018-08-02T18:38:07Z

src/Tasks/Microsoft.NET.Build.Tasks/WriteAppConfig.cs

+        public ITaskItem AppConfigFile { get; set; }
+
+        [Required]
+        public string TargetFramework { get; set; }


This task should be based on the TargetFrameworkIdentifier and TargetFrameworkVersion properties (which are by default inferred from TargetFramework) instead of directly depending on TargetFramework.

used TargetFrameworkIdentifier and TargetFrameworkVersion instead

dsplaisted · 2018-08-02T18:44:13Z

src/Tasks/Microsoft.NET.Build.Tasks/WriteAppConfig.cs

+            {
+                if (targetFrameworkParsed.Version.Major < 4)
+                {
+                    if (_targetFrameworkBelow40RuntimeVersionMap.TryGetValue(targetFrameworkParsed.GetShortFolderName(), out string value))


Instead of using a map here, I would just use if statements, which check the range of values. IE any version >= 2.0 but < 4.0 should use v2.0.50727 in the app.config.

Fixed, check between version range

nguerrera · 2018-08-03T00:27:42Z

src/Tasks/Microsoft.NET.Build.Tasks/WriteAppConfig.cs

+
+        [Required]
+        public string TargetFrameworkVersion { get; set; }
+


For completeness, this should have optional TargetFrameworkProfile as well.

fixed, added TargetFrameworkProfile

nguerrera · 2018-08-03T00:29:04Z

src/Tasks/Microsoft.NET.Build.Tasks/WriteAppConfig.cs

+                        new XElement(
+                            "supportedRuntime",
+                            new XAttribute("version", "v4.0"),
+                                new XAttribute("sku", $"{targetFrameworkIdentifier},Version={targetFrameworkVersion}"));


For completeness, sku should include Profile (if any) as well in TFM. We don't support net40-client directly with our target framework inference, but I believe some folks do it explicitly or use community "extras" package to do it.

an example? And is there a easy way to test it to make sure profile and tfm is in correct "format"? I am afraid of adding more property and it ends up cannot be read by runtime. Today, I avoid this issue by matching exactly to the following doc.

https://github.com/dotnet/docs/blob/master/docs/framework/configure-apps/file-schema/startup/supportedruntime-element.md

I find it, it is also a doc

https://docs.microsoft.com/en-us/dotnet/framework/configure-apps/file-schema/startup/supportedruntime-element

So it will just do Profile="${TargetFrameworkProfile}"

You will need to make profile optional and only use it if set.

Fixed, it is optional. Only add it when it is set

nguerrera · 2018-08-03T00:30:44Z

src/Tasks/Microsoft.NET.Build.Tasks/WriteAppConfig.cs

+            supportedRuntime = null;
+
+            if (targetFrameworkIdentifier == ".NETFramework"
+                && NuGetVersion.TryParse(targetFrameworkVersion.TrimStart('v', 'V'), out NuGetVersion parsedVersion))


TargetFrameworkVersion should parse as System.Version

Fixed, used System.Version.Parse

nguerrera · 2018-08-03T00:32:15Z

src/Tasks/Microsoft.NET.Build.Tasks/WriteAppConfig.cs

+            if (File.Exists(OutputAppConfigFile.ItemSpec))
+            {
+                File.Delete(OutputAppConfigFile.ItemSpec);
+            }


This doesn't seem necessary: FileMode.Create below will truncate the existing file.

Create Specifies that the operating system should create a new file. If the file already exists, it will be overwritten. This requires FileIOPermissionAccess.Write permission. FileMode.Create is equivalent to requesting that if the file does not exist, use CreateNew; otherwise, use Truncate. If the file already exists but is a hidden file, an UnauthorizedAccessException exception is thrown.

Fixed, removed File.Exists File.Delete

nguerrera · 2018-08-03T00:32:43Z

src/Tasks/Microsoft.NET.Build.Tasks/WriteAppConfig.cs

+                FileMode.Create,
+                FileAccess.Write,
+                FileShare.Read);
+            using (var stream = new StreamWriter(fileStream))


nit: newline here.

nguerrera · 2018-08-03T00:35:17Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.GenerateAppConfig.targets

+  <Target Name="GenerateAppConfig"
+          Condition="'$(GenerateAppConfig)' != 'false' and '$(TargetFrameworkIdentifier)' == '.NETFramework' and '$(HasRuntimeOutput)' == 'true'"
+          DependsOnTargets="_WriteAppConfig"
+          BeforeTargets="GenerateBindingRedirects">


This means that if I set GenerateBindingRedirects to false, but GenerateAppConfig to true, that it won't work? I think in practice all SDK projects should generate binding redirects, but it feels a little clunky to tie them together.

Fixed, beforetargets ResolveReferences.

nguerrera · 2018-08-03T00:37:07Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.GenerateAppConfig.targets

+    <_GenerateAppConfigIntermediateAppConfig>$(IntermediateOutputPath)$(TargetFileName).withSupportedRuntime.config</_GenerateAppConfigIntermediateAppConfig>
+  </PropertyGroup>
+
+  <Target Name="GenerateAppConfig"


I'm wondering if GenerateAppConfig is a good name. Probably better to say what in app config we're generating to match GenerateBindingRedirects. GenerateSupportedRuntime, perhaps? It is a common case for netfx apps to already have an app.config for other reasons so we aren't always generating one from scratch, but that's what the name reads like to me.

Fixed, renamed target to GenerateSupportedRuntime and the task is called WriteAppConfigWithSupportedRuntime (since it will create one)

wli3 · 2018-08-08T00:09:28Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.GenerateSupportedRuntime.targets

+  <Target Name="GenerateSupportedRuntime"
+          Condition="'$(GenerateSupportedRuntime)' != 'false' and '$(TargetFrameworkIdentifier)' == '.NETFramework' and '$(HasRuntimeOutput)' == 'true'"
+          DependsOnTargets="_WriteAppConfigWithSupportedRuntime"
+          BeforeTargets="GenerateBindingRedirects">


@rainersigwald this is the issue I asked. We are worried about if this target can still run when GenerateBindingRedirects is disable.

Before a target is executed or skipped, any target that lists it in a BeforeTargets attribute is run.

https://docs.microsoft.com/en-us/visualstudio/msbuild/target-build-order#determine-the-target-build-order

@nguerrera plus I tested out disable AutoGenerateBindingRedirects=false . it is fine to use BeforeTargets="GenerateBindingRedirects"

Ah, ok, I always get confused by that.

wli3 · 2018-08-08T19:46:32Z

@nguerrera all comments are addressed

wli3 force-pushed the gen-app-config-2 branch from 5863af1 to afd4b55 Compare July 31, 2018 21:40

wli3 changed the title ~~WIP Generate appconfig for full framework project~~ Generate appconfig for full framework project Jul 31, 2018

wli3 commented Jul 31, 2018

View reviewed changes

wli3 changed the title ~~Generate appconfig for full framework project~~ WIP Generate appconfig for full framework project Aug 1, 2018

wli3 changed the title ~~WIP Generate appconfig for full framework project~~ Generate appconfig for full framework project Aug 2, 2018

wli3 requested review from nguerrera, dsplaisted and a team August 2, 2018 04:47

wli3 added this to the 2.2.1xx milestone Aug 2, 2018

dsplaisted reviewed Aug 2, 2018

View reviewed changes

nguerrera reviewed Aug 3, 2018

View reviewed changes

wli3 force-pushed the gen-app-config-2 branch 2 times, most recently from 7f3d8aa to c3719c1 Compare August 6, 2018 19:50

Generate appconfig for full framework project

91872d6

wli3 force-pushed the gen-app-config-2 branch from 0cca99d to 91872d6 Compare August 8, 2018 00:06

wli3 commented Aug 8, 2018

View reviewed changes

nguerrera approved these changes Aug 8, 2018

View reviewed changes

wli3 merged commit adff69d into dotnet:release/2.2.1xx Aug 8, 2018

wli3 deleted the gen-app-config-2 branch August 8, 2018 21:02

This was referenced Mar 24, 2019

Disable new MSBuild GenerateSupportedRuntime functionality, which breaks framework targetting nunit/nunit-console#587

Merged

GenerateSupportedRuntimes causes breaking change with targeting specific .NET framework versions #3036

Open

dasMulli mentioned this pull request Oct 3, 2019

Build Order Bug with app.config/BeforeBuild #3708

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate appconfig for full framework project #2447

Generate appconfig for full framework project #2447

wli3 commented Jul 31, 2018 •

edited

Loading

wli3 Jul 31, 2018

wli3 Jul 31, 2018

wli3 Jul 31, 2018 •

edited

Loading

dsplaisted left a comment

dsplaisted Aug 2, 2018

wli3 Aug 2, 2018

wli3 Aug 2, 2018

dsplaisted Aug 2, 2018

wli3 Aug 2, 2018

dsplaisted Aug 2, 2018

wli3 Aug 2, 2018

dsplaisted Aug 2, 2018

wli3 Aug 2, 2018

nguerrera Aug 3, 2018

wli3 Aug 3, 2018 •

edited

Loading

nguerrera Aug 3, 2018

wli3 Aug 3, 2018

wli3 Aug 3, 2018

nguerrera Aug 3, 2018

wli3 Aug 3, 2018

nguerrera Aug 3, 2018

wli3 Aug 3, 2018

nguerrera Aug 3, 2018

wli3 Aug 3, 2018

nguerrera Aug 3, 2018

wli3 Aug 3, 2018

nguerrera Aug 3, 2018

wli3 Aug 3, 2018

nguerrera Aug 3, 2018

wli3 Aug 3, 2018

wli3 Aug 8, 2018

wli3 Aug 8, 2018

nguerrera Aug 8, 2018

wli3 commented Aug 8, 2018


		[Required]
		public string TargetFrameworkVersion { get; set; }

Generate appconfig for full framework project #2447

Generate appconfig for full framework project #2447

Conversation

wli3 commented Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wli3 Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

dsplaisted left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wli3 Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wli3 commented Aug 8, 2018

wli3 commented Jul 31, 2018 •

edited

Loading

wli3 Jul 31, 2018 •

edited

Loading

wli3 Aug 3, 2018 •

edited

Loading