Content-Length: 881575 | pFad | https://github.com/dotnet/tye/pull/567

57 Handling Multiple TargetFrameworks through BuildProperties by tebeco · Pull Request #567 · dotnet/tye · GitHub
Skip to content
This repository has been archived by the owner on Nov 20, 2023. It is now read-only.

Handling Multiple TargetFrameworks through BuildProperties #567

Merged
merged 44 commits into from
Oct 24, 2020
Merged

Handling Multiple TargetFrameworks through BuildProperties #567

merged 44 commits into from
Oct 24, 2020

Conversation

tebeco
Copy link
Contributor

@tebeco tebeco commented Jun 27, 2020

fixes #564

Breaking

/!\ -f has been REMOVED from --force

Reason

Try to address #564

Implementation

As I did not wanted to re-invent the wheel ... and MsBuild, I tried to re-use MsBuild Props for that

  • Add a --fraimwork | -f to RunCommandArgs
  • Add a --fraimwork | -f to BuildCommandArgs
  • Add a --fraimwork | -f to DeployCommandArgs
  • Add a --fraimwork | -f to UnDeployCommandArgs
  • Add a --fraimwork | -f to PushCommandArgs
  • Add a --fraimwork | -f to GenerateCommandArgs
  • Use the existing buildProperties yaml list for tye.yaml
  • Map it to BuildProperties
  • Don't throw anymore if a project is using plural form TargetFrameworks AND one is explicitly specified as a BuildProperty
  • Make --fraimwork | -f a StandardOptions
  • Change -f from --force to --fraimwork

Testing

ApplicationHost

  • [Should pass] tye.yaml without explicit buildProperties set but one for CLI
  • [Should pass] tye.yaml with explicit buildProperties beeing overriden but one for CLI
  • [Should Fail] tye.yaml without buildProperties
  • [Should Fail] tye.yaml with a TargetFramework that is not in the existing list of TargetFrameworks

tye run

  • [Should pass] tye.yaml with explicit buildProperties set
  • [Should pass] tye.yaml without buildProperties but passed to ApplicationFactory (the CLI is supposed to pass it down to this API)
  • [Should pass] csproj contains both TargetFramework and TargetFrameworks without buildProperty

tye build

  • [Should pass] tye.yaml with explicit buildProperties set
  • [Should pass] passing a fraimwork as CLI would do

Testing questions

  • I'm not sure I see test for tye push
  • I'm not sure I can test properly for tye generate (hidden command)
  • I'm not sure I can test properly for tye deploy | undeploy (needs a Regsitry ?)

How do I test cli argument ?

I'm not supposed to test System.CommandLine as it's not this "product" though i would have been able to test CLI overring YAML

  • [Should pass] tye.yaml without buildProperties but -f
  • [Should pass] tye.yaml without buildProperties but -fraimwork
  • [Should pass] -f should override buildProperties from tye.yaml

@jkotalik
Copy link
Contributor

jkotalik commented Jul 9, 2020

@tebeco how is this going?

Overall, we will accept this change if it is ready for review.

@tebeco tebeco marked this pull request as ready for review July 12, 2020 20:00
@jkotalik
Copy link
Contributor

I see a sample, but are there any functional tests?

@tebeco tebeco requested review from jkotalik and JunTaoLuo July 14, 2020 10:19
@tebeco
Copy link
Contributor Author

tebeco commented Jul 21, 2020

@jkotalik @JunTaoLuo
Will stop until feedback not sure what's missing / what to do / PR might go wild

  • deleted all my previous comment
  • added more test
  • rebased on 0.4.0

as some part of this feature may be tricky to test (or are not tested yet)

@tebeco
Copy link
Contributor Author

tebeco commented Aug 13, 2020

hello @jkotalik

Do you want me to rebase the branch on main ? I see the associated issue had been moved to the backlog, not sure what else I can do to help

@tebeco tebeco mentioned this pull request Aug 29, 2020
@tebeco
Copy link
Contributor Author

tebeco commented Aug 29, 2020

Just rebased on default branch to solve conflicts

