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

Feedback changes on #80 #84

Merged
merged 18 commits into from
Jun 24, 2018
Merged

Feedback changes on #80 #84

merged 18 commits into from
Jun 24, 2018

Conversation

Nirmal4G
Copy link
Contributor

@Nirmal4G Nirmal4G commented Jun 16, 2018

Fix Local Build
Fix NuSpec less packaging
Upstream (build env) patches
  - SourceControlInformation fix
  - Add some Labels
@Nirmal4G
Copy link
Contributor Author

Nirmal4G commented Jun 17, 2018

User Facing Properties:

  1. ExtrasUwpMetaPackageVersion -> _ImplicitPlatformPackageVersion + TargetFramework condition
  2. ExtrasImplicitPlatformPackageDisabled -> DisableImplicitFrameworkReferences + TargetFramework condition
  3. ExtrasImplicitPlatformPackageIsPrivate -> Auto set for library like items
  4. SuppressWarnIfOldSdkPack -> IgnoreOldSdkWarning
  5. EmbeddedResourceGeneratorVisibilityIsInternal -> ! EmbeddedResourceGeneratedCodeIsPublic
  6. _SdkSetAndroidResgenFile -> IncludeAndroidResgenFile

Every other property that is changed and not mentioned above is not supposed to be user facing.

I propose we keep the new names instead of renaming all over the place. I tested at the best of my ability to know that these new names don't interfere with the build process. But to be sure we can test some more with the projects like windows toolkit, package explorer, msbuild log viewer, any other projects etc... to find any issues during the course of this PR.

And not to break other user's builds we can provide a shim properties until a major version (say 2.0) and an optional warning to use the new names.

Nirmal4G added 5 commits June 20, 2018 12:49
No Longer needed since we need VS/MSBuild 15 and above
Silverlight and WP targets are not affected!
So that we won't clash with other names that matches these!
upstream patch for Identifier check
comments clean up
upstream patch:
fixed EmbeddedResource
Added section comments
Some people really need it!!!
And no, they don't want to update to net472
@Nirmal4G Nirmal4G changed the title [WIP] Feedback changes on #80 Feedback changes on #80 Jun 20, 2018
@clairernovotny
Copy link
Collaborator

After this, are there other changes, or does this mostly complete your updates for now?

@@ -6,6 +6,7 @@ root = true

# Don't use tabs for indentation.
[*]
charset = utf-8
Copy link
Collaborator

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

Copy link
Contributor Author

@Nirmal4G Nirmal4G Jun 20, 2018

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.

</When>

<!-- Check for _*_wpftmp.*proj files -->
<When Condition="$([System.Text.RegularExpressions.Regex]::IsMatch('$(MSBuildProjectName)', '$(_GeneratedProjectSuffixPattern)%24'))">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why use a regex here instead of checking/using the presence _TargetAssemblyProjectName. What does that gain?

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 we know for sure that the generated project use only _TargetAssemblyProjectName and it is only present in the generated project? If that's so I can remove the RegEx check.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That property is defined only in the WPF generation task, so it's a safe bet for now that it's only there in the 2nd pass compile.

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, then, I'll remove them

@@ -16,10 +16,10 @@
-->
<Target Name="_CheckAndFixupRootNamespaceWithGeneratedSuffix"
BeforeTargets="_CompileTemporaryAssembly;CoreCompile"
Condition="('$(IsGeneratedProject)' == 'true') AND $([System.Text.RegularExpressions.Regex]::IsMatch('$(RootNamespace)', '$(GeneratedProjectSuffixPattern)'))">
Condition="'$(IsGeneratedProject)' == 'true' AND $([System.Text.RegularExpressions.Regex]::IsMatch('$(RootNamespace)', '$(_GeneratedProjectSuffixPattern)'))">
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we need a regex, can we store the result so we don't have to evaluate it multiple times. That's likely expensive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do I do it? Is it possible to store RegEx object in a MSBuild property?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd store the results of a regex by storing the result in a property instead of using directly in a condition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Outside of the target right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, I think that would work. but you may not need it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we don't set the RootNamespace it defaults to MSBuildProjectName. If it contains invalid chars or suffix for generated projects and if we dynamically transform existing RootNamespace (i.e. MSBuildProjectName) to the correct one, the suffix flows through. From there all hell breaks loose! Believe me I've had my fair share.

Choose a reason for hiding this comment

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

I feel like this is better off as a StartsWith string comparison rather than a RegEx? Namespaces/Project Filenames contain "." characters which match any character in Regex plus this isn't using the "^" Start of String anchor, each potentially matching false positives. For example "My.Project.Name" would match "This.IsNot.My_Project_Namespace" in this scenario, which wouldn't be desired whereas "^My.Project.Name" would match things like "My.Project.Name.Sub.Namespace" (though it would also match "My.Project.Namespace").

@Nirmal4G
Copy link
Contributor Author

Nirmal4G commented Jun 20, 2018

After this, are there other changes, or does this mostly complete your updates for now?

I have asked @GeertvanHorrik for checking his multitarget template with the latest code #43. Once he confirms it's working without any issues we can merge this.

Can we remove DependentUpon since the DynamicDependentFile Capability support is good in 15.6+?

@clairernovotny
Copy link
Collaborator

What does DynamicDependentFile do, and how is it configured I haven't come across that yet. If we don't need the DepenentUpon anymore, that's good too.

We should have both shims and release notes since we don't want to break people.

@Nirmal4G
Copy link
Contributor Author

We should have both shims and release notes since we don't want to break people.

Ok. then I'll get right on it! Again I'm so sorry to force such large change to you. I know this breaks git flow. I assure you this will be the last large change.

@clairernovotny
Copy link
Collaborator

Sounds like DynamicDependentFile does what we need. Lets just make sure we have the appropriate ProjectSchemaDefinitions to handle Setttings, WinForms resx and resx with a designer and Xaml, then that's even better.

@Nirmal4G
Copy link
Contributor Author

Nirmal4G commented Jun 20, 2018

I already added the support for Xaml. But have to check for WinForms. Can I add the changes here?

Seems like it's not working properly, will skip this for now!

Nirmal4G added 4 commits June 21, 2018 01:16
upstream patches:
Added Nesting and Dependent files support
Workaround for CodeAnalysis, Intellisense, etc...
Move packaging related props
Use '_TargetAssemblyProjectName'
Generalize Fixup target
Drop '_Sdk' for user facing properties.
Take 2:
DynamicDependentFile not working, keep static evaluation for now!
Include missing ProjectSystem.targets file
@GeertvanHorrik
Copy link
Contributor

I am having a bit more time now. What's the easiest way to test this PR?

Since, IsGeneratedProject is not supposed to be a user facing property!
@Nirmal4G
Copy link
Contributor Author

Nirmal4G commented Jun 23, 2018

@GeertvanHorrik

You can do this:

  1. You can put your project folder into TestProjects folder in the repo and make your changes, it'll automatically picks up latest extras when you build the repo solution first and then yours.

  2. You can clone my fork and build the solution and you'll get NuGet package copied on to a temp folder, you can grab a NuGet.config file from my fork and put it in your repo, that should bring in the package from temp folder.

  3. I'll drop a nuget package in the MyGet, you can grab that!

@GeertvanHorrik
Copy link
Contributor

Steps taken:

  1. Copy project to TestProjects directory
  2. Remove the debug lines at the top of the (multitargeting) project
  3. Run .\build.ps1 -target package

Without the debug lines, it doesn't seem to work:

C:\Source\Forks_MSBuildSdkExtras\TestProjects\MultiTargetingProject\src\Ghk.MultiTargeting\Platforms\uap10.0\Views\MyView.xaml(1,23): error MC3074: The tag
 'UserControl' does not exist in XML namespace 'using:Catel.Windows.Controls'. Line 1 Position 23. [C:\Source\Forks_MSBuildSdkExtras\TestProjects\MultiTarg
etingProject\src\Ghk.MultiTargeting\Ghk.MultiTargeting.csproj]
C:\Source\Forks_MSBuildSdkExtras\TestProjects\MultiTargetingProject\src\Ghk.MultiTargeting\Platforms\uap10.0\Views\MyView.xaml(1,23): error MC3074: The tag
 'UserControl' does not exist in XML namespace 'using:Catel.Windows.Controls'. Line 1 Position 23. [C:\Source\Forks_MSBuildSdkExtras\TestProjects\MultiTarg
etingProject\src\Ghk.MultiTargeting\Ghk.MultiTargeting.csproj]
An error occurred when executing task 'Build'.
Error: One or more errors occurred.
        MSBuild: Process returned an error (exit code 1).

Anything specific I have to do to make sure I am using the latest stuff?

@GeertvanHorrik
Copy link
Contributor

Ok, just read your guide, I didn't build your version first. Let me do that real quick.

@GeertvanHorrik
Copy link
Contributor

Final steps:

  1. Clone your fork
  2. Copy project to TestProjects directory
  3. Remove the debug lines at the top of the (multitargeting) project
  4. Remove the version number from the definition so it picks the latest by default
  5. Run .\build.ps1 -target package
Microsoft (R) Build Engine version 15.7.177.53362 for .NET Framework
Copyright (C) Microsoft Corporation. All rights reserved.

