Skip to content
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

Enable TargetFrameworkAttribute generation #32680

Closed

Conversation

Projects
None yet
6 participants
@ViktorHofer
Copy link
Member

ViktorHofer commented Oct 7, 2018

Fixes #26456
Fixes #26457

@eerhardt I assume you disabled the target framework attribute generation because we haven't had set it before?

@ViktorHofer ViktorHofer self-assigned this Oct 7, 2018

@ViktorHofer ViktorHofer requested a review from weshaggard Oct 7, 2018

@ViktorHofer ViktorHofer force-pushed the ViktorHofer:TargetFrameworkAttribute branch from 906ca93 to 18aa076 Oct 7, 2018

@@ -3,4 +3,3 @@
// See the LICENSE file in the project root for more information.

[assembly: System.Diagnostics.CodeAnalysis.ExcludeFromCodeCoverageAssembly]
[assembly: System.Runtime.Versioning.TargetFrameworkAttribute("DUMMY-TFA,Version=v0.0.1")]

This comment has been minimized.

Copy link
@jkotas

jkotas Oct 7, 2018

Member

Disable auto-generation of the attribute for this test instead?

Most of the value of this test is lost with this change.

This comment has been minimized.

Copy link
@ViktorHofer

ViktorHofer Oct 7, 2018

Author Member

I would want to avoid special casing the RemoteExecutorConsoleApp just for the sake of the one test.

This comment has been minimized.

Copy link
@jkotas

jkotas Oct 8, 2018

Member

So create a dedicated .exe just for this test like we have elsewhere. E.g.: https://github.com/dotnet/corefx/tree/master/src/System.Runtime.Extensions/tests/VoidMainWithExitCodeApp

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Oct 7, 2018

Is this going to add TargetFrameworkAttribute on the framework assemblies itself (like System.Runtime, ...)? If yes, is it something that we want?

We have never done that before. For example, System.dll in desktop does not have TargetFrameworkAttribute.

@ViktorHofer

This comment has been minimized.

Copy link
Member Author

ViktorHofer commented Oct 7, 2018

Is this going to add TargetFrameworkAttribute on the framework assemblies itself (like System.Runtime, ...)? If yes, is it something that we want?

Correct, framework assemblies will have the attribute on them. Based on the discussion between @weshaggard and @ericstj I believe we want to set it as some tools default to .NET Framework if the attribute isn't present (#26456).

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Oct 8, 2018

I believe we want to set it as some tools default to .NET Framework if the attribute isn't present (#26456).

The root cause of the problem described in #26456 is a bug in dotPeek making stuff up. I do not think we should be "fixing" our framework assemblies to workaround it. (Having this attribute on OOB assemblies that are not part of the framework is fine.)

What are the chances that having the target attribute on framework assemblies itself is going to break stuff?

E.g. consider what this code fragment from stackoverflow going to print before/after your change:

using System;
using System.Reflection;
using System.Linq;
using System.Runtime.Versioning;

class Program
{
    static void Main(string[] args)
    {
        // Lets examine all assemblies loaded into the current application domain.
        var assems = AppDomain.CurrentDomain.GetAssemblies();

        // The target framework attribute used when the assemby was compiled.
        var filteredType = typeof(TargetFrameworkAttribute);

        // Get all assemblies that have the TargetFrameworkAttribute applied.
        var assemblyMatches = assems.Select(x => new { Assembly = x, TargetAttribute = (TargetFrameworkAttribute)x.GetCustomAttribute(filteredType) })
                                    .Where(x => x.TargetAttribute != null);

        // Report assemblies framework target
        foreach (var assem in assemblyMatches)
        {
            var framework = new System.Runtime.Versioning.FrameworkName(assem.TargetAttribute.FrameworkName);
            Console.WriteLine("Assembly: '{0}' targets .NET version: '{1}'.",
                                assem.Assembly.FullName,
                                framework.Version);
        }
    }
}
@ViktorHofer

This comment has been minimized.

Copy link
Member Author

ViktorHofer commented Oct 8, 2018

The root cause of the problem described in #26456 is a bug in dotPeek making stuff up. I do not think we should be "fixing" our framework assemblies to workaround it. (Having this attribute on OOB assemblies that are not part of the framework is fine.)

I agree that the behavior of dotPeek is sub-optimal and should probably be changed to not make things up (as also mentioned by @akoeplinger).

What are the chances that having the target attribute on framework assemblies itself is going to break stuff?

The question is if customers are relying on inbox framework assemblies not having the TargetFrameworkAttribute set and if such an assumption is safe. I don't have any intel on that, that said, I'm unsure if the additional entries that get printed in the code piece you posted above would count as a breaking change.

I'm not a fan of behavioral differences between our assemblies and customer ones and how me leverage the SDK/msbuild to produce them. I think @weshaggard and @ericstj should weigh in.

@eerhardt

This comment has been minimized.

Copy link
Member

eerhardt commented Oct 8, 2018

@eerhardt I assume you disabled the target framework attribute generation because we haven't had set it before?

I never disabled it. It has always been disabled for the past 4 years:

3d95f9c#diff-b87fdd5b06580decb8c67703b971fbbcR30

@ViktorHofer

This comment has been minimized.

Copy link
Member Author

ViktorHofer commented Oct 8, 2018

Thanks, when I wrote that post I assumed the property is controlled by the SDK but in fact it's a msbuild one which is unrelated to the SDK projet change that you did earlier this year.

@weshaggard

This comment has been minimized.

Copy link
Member

weshaggard commented Oct 12, 2018

The root cause of the problem described in #26456 is a bug in dotPeek making stuff up. I do not think we should be "fixing" our framework assemblies to workaround it. (Having this attribute on OOB assemblies that are not part of the framework is fine.)

I agree we don't want to do this just to fix this bug in dotPeek. I also agree we have never done this on .NET Framework but there has always been a desire to have this marker to help identify what a given assembly was built for. @jkotas do you feel there is a strong reason not to include it? I don't think the code sample of someone breaking because of this is a real scenario that we need to worry about as we know every change can break some code.

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Oct 12, 2018

I do not have a problem with this. I just want us to do this for the right reason. If you are doing this for all assemblies (even the inbox ones), you should also make sure to do that for System.Private.CoreLib.

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Oct 12, 2018

Also, we should add tests to validate this.

@stephentoub

This comment has been minimized.

Copy link
Member

stephentoub commented Nov 2, 2018

@ViktorHofer, what's the status of this PR?

@ViktorHofer

This comment has been minimized.

Copy link
Member Author

ViktorHofer commented Nov 2, 2018

I will revisit this after the arcade work is done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.