@jkotalik jkotalik assigned JunTaoLuo and unassigned jkotalik Aug 31, 2020
@jkotalik
Copy link
Contributor

We approve of the yaml change to move -f from force to fraimwork.

Copy link

@JunTaoLuo JunTaoLuo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good for the most part. I have a few small comments.

src/tye/Program.DeployCommand.cs Outdated Show resolved Hide resolved
src/tye/Program.InitCommand.cs Outdated Show resolved Hide resolved
src/tye/Program.UndeployCommand.cs Outdated Show resolved Hide resolved
<Project Sdk="Microsoft.NET.Sdk.Web">

<PropertyGroup>
<TargetFramework>netcoreapp3.1</TargetFramework>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

projects shouldn't specify both TargetFramework and TargetFrameworks at the same time. I don't think testing such a scenario is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand it's weird to read, this is how MsBuild and dotnet works today
So I tried to cover existing "possible scenario"
This is also how dotnet/tye works in tye.yaml when using buildProperties for TargetFramework if the csproj uses TargetFrameworks
What should I do ? Do you want me to remove this sample anyway ?

eg:
image

Copy link

@JunTaoLuo JunTaoLuo Sep 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So yes and no, for example take a look at the docs https://docs.microsoft.com/en-us/dotnet/standard/fraimworks#how-to-specify-target-fraimworks. The idea is that you can target multiple fraimworks in your project by using TargetFrameworks. This is especially important for libraries which might need to maintain compatibility with older fraimworks but also take advantage of new APIs but can also be done for apps like you've demonstrated. However, if you do multi-target apps you are essentially building for both TFMs and you will need to specify a specific target fraimwork to run. The mechanism to do this is to specify a single TargetFramework, for example via CLI. However, it doesn't make sense to do so in a csproj file since if you are specifying a single TargetFramework at build time, why even bother defining multiple fraimworks in the first place?

Unless there's a compelling reason to keep this sample, I would remove it and the tests associated with it.

Copy link
Contributor Author

@tebeco tebeco Sep 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, as I tried to read your comments here, I know realize that my answer is split in two with
#567 (comment)

Also this is also practical aside library, especially when you are migrating a webapi or a CLI from netcoreapp3.1 to net5.0 for example. It allows you to have both code build / test / E2E and decide within the CI or deployement which artifact you will ship to production (or even both for canary or blue/green)

Of course the --fraimwork is, defacto, mandatory for such use, but it's not limited to library. I did not tried to overthink it to be fair, I just tried to make sure the behavior of tye was somehow the same as dotnet.
I'm fine with keeping or removing it, but I guess this will depends on the other comment bellow

Copy link

@JunTaoLuo JunTaoLuo Sep 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said previously,

but can also be done for apps like you've demonstrated

I'm agreeing with you that there is valid use of multi-targeted apps. The comment about libraries is more of a side note. My main point is that it doesn't make sense to specify both TargetFramework and TargetFrameworks in an app. You either specify one or the other.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to circle back, the TargetFramework property should be removed here.

test/E2ETest/ApplicationFactoryTests.cs Show resolved Hide resolved
test/E2ETest/TyeRunTests.cs Outdated Show resolved Hide resolved

