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

Muti target 5.0 second part #3155

Merged
merged 2 commits into from
Jul 7, 2020
Merged

Conversation

wli3
Copy link

@wli3 wli3 commented Jun 18, 2020

No description provided.

William Li added 2 commits June 18, 2020 15:46
In Sdk.props. Set the property so the dotnet core SDK will import it.
@ghost ghost added the PR metadata: Label to tag PRs, to facilitate with triage label Jun 18, 2020
@ghost ghost requested review from fabiant3, ryalanms and SamBent June 18, 2020 22:50
@wli3 wli3 marked this pull request as ready for review June 18, 2020 23:41
@wli3 wli3 requested a review from a team as a code owner June 18, 2020 23:41
@wli3
Copy link
Author

wli3 commented Jun 18, 2020

@ryalanms could you review the second part of #2869?

It is part of dotnet/sdk#12108

<Import Project="$(WpfSourceDir)PresentationBuildTasks\Microsoft.WinFX.props"
Condition="Exists('$(LocalMicrosoftWinFXProps)') "/>

<Import Sdk="Microsoft.NET.Sdk.WindowsDesktop"
Copy link
Author

Choose a reason for hiding this comment

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

I am not very sure deleting that is safe. However, after the last change, Microsoft.WinFX.props files is empty

Copy link
Member

Choose a reason for hiding this comment

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

Did you confirm that this is still picking up the locally built version of PresentationBuildTasks?

Copy link
Member

@vatsan-madhavan vatsan-madhavan Jun 23, 2020

Choose a reason for hiding this comment

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

This (the ‘otherwise import Windowsdesktop props’ part) can only be tested by flowing (simulating the flow) the wpf.arcade package to the wpf-int repo, and building a project there that relies on InternalMarkupCompilation. One may not exist there today, but creating a test app that does should be quite easy.

This functionality may important to retain for future use by the test repo.

Copy link
Member

Choose a reason for hiding this comment

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

I now see that InternalMarkupCompilation isn't set in the WPF repo.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I have no idea what you are talking about. What is the action item here? Can I remove this or not? I could left the empty WinFXProps file if we need it

Copy link
Member

Choose a reason for hiding this comment

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

@wli3: Can you add the replacement of pbt.props/targets in to your refactoring work issue? Thanks.

Copy link
Author

Choose a reason for hiding this comment

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

Do you have doc for what pbt.props/targets is for? I did read the binlog of build of wpf repo. But I still don't have a clue

Copy link
Member

Choose a reason for hiding this comment

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

There is some description in the markdown file. Sorry, I don't believe there is a separate doc with a more detailed description. We can discuss it during the meeting later today. Thanks.

Copy link
Author

Choose a reason for hiding this comment

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

Do you have any on boarding document that I can read in the meanwhile?

@wli3
Copy link
Author

wli3 commented Jul 7, 2020

Manual tested
WPF-Samples\Graphics\DrawingBrush

Sdk="Microsoft.NET.Sdk.WindowsDesktop"

  • Build on fullmsbuild target net5.0
  • net48
  • Build on core msbuild target net5.0
  • net48

Sdk="Microsoft.NET.Sdk"

  • Build on fullmsbuild target net5.0
  • Net48 -- should not work, have expected error
  • Build on core msbuild target net5.0
  • Net48 -- should not work, have expected error

And the inner build still uses the pbt just created.

@ryalanms

@ryalanms
Copy link
Member

ryalanms commented Jul 7, 2020

I'm signing off based on the testing you performed we discussed offline. Thanks.

@ryalanms ryalanms merged commit 9f51ab4 into dotnet:master Jul 7, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Apr 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
PR metadata: Label to tag PRs, to facilitate with triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants