-
Notifications
You must be signed in to change notification settings - Fork 382
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
Move to MSBuild tooling #144
Conversation
Hi @galvesribeiro, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! The agreement was validated by Microsoft and real humans are currently evaluating your PR. TTYL, MSBOT; |
For whatever reason on appveyor we are getting that error:
It build just fine here... Trying to find someone from .Net SDK Team to check this out... |
Changed the build script to install |
@galvesribeiro - Thanks for the contribution. Before I do a full CR I noticed that you are removing support for .NET 45. Can I ask why? Some of our users want that to our knowledge and the package should continue to be supported. |
@jterry75 Hey, I tried to get on If this is a requirement, no problem, I'll change it to multitarget just as before. In all cases, you can review the code changes since they are minimal. All I did was remove the I'll add them back and update the PR but feel free to start reviewing. Thanks |
@jstarks - Do we know if we can update to net 4.6 or not? |
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.
Good start! I didnt review the code to remove the net45 or 46 pending what @jstarks says on the main thread.
<GenerateAssemblyInfo>false</GenerateAssemblyInfo> | ||
</PropertyGroup> | ||
<ItemGroup> | ||
<Compile Include="**\*.cs" /> |
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.
This can be removed right? It will pickup all *.cs in the project folders.
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.
If we use the latest bits from the SDK, we can remove it however, it is broken. Visual Studio don't work with it, just the CLI. I'm looking at it with the proper team here dotnet/sdk#604
The version I added on AppVeyor scripts is the preview4
which is the stable version of the MSBuild sdk for .net core. And that require this globbing.
When they fix that issue (scheduled for the next release of the SDK probably) I can remove it and update the build script.
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.
Ok no worries we can update it later.
@@ -0,0 +1,21 @@ | |||
<Project Sdk="Microsoft.NET.Sdk" ToolsVersion="15.0"> |
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.
Want to put this under tests/Docker.DotNet.Tests
instead? Really the proper folder layout would be to move the project under src/
and then test/
as a peer.
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.
But great to add the test project now!
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 was about to do that, but I first wanted you guys to easily review it so the diff here is easier without a bunch of delete/move that GH will do.
Once you give the OK on this PR, I'll reset and push again with the src/ and test/ folders in place.
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.
Awesome
<package xmlns="http://schemas.microsoft.com/packaging/2010/07/nuspec.xsd"> | ||
<metadata> | ||
<id>Docker.DotNet.BasicAuth</id> | ||
<version>2.124.3</version> |
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.
We probably should update the AppVeyor to change this at build to maintain the dev build numbers.
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 was trying to just get the nuget metadata from the project.json
to a .nuspec the way it was in the repo. But yes, will look on how to do that with appVeyor once I have everything else approved.
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.
Im actually confused here. What I was remembering was our other project AppVeyor.yaml where you see we crack the .json
in order to maintain a dev build. The one in the .nuspec
is for the released version.
@swernli - Are we not doing dev builds for this AppVeyor here too?
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.
We can, like I said, that will be updated to accept the build number as a parameter from the one calling the build (i.e. AppVeyor)
@@ -19,20 +15,6 @@ public static X509Certificate2 GetCertFromPFX(string pfxFilePath, string passwor | |||
return new X509Certificate2(pfxFilePath, password); | |||
} | |||
|
|||
#if (NET45 || NET46) | |||
public static X509Certificate2 GetCertFromPFXSecure(string pfxFilePath, SecureString password) |
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.
Can you leave these functions. It is additional support we want to add when netstandard supports it.
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.
Ok, will get all the #if
back.
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.
Btw, X509Certificate2
is supported on netstandard1.1
+ (which has net45
in it). However, SecureString
is netstandard1.3
(which includes net46
) so if staying with netstandard1.3
we keep this just fine.
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.
Yea its not X509Certificate2 its actually the .PrivateKey that I remember not being there. We need to support reading keys from .PEM files so we cant remove this
@@ -1,39 +1,93 @@ | |||
| |||
Microsoft Visual Studio Solution File, Format Version 12.00 | |||
# Visual Studio 14 | |||
VisualStudioVersion = 14.0.25123.0 | |||
# Visual Studio 15 |
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 don't love requiring VS 15 but oh well. We can still use the command line I guess
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.
Yeah, unfortunately the MSBuild
tooling require VS15 :( And yes, if you don't want VS to build, you can use command line.
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Solution Items", "Solution Items", "{9ADC524F-FC79-4D05-9536-37BB4D426C0F}" | ||
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Docker.DotNet.Tests", "Docker.DotNet.Tests\Docker.DotNet.Tests.csproj", "{3225A078-3F6C-4293-B580-1EA3203EA3E8}" | ||
EndProject | ||
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "NuGet", "NuGet", "{5B3E2DDE-7EF4-4E55-A0BF-05EF43048336}" |
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.
This is new to me. Why add the nuspec's to the solution? Is that a new csproj requirement?
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 was just used to do it so it is handy to edit. But I'll remove.
{ | ||
_proxy = WebRequest.DefaultWebProxy; | ||
} | ||
//Need check what is the netstandard replacement for this |
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.
Last I checked there wasn't a good one but if you find it that would be great.
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.
@jterry75 Please check my comments. I'm just waiting the decision if will stay on |
@galvesribeiro - Talked to @jstarks, we need to keep the support for .NET 45 and .NET 46 for now so can we put the |
@jterry75 Ok, absolutely. Will put the |
Hey @jterry75, it is done. Sorry for the delay. All feedback was addressed. The only thing that I don't know much is that AppVeyor version #. I never used AppVeyor before, so I appreciate any guidance on change that script to get the proper version. Please let me know if you need anything else. Once I had it merged I'll tackle the other tasks. Thanks |
@jterry75 can we move forward? I want to finish this one to get moving to the other PRs. |
LGTM. 1 question before merge. Do we have to have a .nuspec and a .csproj? The .csproj file includes all the nuget info already. Seems like duplicate work |
@jterry75 the idea is to use only the .csproj however, we can't (yet) have a project that is at development time referencing another and at production referencing a nuget package (i.e X509 and BasicAuth that references the Docker.DotNet). I left the project pre-ready to be stand alone. While we are waiting for it to work from the .Net SDK team. We will remove later on the .nuspec and keep the .csproj only. |
Ok cool. |
Thanks! Merged this |
Moving on :) |
As mentioned in #143, starting by migrating projects.