Update most Batch projects to VS17 #3130

matthchr · 2017-04-26T01:13:28Z

Description

This checklist is used to make sure that common guidelines for a pull request are followed.

I have read the contribution guidelines.
The pull request does not introduce breaking changes.

General Guidelines

Title of the pull request is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

Pull request includes test coverage for the included changes.

SDK Generation Guidelines

If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.
The generate.cmd file for the SDK has been updated with the version of AutoRest, as well as a link to the swagger spec, used to generate the code.
The project.json and AssemblyInfo.cs files have been updated with the new version of the SDK.

shahabhijeet · 2017-04-26T22:06:09Z

@azuresdkci test this please

matthchr · 2017-04-27T23:22:30Z

@itowlson @darylmsft @xingwu1 -- Feel free to review this if you want. It's just fixing what we already had though (mostly) so not sure it's worth combing through too much.
@shahabhijeet -- you can merge this now, it gives me a good starting point for doing our actual SDK work.

shahabhijeet · 2017-04-28T04:28:24Z

src/SDKs/Batch/DataPlane/Azure.Batch/Azure.Batch.csproj

+
+  <PropertyGroup Condition=" '$(TargetFramework)' == 'netstandard1.4' ">
+    <!-- Portable is defined for legacy reasons, if updating AutoRest removes it from BatchErrorException we can remove it here -->
+    <DefineConstants>$(DefineConstants);netstandard14;PORTABLE</DefineConstants>