Some command line switches were read from the auto-response file "MSBuild.rsp". To disable this file, use the "/noautoresponse" switch.
C:/Program Files (x86)/Microsoft Visual Studio/2017/Professional/MSBuild/15.0/Bin/MSBuild.exe /DetailedSummary /BinaryLogger /MaxCPUCount /NodeReuse:false /p:Configuration=Release /p:Platform=Any CPU /Restore /target:Build /v:quiet C:/Source/Forks_MSBuildSdkExtras/TestProjects/MultiTargetingProject/src/Ghk.MultiTargeting.sln
C:\WINDOWS\Microsoft.NET\Framework\v4.0.30319\Microsoft.WinFx.targets(268,9): error MC6000: Project file must include the .NET Framework assembly 'WindowsB
ase, PresentationCore, PresentationFramework' in the reference list. [C:\Source\Forks_MSBuildSdkExtras\TestProjects\MultiTargetingProject\src\Ghk.MultiTarg
eting\Ghk.MultiTargeting.csproj]
C:\WINDOWS\Microsoft.NET\Framework\v4.0.30319\Microsoft.WinFx.targets(268,9): error MC6000: Project file must include the .NET Framework assembly 'WindowsB
ase, PresentationCore, PresentationFramework' in the reference list. [C:\Source\Forks_MSBuildSdkExtras\TestProjects\MultiTargetingProject\src\Ghk.MultiTarg
eting\Ghk.MultiTargeting.csproj]
An error occurred when executing task 'Build'.
Error: One or more errors occurred.
        MSBuild: Process returned an error (exit code 1).

Then I checked out your examples, and found that I need to define this:

    <EnableWpfProjectSetup>true</EnableWpfProjectSetup>

Questions:

A. Would it be possible to make this work out of the box?
B. Does enabling this affect any of the other targets?

One option is to add it to the wpf only targets in the implicit targets file in the root of the template. Just tested that and that works great :-)

@Nirmal4G
Copy link
Contributor Author

Nirmal4G commented Jun 23, 2018

Anything specific I have to do to make sure I am using the latest stuff?

  1. Try removing all the workarounds.
  2. For WPF projects set EnableWpfProjectSetup to true
  3. For UWP projects set EnableDefaultXamlItems to true
  4. For Public Generators set <TypeOfGenerator>GeneratedCodeIsPublic to true
    E.g.: **EmbeddedResource**GeneratedCodeIsPublic

@GeertvanHorrik
Copy link
Contributor

Excellent, here is the commit with the changes:

GeertvanHorrik/MultiTargetingProject@0b7a8de

@GeertvanHorrik
Copy link
Contributor

Not sure I understand point 4, maybe you can explain that one?

@Nirmal4G
Copy link
Contributor Author

Nirmal4G commented Jun 23, 2018

A. Would it be possible to make this work out of the box?

The problem is enabling different type of projects (for a single framework) needs some intervention, like setting up default items, implicit references, some property overrides etc...

B. Does enabling this affect any of the other targets?

No. That's why the explicit properties.
One side-effect is that when @microsoft and @dotnet releases .NETCore 3 with WinForms/WPF support, we can use the same property to setup the WinForms/WPF behaviour (though the actual process would be different but what the user does to enable with this SDK will be the same) just as now we're enabling for the full framework.

@Nirmal4G
Copy link
Contributor Author

Nirmal4G commented Jun 23, 2018

Not sure I understand point 4, maybe you can explain that one?

Some of the Generators like SettingsSingleFileGenerator, ResXFileCodeGenerator support making the generated code public (default - internal), this switch enables that.

@Nirmal4G
Copy link
Contributor Author

Nirmal4G commented Jun 23, 2018

@clairernovotny
Copy link
Collaborator

@Nirmal4G When do you think you'll have the compat shims and docs updated so we can close this PR out?

Nirmal4G added 2 commits June 24, 2018 13:21
Remove Test Hooks
Give a common property
just like every other custom tool so that we can keep track of it!
@Nirmal4G
Copy link
Contributor Author

@onovotny The changes are done, but adding shims and the warnings make Extras SDK a little bit noticeably slower. Is this OK?

@clairernovotny
Copy link
Collaborator

Why would adding shims slow things down, they're additional variables? We cannot break people on upgrade.

Nirmal4G added 2 commits June 24, 2018 18:39
for projects that require full MSBuild!
Issue a warning when using those properties
</PropertyGroup>

<!-- Check to see if we are using msbuild core! -->
<Target Name="_WarnWhenUsingMSBuildCore"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is slowing the build down!

Copy link
Collaborator

Choose a reason for hiding this comment

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

How are you measuring the timings?

Copy link
Contributor Author

@Nirmal4G Nirmal4G Jun 24, 2018

Choose a reason for hiding this comment

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

msbuild /bl /DetailedSummary and .binlog too!

<Target
Name="_WarnWhenUsingObsoleteProperties"
BeforeTargets="_CheckForInvalidConfigurationAndPlatform"
Condition="'$(_IgnoreObsoletePropertyWarnings)' != 'true'">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This target is also slowing the build a bit!

