-
Notifications
You must be signed in to change notification settings - Fork 44
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
Feedback changes on #80 #84
Changes from 14 commits
c6ed4e6
6541277
12b130d
d95f8f7
e34ebde
e806c09
6297fae
c36d063
324e16c
5cb746f
40c05d6
bd5a49c
7e70979
39f9356
70001b8
6359451
0edd841
41c1560
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
/MaxCPUCount | ||
/BinaryLogger | ||
/DetailedSummary | ||
/Verbosity:minimal | ||
/Verbosity:normal | ||
/NodeReuse:false | ||
/Restore |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,20 +4,27 @@ | |
<MSBuildAllProjects>$(MSBuildAllProjects);$(MSBuildThisFileFullPath)</MSBuildAllProjects> | ||
</PropertyGroup> | ||
|
||
<!-- | ||
Only MSBuild Framework build includes Microsoft.Xaml.targets which redirects to MSBuildFrameworkToolsPath. | ||
So to have Xaml Support, we need to use those targets instead of .NETCoreSdk targets | ||
--> | ||
<PropertyGroup Condition="$(_SdkDesktopTargets) == ''"> | ||
<_SdkDesktopTargets Condition="'$(_LanguageSourceName)' != 'FSharp'">$(MSBuildExtensionsPath)\$(_SdkVisualStudioVersion)\Bin\Microsoft.$(_LanguageSourceName).targets</_SdkDesktopTargets> | ||
<_SdkDesktopTargets Condition="'$(_LanguageSourceName)' == 'FSharp'">$(MSBuildExtensionsPath)\Microsoft\VisualStudio\v$(_SdkVisualStudioVersion)\FSharp\Microsoft.FSharp.Targets</_SdkDesktopTargets> | ||
<PropertyGroup Condition="'$(MicrosoftNETSdkTargetsPath)' == ''"> | ||
<MicrosoftNETSdkTargetsPath>$(ToolDepsJsonGeneratorProject.TrimEnd('GenerateDeps\GenerateDeps.proj'))</MicrosoftNETSdkTargetsPath> | ||
<MicrosoftNETSdkTargetsPath Condition="!Exists('$(MicrosoftNETSdkTargetsPath)')">$(_NugetFallbackFolder.TrimEnd('..\..\..\..\NuGetFallbackFolder'))</MicrosoftNETSdkTargetsPath> | ||
<MicrosoftNETSdkTargetsPath Condition="!Exists('$(MicrosoftNETSdkTargetsPath)')">$(NETCoreSdkBundledVersionsProps.TrimEnd('..\..\..\Microsoft.NETCoreSdk.BundledVersions.props'))</MicrosoftNETSdkTargetsPath> | ||
</PropertyGroup> | ||
|
||
<PropertyGroup Condition="'$(_FSharpTargetsPath)' == ''"> | ||
<_FSharpTargetsPath>$(MicrosoftNETSdkTargetsPath)Microsoft.NET.Sdk.FSharpTargetsShim.targets</_FSharpTargetsPath> | ||
<_FSharpTargetsPath Condition="!Exists('$(_FSharpTargetsPath)')">$(MicrosoftNETSdkTargetsPath)Microsoft.NET.Sdk.FSharp.targets</_FSharpTargetsPath> | ||
</PropertyGroup> | ||
|
||
<PropertyGroup> | ||
<_SdkLanguageTargets Condition="'$(_LanguageSourceName)' != 'FSharp'">$(MSBuildToolsPath)\Microsoft.$(_LanguageSourceName).targets</_SdkLanguageTargets> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we looking for the F# Targets this way instead of the VS directory? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are these being set to a private variable and imported below instead of using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know how to explain it (in writing) but I can show you. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Still not following... I can see that you may want to add a second import, but I don't think we should be overriding the base language targets if they're the same? At least use the I know there was one specific issue around WPF builds that needed to use the VS's version, but that was already handled? So I'm not really sure why stuff is being searched for via There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You were talking about this?
I wanted to make sure F# would have the same behaviour when under .NET SDK in this SDK too! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We were already doing that here: So why do we need the more complex logic here now? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, we're not doing that there! We're not importing from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand what the issue is -- I thought we had to explicitly import the Visual Studio version for this scenario, not import from the SDK. If all we're doing is importing from the SDK, then we don't need anything and can fall back to the default |
||
<_SdkLanguageTargets Condition="'$(_LanguageSourceName)' == 'FSharp'">$(_FSharpTargetsPath)</_SdkLanguageTargets> | ||
</PropertyGroup> | ||
|
||
<PropertyGroup> | ||
<_SdkLanguageTargetsMissing Condition="!Exists('$(_SdkDesktopTargets)')">true</_SdkLanguageTargetsMissing> | ||
<_SdkLanguageTargetsMissing Condition="!Exists('$(_SdkLanguageTargets)')">true</_SdkLanguageTargetsMissing> | ||
</PropertyGroup> | ||
|
||
<Import Project="$(_SdkDesktopTargets)" Condition="Exists('$(_SdkDesktopTargets)')"/> | ||
<Import Project="$(_SdkLanguageTargets)" Condition="Exists('$(_SdkLanguageTargets)')"/> | ||
<Import Project="$(MSBuildThisFileDirectory)CheckMissing.targets" Condition="'$(_SdkLanguageTargetsMissing)' == 'true'"/> | ||
|
||
</Project> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be utf-8-bom
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It caused some problems on my machine, so I made sure all the files would be
utf-8
using these tools.