@matthchr portable is no longer a library type we support, so adding a constant that says this code is Portable is misleading.
Rather either your code targets full desktop for that we have used the constant as FullNetFx (so the idea is this can be any version .NET 4.5.2 or .NET 4.6 or !FullNetFx (which is essentially .NET Core/.NET Standard

The issue is that we used an older version of AutoRest (just before it switched to the npm distribution model) to generate our code and in the generated code it includes this (PORTABLE). I agree that it's confusing but until we update to use a new AutoRest and regenerate we need it.

@matthchr ok sounds good, would be great to either have a item that is being tracked as to when do you plan to do this work and add it to this PR.
We had put work in AutoRest and SDKs to remove such compiler constants, so I would like this constant to go away from the repo.

shahabhijeet · 2017-04-28T04:29:37Z

src/SDKs/Batch/DataPlane/Azure.Batch/Azure.Batch.csproj

+
+  <PropertyGroup>
+    <TargetFrameworks>net452;netstandard1.4</TargetFrameworks>
+    <GeneratePackageOnBuild>True</GeneratePackageOnBuild>


@matthchr any reason you need all these properties, which are already defined for the entire repo?
Any reason you are adding all this properties inside the .csproj?
This just makes this entire project file not to use the common properties that are set up for the entire repo?

What is "All these properties"? The only things included in AzSdk.Reference.props are signing and the client runtime references. I've removed the signing stuff I had here, but that's only 3 lines, the rest we still need right...?

shahabhijeet · 2017-04-28T04:30:47Z

src/SDKs/Batch/DataPlane/Azure.Batch/Azure.Batch.csproj

+
+  </ItemGroup>
+
+  <ItemGroup Condition=" '$(TargetFramework)' == 'netstandard1.4' ">


twice declaring for .net standard 1.4?

shahabhijeet · 2017-04-28T04:33:19Z

...ane/Tools/ObjectModelCodeGeneration/ObjectModelCodeGenerator/ObjectModelCodeGenerator.csproj

+  </ItemGroup>
+
+  <ItemGroup>
+    <Compile Update="CollectionConversionSnippet.cs">


Isn't project system picking your files without you explicitly adding them?

No, we need these entries because these are cs files genereated from T4 text templates. I didn't add this to this file manually, VS did when I added the T4 text template files (.tt), so i assume that VS needs it (probably for the DesignTime, AutoGen, and DependentUpon annotations

shahabhijeet · 2017-04-28T04:43:38Z

src/SDKs/Batch/DataPlane/Tools/ConfigureAwaitAnalyzer/ConfigureAwaitAnalyzer.Tests/app.config

+<configuration>
+  <runtime>
+    <assemblyBinding xmlns="urn:schemas-microsoft-com:asm.v1">
+        <!--<dependentAssembly>


Do you still need this file after commenting the code?

Good catch -- deleted

shahabhijeet · 2017-04-28T04:45:18Z

...ools/ConfigureAwaitAnalyzer/ConfigureAwaitAnalyzer.Tests/ConfigureAwaitAnalyzer.Tests.csproj

@@ -0,0 +1,57 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <!--<Import Project="$([MSBuild]::GetPathOfFileAbove('AzSdk.test.reference.props'))" />-->
+  <PropertyGroup>


@Matthch you do not need to add all these properties in the project file? Any reason you are adding all these properties?

I copied almost all of this from test.props -- so if I don't need it no other test project needs it too.

I didn't want to use test.props because it includes DisableTestRunParallel.cs which since these tests are all in-memory unit tests is pointless.

If you want, you can update test.props to have a flavor that doesn't have DisableTestRunParallel.cs and I can use that. Although I am not sure why we are bothering to sign our tests either.

- After the move to VS2017 it's no longer needed.

- FileStaging, FileConventions, ConfigureAwaitAnalyzer, OM code generation tools, and the OM integration tests

shahabhijeet · 2017-04-28T16:34:42Z

src/SDKs/Batch/DataPlane/Azure.Batch/Azure.Batch.csproj

+
+  <PropertyGroup Condition=" '$(TargetFramework)' == 'netstandard1.4' ">
+    <!-- Portable is defined for legacy reasons, if updating AutoRest removes it from BatchErrorException we can remove it here -->
+    <DefineConstants>$(DefineConstants);netstandard14;PORTABLE</DefineConstants>


@matthchr ok sounds good, would be great to either have a item that is being tracked as to when do you plan to do this work and add it to this PR.
We had put work in AutoRest and SDKs to remove such compiler constants, so I would like this constant to go away from the repo.

shahabhijeet · 2017-04-28T16:42:26Z

src/SDKs/dirs.proj

@@ -60,6 +61,8 @@
 		<ProjectToBuild Include="$(MSBuildThisFileDirectory)Automation\**\*Tests.csproj" ProjectType="Test"/>

 		<ProjectToBuild Include="$(MSBuildThisFileDirectory)Batch\Management\**\*Tests.csproj" ProjectType="Test"/>
+		<ProjectToBuild Include="$(MSBuildThisFileDirectory)Batch\DataPlane\Client\Tools\ObjectModelCodeGeneration\ObjectModelCodeGenerator\ObjectModelCodeGenerator.csproj" ProjectType="Test"/>


@matthchr we have moved away from the declarative model, so these projects will not make it part of the build system.
Right now we have deliberately made it sure that we build limited target framework projects, we are not honoring random target frameworks for projects to have consistency across our SDK (what we talked earlier)
We can still do it but I would like to avoid as much special casing as possible.
Let's talk separately and we can find a solution for this.

Agree, lets chat.

For right now I've disabled all the projects that don't work in Directory.Build.props, but I would like to get them enabled again. Let's chat today about what we can do.

matthchr · 2017-04-28T17:27:09Z

@shahabhijeet For the issue of PORTABLE I took a look and the latest AutoRest calls the flag LEGACY instead. So even if I upgraded to use the latest AutoRest (which maybe we will do here shortly) it still isn't called FullNetFx

Given that this property is in generated code (and even the newest AutoRest isn't using the same one the build is) I think we should punt on this for now. I can and will fix it, but you should coordinate with AutoRest about what this flag should be called (for what it's worth I like your FullNetFx better than Legacy.

shahabhijeet · 2017-04-28T19:35:31Z

@matthchr if AutoRest is using something called "Legacy", I need to know about it as to what "Legacy" mean here. It is even more confusing than portable :)
So let me start an email thread and let's figure out what this "legacy" means.
Also to make sure you are using the latest AutoRest for this?

shahabhijeet · 2017-04-28T20:10:02Z

@matthchr can you link the new issue to undo the legacy compiler constant once AutoRest fixes it on their end.
I think let's just get this huge PR in. So can you link 3 issues that we talked about:

Undoing compiler constants
Special casing for few of your projects that are targeting .net 46 (please specify the reasons why it has to target .NET 46 and cannot be done for .net 452
This issue will also need cleaning up of .csproj files once these projects enabled for CI.

For now these special casing projects will not be part of the build system, but will remain in the repo.
Once you have these 2 issues linked, we should be able to accept this PR.

matthchr · 2017-04-28T21:18:41Z

Issues here: #3139 and #3140

shahabhijeet · 2017-04-28T22:00:39Z

looks good.

matthchr · 2017-04-28T22:49:15Z

@shahabhijeet Feel free to merge when you're ready.

msftclas added the cla-required label Apr 26, 2017

shahabhijeet added cla-not-required and removed cla-required labels Apr 26, 2017

updating dataplane for batch

97e713d

matthchr force-pushed the feature/batch-vs17 branch 4 times, most recently from e171dd0 to 8032437 Compare April 27, 2017 20:05

matthchr changed the title ~~[DO NOT MERGE] Feature/batch vs17~~ Update most Batch projects to VS17 Apr 27, 2017

shahabhijeet suggested changes Apr 28, 2017

View reviewed changes

matthchr added 7 commits April 27, 2017 23:01

Initial pass at Batch in VS17

c2e755c

Delete ClientIntegrationTesting folder

203b1be

- After the move to VS2017 it's no longer needed.

Fix static analysis test path

f56449a

Temporarily disable Batch Analyzer

b5a8678

Add release notes reference to Batch packages

1a15e0a

Fix Batch generate.cmd files

97a5991

Temporarily ignore some batch projects on official build

815e272

- FileStaging, FileConventions, ConfigureAwaitAnalyzer, OM code generation tools, and the OM integration tests

matthchr force-pushed the feature/batch-vs17 branch from 8032437 to 815e272 Compare April 28, 2017 06:02

shahabhijeet suggested changes Apr 28, 2017

View reviewed changes

Enable WarningAsError on Batch projects

20e2d74

shahabhijeet merged commit 79dbda2 into Azure:vs17Dev Apr 28, 2017

matthchr deleted the feature/batch-vs17 branch April 29, 2017 00:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update most Batch projects to VS17 #3130

Update most Batch projects to VS17 #3130

matthchr commented Apr 26, 2017 •

edited

Loading

shahabhijeet commented Apr 26, 2017

matthchr commented Apr 27, 2017 •

edited

Loading

shahabhijeet Apr 28, 2017

matthchr Apr 28, 2017 •

edited

Loading

shahabhijeet Apr 28, 2017

shahabhijeet Apr 28, 2017

matthchr Apr 28, 2017

shahabhijeet Apr 28, 2017

matthchr Apr 28, 2017

shahabhijeet Apr 28, 2017

matthchr Apr 28, 2017 •

edited

Loading

shahabhijeet Apr 28, 2017

matthchr Apr 28, 2017

shahabhijeet Apr 28, 2017

matthchr Apr 28, 2017

shahabhijeet Apr 28, 2017

shahabhijeet Apr 28, 2017

matthchr Apr 28, 2017

matthchr commented Apr 28, 2017

shahabhijeet commented Apr 28, 2017

shahabhijeet commented Apr 28, 2017

matthchr commented Apr 28, 2017

shahabhijeet commented Apr 28, 2017

matthchr commented Apr 28, 2017


		</ItemGroup>

		<ItemGroup Condition=" '$(TargetFramework)' == 'netstandard1.4' ">

Update most Batch projects to VS17 #3130

Update most Batch projects to VS17 #3130

Conversation

matthchr commented Apr 26, 2017 • edited Loading

Description

shahabhijeet commented Apr 26, 2017

matthchr commented Apr 27, 2017 • edited Loading

Choose a reason for hiding this comment

matthchr Apr 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthchr Apr 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthchr commented Apr 28, 2017

shahabhijeet commented Apr 28, 2017

shahabhijeet commented Apr 28, 2017

matthchr commented Apr 28, 2017

shahabhijeet commented Apr 28, 2017

matthchr commented Apr 28, 2017

matthchr commented Apr 26, 2017 •

edited

Loading

matthchr commented Apr 27, 2017 •

edited

Loading

matthchr Apr 28, 2017 •

edited

Loading

matthchr Apr 28, 2017 •

edited

Loading