For now it only includes migration path from old SDK version.
@clairernovotny
Copy link
Collaborator

As a general rule, we should not be using StartsWith any more than necessary. We should be using the TargetPlatformIdentifier with an equals comparison since that's faster.

@Nirmal4G
Copy link
Contributor Author

I'm not good at writing docs as English is not my first language. Any help with docs is much appreciated.

@Nirmal4G
Copy link
Contributor Author

As a general rule, we should not be using StartsWith any more than necessary. We should be using the TargetPlatformIdentifier with an equals comparison since that's faster.

Since we're dealing with multiple platforms, I've reduced them to platform wise and made comparisons and this is much faster than checking TFI with every value every time.

If you know anyone who can give Perf review for these patches, reel'em in, that'd be very much helpful.

@Nirmal4G
Copy link
Contributor Author

Both the targets add approximately 150ms each to the Build!

@clairernovotny
Copy link
Collaborator

I think StartsWith is much slower than doing an equals comparison with multiple OR's. Those conditions get evaluated a lot.

Will work once the PR is merged
@clairernovotny
Copy link
Collaborator

the binlog in msbuildlog may have more detailed evaluation numbers

@Nirmal4G
Copy link
Contributor Author

I did that version too, It takes about 350ms. I don't know how, but got reduced to 150ms for the above version!

the binlog in msbuildlog may have more detailed evaluation numbers

This is from that but just those targets!

@Nirmal4G
Copy link
Contributor Author

I think that's just about it. You can merge anytime.

@clairernovotny
Copy link
Collaborator

Why do the sample projects have ProjectGuid's in them?

@Nirmal4G
Copy link
Contributor Author

See...
#80 (comment)
#80 (comment)

@clairernovotny clairernovotny merged commit f77f39f into novotnyllc:master Jun 24, 2018
@Nirmal4G Nirmal4G deleted the pr/feedback branch June 24, 2018 19:36
</PropertyGroup>

<PropertyGroup>
<_SdkLanguageTargets Condition="'$(_LanguageSourceName)' != 'FSharp'">$(MSBuildToolsPath)\Microsoft.$(_LanguageSourceName).targets</_SdkLanguageTargets>
Copy link
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Collaborator

Choose a reason for hiding this comment

The 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 LanguageTargets?

https://github.com/onovotny/MSBuildSdkExtras/blob/rel/1.5/MSBuild.Sdk.Extras/build/platforms/languageTargets/Wpf.targets#L9-L10

Copy link
Contributor Author

@Nirmal4G Nirmal4G Jun 25, 2018

Choose a reason for hiding this comment

The 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.

Desktop.targets#L27-L41 on MSBuild.NET.Extras.Sdk

Copy link
Collaborator

Choose a reason for hiding this comment

The 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 LanguageTargets value that's provided by default?

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 MicrosoftNETSdkTargetsPath?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You were talking about this?

Why are we looking for the F# Targets this way instead of the VS directory?

I wanted to make sure F# would have the same behaviour when under .NET SDK in this SDK too!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we're not doing that there! We're not importing from Microsoft.NET.Sdk

Copy link
Collaborator

Choose a reason for hiding this comment

The 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 LanguageTargets?

@clairernovotny
Copy link
Collaborator

Also, why are the VB defines treated differently, with the =-1?

  <PropertyGroup Condition="'$(DisableImplicitFrameworkDefines)' != 'true'">
    <ImplicitFrameworkProfileDefine Condition="'$(_SdkLanguageSourceName)' != 'VisualBasic'">$(TargetFrameworkProfile.ToUpperInvariant());$(ImplicitFrameworkIdentifierDefine)$(TargetFrameworkProfile.Substring('7'))</ImplicitFrameworkProfileDefine>
    <ImplicitFrameworkProfileDefine Condition="'$(_SdkLanguageSourceName)' == 'VisualBasic'">$(TargetFrameworkProfile.ToUpperInvariant())=-1,$(ImplicitFrameworkIdentifierDefine)$(TargetFrameworkProfile.Substring('7'))=-1</ImplicitFrameworkProfileDefine>
  </PropertyGroup>

@Nirmal4G
Copy link
Contributor Author

Also, why are the VB defines treated differently, with the =-1?

That's a Q for the language team!

VB is different for historical reasons!!

@clairernovotny
Copy link
Collaborator

VB is different for historical reasons!!

When I looked at what the main SDK does, I don't see it setting things to =-1. The only difference I see for VB is using , instead of ; as a separator.

@Nirmal4G
Copy link
Contributor Author

Nirmal4G commented Jun 25, 2018

FinalDefineConstants is what VB uses.

Ok, I get what you're asking, I should've the property named Defines plural, that would clear the confusion. It mimics FinalDefineConstants.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants