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.

Sign up for GitHub

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

Jump to bottom

Import Windows desktop target when property is set #12106

Merged
5 commits merged into from
Jul 10, 2020
Merged

Import Windows desktop target when property is set #12106

5 commits merged into from
Jul 10, 2020

Conversation

wli3
Copy link

@wli3 wli3 commented Jun 18, 2020

No description provided.

@wli3 wli3 mentioned this pull request Jun 18, 2020
Closed
4 tasks
@wli3 wli3 requested a review from dsplaisted June 18, 2020 23:45
@wli3 wli3 force-pushed the muti-target-5.0-2 branch from f1b11a3 to 467b9ea Compare July 7, 2020 05:09
@wli3 wli3 force-pushed the muti-target-5.0-2 branch from e916625 to 6341409 Compare July 7, 2020 21:29
@wli3 wli3 mentioned this pull request Jul 7, 2020
Closed
@wli3
Copy link
Author

wli3 commented Jul 8, 2020

Find the weird issue (the reason of test failed. dotnet build are all fine. However, dotnet add packages chock on missing $(_TargetFrameworkVersionValue) for some reason. It should be skipped due to evaluation order)

@dsplaisted dsplaisted mentioned this pull request Jul 8, 2020
Closed
25 tasks
@wli3 wli3 force-pushed the muti-target-5.0-2 branch from 58af0d3 to 5229a5c Compare July 9, 2020 18:16
@wli3 wli3 force-pushed the muti-target-5.0-2 branch from 5229a5c to 8e69109 Compare July 9, 2020 18:22
@wli3 wli3 force-pushed the muti-target-5.0-2 branch from 8e69109 to 60b3efd Compare July 9, 2020 18:46
wli3
wli3 commented Jul 9, 2020
View reviewed changes
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Windows.props

<!--https://github.com/dotnet/sdk/issues/12403-->
<PropertyGroup>
<_TargetFrameworkVersionValue>0.0</_TargetFrameworkVersionValue>
Copy link
Author

Choose a reason for hiding this comment

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

@dsplaisted I added this to workaround the missing property issue while I am checking into dotnet/wpf

@wli3 wli3 added auto-merge Automatically merge PR once CI passes. Auto-Merge If Tests Pass labels Jul 9, 2020
@ghost
Copy link

ghost commented Jul 9, 2020

Hello @wli3!

Because this pull request has the Auto-Merge If Tests Pass label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@wli3 wli3 force-pushed the muti-target-5.0-2 branch from 60b3efd to 9c89c89 Compare July 9, 2020 21:25
@ghost ghost merged commit 973a849 into dotnet:master Jul 10, 2020
Nirmal4G
Nirmal4G reviewed Aug 5, 2020
View reviewed changes
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.props
@@ -127,5 +127,7 @@ Copyright (c) .NET Foundation. All rights reserved.

<Import Project="$(MSBuildThisFileDirectory)Microsoft.NET.PackTool.props" />
<Import Project="$(MSBuildThisFileDirectory)Microsoft.NET.PackProjectTool.props" />

<Import Project="$(MSBuildThisFileDirectory)../../Microsoft.NET.Sdk.WindowsDesktop/targets/Microsoft.NET.Sdk.WindowsDesktop.props" Condition="'$(_MicrosoftWindowsDesktopSdkImported)' != 'true'"/>
Copy link
Contributor

@Nirmal4G Nirmal4G Aug 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
<Import Project="$(MSBuildThisFileDirectory)../../Microsoft.NET.Sdk.WindowsDesktop/targets/Microsoft.NET.Sdk.WindowsDesktop.props" Condition="'$(_MicrosoftWindowsDesktopSdkImported)' != 'true'"/>
<Import Sdk="Microsoft.NET.Sdk.WindowsDesktop" Project="../targets/Microsoft.NET.Sdk.WindowsDesktop.props" Condition="'$(_MicrosoftWindowsDesktopSdkImported)' != 'true'"/>

I just ran into an issue where I need to test a patched Desktop SDK. This allows easier substitution of SDK via global json from a custom NuGet feed.

@wli3 @dsplaisted Can I ask why this pattern wasn't used?

Copy link
Member

Choose a reason for hiding this comment

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

@Nirmal4G I don't know that we considered it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dsplaisted Can we do this then?

Copy link
Author

Choose a reason for hiding this comment

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

What advantage that give us? I think it is easy to understand what a simple file import does.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, but it does not allow substitute Sdks from a custom NuGet feed without patching the base SDK.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dsplaisted dsplaisted dsplaisted approved these changes

@Nirmal4G Nirmal4G Nirmal4G left review comments

Assignees
No one assigned
Labels
Auto-Merge If Tests Pass auto-merge Automatically merge PR once CI passes.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wli3 @dsplaisted @Nirmal4G