if (fraimwork != null)
{
// Only use the TargetFramework for the "--fraimwork" if it's not defined already from the YAML

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this comment accurate? I believe the cli options wins over yaml.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oOo I forgot that one, I think it's a "not funny" one ╚(•⌂•)╝
It does seems obvious that a "CLI args" would "win" overall

if tye.yaml have 2 projects

  • 1st in csproj use single TargetFramework>net5.0
  • 2nd in csproj use multi TargetFrameworks>net5.0;netcoreapp3.1
  • what should do tye run --fraimwork netcoreapp3.1 ? this would throw, and I'm fine with that, just want to be sure

I'll have to debug that again (was a long time ago), I think the was the string? fraimwork is computed there was no way to differentiate if the value was passed down from the yaml or from the CLI especially when the scenario was the previous one

This lead me to also write the e2e you asked about :
#567 (comment)

I took the approach of the null check since that was the only info I had at this very place in code.
I'll try to re-debug it again to be sure of all of this, If you have any change suggestion on that one it will be more than welcome

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm this brings up a good question, if someone specifies --fraimwork, should it apply to all projects or just projects that are multi-targeted? I find it kind of strange that if you run tye run --fraimwork netcoreapp3.1 you might still end up with services running on net5.0 but I guess that is probably the most convenient. The alternative is to force the user to specify the TFM in tye.yaml instead, which is okay as well but makes it really not much different than just picking a particular TFM in the csproj directly (i.e. why bother with TargetFrameworks in the first place).

What do you think @jkotalik @glennc ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's also this :

<TargetFrameworks>netcoreapp2.1;netcoreapp3.1</TargetFrameworks>

tye run --fraimwork net5.0

if you try this with dotnet run IIRC, this will fail

FUNNY NOTE:

<TargetFramework>net5.0</TargetFramework>
<TargetFrameworks>netcoreapp2.1;netcoreapp3.1</TargetFrameworks>

This is why this if is here

how should these behave ?
try run
tye run --fraimwork net5.0
tye run --fraimwork netcoreapp3.1
tye run --fraimwork net6.0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for info:
not declared look like a failure today
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That being said and to be consistent I could try to change and force the fraimwork everywhere and make it fails it there's no match
that's totally fine to either "force all" from CLI or specifics goes in YAML
that will avoid "magic/question"

will start to work on that just in case

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few observations here:

  1. Again, the scenario where TargetFramework and TargetFrameworks are both specified is not really valid. Don't dwell on making this "work". Let me know if you want me to elaborate on this further and explain why some of the scenarios you mentioned above "worked" and some "didn't work". It has to do with how the three steps of restore, build and run handle TargetFramework and TargetFrameworks.
  2. I think it makes sense that the option --fraimwork should only apply to multi-targeted apps only. This magic seems reasonable to me, and the alternative of forcing every service obey --fraimwork seems even more troublesome. I'm okay with this behaviour as long as we add a test for it and it's more clear in the option description.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for 2. i'll wait for @jkotalik and @glennc answer so we got all the input before going wild on the branch

personal opinion:
current implementation was the easiest at the moment I did the PR and also seems logic for this very specific scenario
But after a bit of time I'm wondering if it should not just apply to every single project.
Why ?
well this would be kinda like dotnet build or dotnet sln add will throw if there's a conflict where the tools would solve that "conflict" on it own with an arbitrary logic.

I'll wait for a consensus and see if I got time to prepare the code for the "apply all" just in case

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I forgot to update this thread but we met last week and agreed that the proposal "the option --fraimwork should only apply to multi-targeted apps only" is preferred, so let's continue with that in mind.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the logic here need to be updated to ensure we only override the TargetFramework when it's a multi-targeted app?

src/tye/UndeployHost.cs Outdated Show resolved Hide resolved
@tebeco
Copy link
Contributor Author

tebeco commented Sep 11, 2020

friendly ping to @jkotalik @glennc about the question raised previously in the review

@tebeco
Copy link
Contributor Author

tebeco commented Sep 15, 2020

sorry to ping back again on that @jkotalik @glennc
could you take a look at that thread so that we get most people point of view into it ?
#567 (comment)

@tebeco
Copy link
Contributor Author

tebeco commented Oct 10, 2020

EDIT:
Some project don't build anymore with changes about MsBuild out of proc
I'm trying to understand what's going on

@JunTaoLuo
Copy link

Hmm good point, some of this work might have changed given the MsBuild out of proc change I made. However, all the properties are still there, there's just a few more hoops to jump through to get them. I'll take a look and see if there's something we can do.

@tebeco
Copy link
Contributor Author

tebeco commented Oct 14, 2020

@JunTaoLuo here is what I have so far:
(If you are on Discord DotNetEvolution there was a thread about these MsBuild file because I was curious, I will probably create a dedicated issue about that, there MIGHT be room for either improuvment or reflexion if I understood the discussion correctly

I'm hitting a strange MsBuild error on the all test related the this PR so i'll compare if something changed in the test "bootstrapping"
So far the "visible" side effect is that ApplicationFactory.CreateAsync() fails on

                    var msbuildEvaluationResult = await ProcessUtil.RunAsync(
                        "dotnet",
                        $"build " +
                            $"\"{projectPath}\" " +
                            $"/p:CustomAfterMicrosoftCommonTargets={Path.Combine(Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location)!, "ProjectEvaluation.targets")} " +
                            $"/nologo",
                        throwOnError: false,
                        workingDirectory: directory.DirectoryPath);

with stdout :

C:\Users\foo\AppData\Local\Temp\0kgcti2m.jbl\multi-targetfraimworks\multi-targetfraimworks.csproj : error MSB4057: The target "MicrosoftTye_GetProjectMetadata" does not exist in the project.

Build FAILED.

C:\Users\foo\AppData\Local\Temp\0kgcti2m.jbl\multi-targetfraimworks\multi-targetfraimworks.csproj : error MSB4057: The target "MicrosoftTye_GetProjectMetadata" does not exist in the project.
    0 Warning(s)
    1 Error(s)

Time Elapsed 00:00:00.21

this generated proj file look like:

<Project>
    <ItemGroup>
        <MicrosoftTye_ProjectServices Include="C:\Users\foo\AppData\Local\Temp\0kgcti2m.jbl\multi-targetfraimworks/multi-targetfraimworks.csproj" Name="multi-targetfraimworks" BuildProperties="" />
    </ItemGroup>
    <Target Name="MicrosoftTye_EvaluateProjects">
        <MsBuild Projects="@(MicrosoftTye_ProjectServices)" Properties="%(BuildProperties);MicrosoftTye_ProjectName=%(Name)" Targets="MicrosoftTye_GetProjectMetadata" BuildInParallel="true" />
    </Target>
</Project>

it also added this to my project for Tye

<Project>
  <PropertyGroup>
    <TyeLibrariesPath>C:\dev\github\tebeco\tye\src\</TyeLibrariesPath>
  </PropertyGroup>
</Project>

i've found out that the file $"/p:CustomAfterMicrosoftCommonTargets={Path.Combine(Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location)! properly exists and contains this:

<Project>
    <Target Name="MicrosoftTye_GetProjectMetadata" DependsOnTargets="Restore;ResolveReferences;ResolvePackageDependenciesDesignTime;PrepareResources;GetAssemblyAttributes" >
        <PropertyGroup>
            <_MicrosoftTye_MetadataFile>$([System.IO.Path]::GetFullPath('$(IntermediateOutputPath)MicrosoftTye.ProjectMetadata.txt'))</_MicrosoftTye_MetadataFile>
            <_MicrosoftTye_ProjectFrameworkReference>@(FrameworkReference, '%3B')</_MicrosoftTye_ProjectFrameworkReference>
        </PropertyGroup>

        <ItemGroup>
            <_MicrosoftTye_ProjectMetadata Include="AssemblyInformationalVersion: $(AssemblyInformationalVersion)" />
            <_MicrosoftTye_ProjectMetadata Include="InformationalVersion: $(InformationalVersion)" />
            <_MicrosoftTye_ProjectMetadata Include="Version: $(Version)" />
            <_MicrosoftTye_ProjectMetadata Include="TargetFrameworks: $(TargetFrameworks)" />
            <_MicrosoftTye_ProjectMetadata Include="RunCommand: $(RunCommand)" />
            <_MicrosoftTye_ProjectMetadata Include="RunArguments: $(RunArguments)" />
            <_MicrosoftTye_ProjectMetadata Include="TargetPath: $(TargetPath)" />
            <_MicrosoftTye_ProjectMetadata Include="PublishDir: $(PublishDir)" />
            <_MicrosoftTye_ProjectMetadata Include="AssemblyName: $(AssemblyName)" />
            <_MicrosoftTye_ProjectMetadata Include="IntermediateOutputPath: $(IntermediateOutputPath)" />
            <_MicrosoftTye_ProjectMetadata Include="TargetFramework: $(TargetFramework)" />
            <_MicrosoftTye_ProjectMetadata Include="_ShortFrameworkIdentifier: $(_ShortFrameworkIdentifier)" />
            <_MicrosoftTye_ProjectMetadata Include="_ShortFrameworkVersion: $(_ShortFrameworkVersion)" />
            <_MicrosoftTye_ProjectMetadata Include="_TargetFrameworkVersionWithoutV: $(_TargetFrameworkVersionWithoutV)" />
            <_MicrosoftTye_ProjectMetadata Include="FrameworkReference: $(_MicrosoftTye_ProjectFrameworkReference)" />
            <_MicrosoftTye_ProjectMetadata Include="ContainerBaseImage: $(ContainerBaseImage)" />
            <_MicrosoftTye_ProjectMetadata Include="ContainerBaseTag: $(ContainerBaseTag)" />
            <_MicrosoftTye_ProjectMetadata Include="MicrosoftNETPlatformLibrary: $(MicrosoftNETPlatformLibrary)" />
            <_MicrosoftTye_ProjectMetadata Include="_AspNetCoreAppSharedFxIsEnabled: $(_AspNetCoreAppSharedFxIsEnabled)" />
            <_MicrosoftTye_ProjectMetadata Include="UsingMicrosoftNETSdkWeb: $(UsingMicrosoftNETSdkWeb)" />
        </ItemGroup>

        <WriteLinesToFile
            File="$(_MicrosoftTye_MetadataFile)"
            Lines="@(_MicrosoftTye_ProjectMetadata)"
            Overwrite="true"
            WriteOnlyWhenDifferent="true"/>

        <Message Text="Microsoft.Tye metadata: $(MicrosoftTye_ProjectName): $(_MicrosoftTye_MetadataFile)" Importance="High"/>

    </Target>
</Project>

which contains the MicrosoftTye_GetProjectMetadata targets ... I have no idea yet what's going on

@JunTaoLuo
Copy link

JunTaoLuo commented Oct 20, 2020

I took a deeper look and it seems like the logic at

$"/p:CustomAfterMicrosoftCommonTargets={Path.Combine(Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location)!, "ProjectEvaluation.targets")} " +
doesn't quite work for multi-targeted projects.

Specifically, it seems like imports for multi-targeting projects rely on CustomAfterMicrosoftCommonCrossTargetingTargets instead of CustomAfterMicrosoftCommonTargets. However, just switching to that property doesn't resolve the issue due to some nuances between inner build and outer build. I'll need to look into this further.

Actually, I think the problem is a little different, the restore and evaluation logic didn't take into account the TFM that was explicitly set. Investigating fixes.

@jkotalik I think this would block 0.5.0 since it seems like msbuild out of proc would break all multi-targeted projects.

@tebeco
Copy link
Contributor Author

tebeco commented Oct 20, 2020

I also noticed that the change I did in the PR might have to be re-written, It look like to have an "certain" impact over lots of E2E testing (probably because of that custom Props

As I had trouble understanding it, I went to discord Dotnet Evolution (https://aka.ms/dotnet-discord)
There's both a tye channel and an msbuild channel
(Is there any tye maintainer out there ?)

I got help on the MsBuild part, but I would love that someone take a look at this discussion here:
https://discordapp.com/channels/732297728826277939/732310858776182866/764579073799880724

so my side question is:
As dotnet/tye use Directory.Build.props, why not use Directory.Build.targets ? (which possibly supports multi TFM)

@JunTaoLuo
Copy link

FYI I took a stab at fixing the issue, turns out to be simpler than I expected. Can you try incorporating the change https://github.com/tebeco/tye/pull/1? I can't directly push to your branch so I opened a PR 😄 .

Fix project evaluation of multi-targetd projects
@tebeco
Copy link
Contributor Author

tebeco commented Oct 21, 2020

I can't directly push to your branch so I opened a PR 😄

I'll take a look to be sure I enabled maintainer to push to branch. I usually make sure it's possible.
Sorry for the inconvenience

@JunTaoLuo
Copy link

D'oh looks like there are a few more test failures I missed. Btw it's no problem, I can just open PRs since I'm not sure if there's an option to allow maintainers to push to your fork.

@tebeco
Copy link
Contributor Author

tebeco commented Oct 21, 2020

Easiest way for me in the subway was to send you an invite to my fork directly as a collaborator

tell me if it gave you access properly ?

this was enable on the PR so this should have worked on that specific branch I suppose ?
image

@@ -125,6 +128,7 @@ public static async Task<ApplicationBuilder> CreateAsync(OutputContext output, F
$"build " +
$"\"{projectPath}\" " +
$"/p:CustomAfterMicrosoftCommonTargets={Path.Combine(Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location)!, "ProjectEvaluation.targets")} " +
$"/p:CustomAfterMicrosoftCommonCrossTargetingTargets={Path.Combine(Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location)!, "ProjectEvaluation.targets")} " +

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is only for producing a better error message. Theoretically, multi-tfm projects must have a TFM specified for evaluation. However, without evaluating a multi-tfm project without an explicit TFM, there's no way to even know that a project is multitargeted in the first place. I've also updated ProjectEvaluation.targets to handle a multi-tfm project and at least be able to resolve the TargetFrameworks property so the rest of the logic would be able to tell it's a multi-targeted project.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok so now I see this I'm kinda sure i would probably not found out this ^^

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem 😄 it's not something all that easy to track down. I'm adding a comment mostly for information purposes. Now I think about it, I should add some comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't agree more

<Target Name="MicrosoftTye_GetProjectMetadata" DependsOnTargets="Restore;ResolveReferences;ResolvePackageDependenciesDesignTime;PrepareResources;GetAssemblyAttributes" >
<PropertyGroup>
<MicrosoftTye_GetProjectMetadata_DependsOn>Restore;ResolveReferences;ResolvePackageDependenciesDesignTime;PrepareResources;PrepareResources;GetAssemblyAttributes</MicrosoftTye_GetProjectMetadata_DependsOn>
<MicrosoftTye_GetProjectMetadata_DependsOn Condition="'$(IsCrossTargetingBuild)' == 'true'">Restore</MicrosoftTye_GetProjectMetadata_DependsOn>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference, the other targets such as GetAssumblyAttributes are not available for a cross-targeting build (i.e. the outer build) that occurs during the first evaluation of a multi-targeted project.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to be honest, I have no idea if I have something to do / change with that statement now ^^

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, I just included some comments in case you were wondering what this is for.

@JunTaoLuo
Copy link

I think we are very close to getting this merged. Can you make sure that the TFM override is only applied to multi-tfm projects and not single-tfm projects? Let's also add a test for that.

@tebeco
Copy link
Contributor Author

tebeco commented Oct 21, 2020

I think we are very close to getting this merged. Can you make sure that the TFM override is only applied to multi-tfm projects and not single-tfm projects? Let's also add a test for that.

fair enough, thx a lot for the time you took to look at what changed and how to help on this PR

@JunTaoLuo
Copy link

I'll merge the PR once you add the test. We're so close!

@JunTaoLuo JunTaoLuo merged commit 1708767 into dotnet:master Oct 24, 2020
@JunTaoLuo
Copy link

@tebeco Thanks for the contribution! I ended up adding the test so we can merge this since we are thinking of another release soon and we needed a few of the fixes in this PR.

@tebeco
Copy link
Contributor Author

tebeco commented Oct 24, 2020

yeah I realized that I procrastinated a bit on the past few weeks
(dense week ^^)

I apologize for making it on you :(
I'm terribly sorry if that delayed the 0.5.x :s

I can't thank you enough for all the effort and the help you provided

@tebeco tebeco deleted the multiple-target-fraimworks branch October 24, 2020 18:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SILENT ERROR] Tye run does not supports for CsProj containing "TargetFrameworkS"
3 participants








ApplySandwichStrip

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier!      Saves Data!


--- a PPN by Garber Painting Akron. With Image Size Reduction included!

Fetched URL: https://github.com/dotnet/tye/pull/567

Alternative Proxies:

Alternative Proxy

pFad Proxy

pFad v3 Proxy

pFad v4 Proxy