Skip to content

Conversation

@clcrutch
Copy link
Contributor

@clcrutch clcrutch commented Mar 14, 2018

Cake Addin for getting Nerdbank.GitVersioning VersionOracle.

This is a cake addin for getting the VersionOracle for the current git repo.

Fixes #148

@clcrutch
Copy link
Contributor Author

clcrutch commented Mar 14, 2018

I'm currently working on addressing these build failures. Will report back. #Closed

@clcrutch
Copy link
Contributor Author

clcrutch commented Jun 2, 2018

Sorry for the delay in getting the build issues addressed. #Closed

Copy link
Collaborator

@AArnott AArnott left a comment

Choose a reason for hiding this comment

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

I've made several changes. Can you validate using cake that the built package still works?

<PackageLicenseUrl>https://raw.githubusercontent.com/AArnott/Nerdbank.GitVersioning/39ac76891c/LICENSE.txt</PackageLicenseUrl>
<PackageRequireLicenseAcceptance>true</PackageRequireLicenseAcceptance>
<PackageProjectUrl>http://github.com/aarnott/Nerdbank.GitVersioning</PackageProjectUrl>
<SignAssembly>false</SignAssembly>
Copy link
Collaborator

@AArnott AArnott Jun 2, 2018

Choose a reason for hiding this comment

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

Did signing mess anything up? Otherwise, I'd prefer we sign it. #Closed

Copy link
Collaborator

@AArnott AArnott Jun 2, 2018

Choose a reason for hiding this comment

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

I see... Cake.Core itself isn't strong name signed. #Closed

Copy link
Contributor Author

@clcrutch clcrutch Jun 2, 2018

Choose a reason for hiding this comment

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

Yeah, I initially attempted to sign the assembly, but also discovered that Cake.Core was not signed. I would be willing to submit an issue to the Cake team to see if they'd be willing to strong name sign Cake.Core. I am unsure if that would affect anything down stream of them though. #Closed

Copy link
Collaborator

@AArnott AArnott Jun 2, 2018

Choose a reason for hiding this comment

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

It probably would impact their existing users. And my guess for a package that popular is that if they're not signed, they probably are opposed to the idea. It's OK. We can fall in line for the cake plugin. #Closed


<ItemGroup>
<PackageReference Include="Cake.Core" Version="0.26.0" />
<PackageReference Include="Nerdbank.GitVersioning.LKG" Version="1.6.20-beta-gfea83a8c9e" />
Copy link
Collaborator

@AArnott AArnott Jun 2, 2018

Choose a reason for hiding this comment

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

I've modified both of these to suppress the dependency #Closed

@AArnott
Copy link
Collaborator

AArnott commented Jun 2, 2018

@clcrutch Can you confirm that the package we're building after my changes to your PR works? #Closed

@AArnott
Copy link
Collaborator

AArnott commented Jun 2, 2018

Also, can you update the docs in this repo to include something about this feature? #Closed

@clcrutch
Copy link
Contributor Author

clcrutch commented Jun 2, 2018

@AArnott I just confirmed that the changes still work after your changes. I'll update the docs to include information about this feature and let you know when I am finished. #Closed

@AArnott
Copy link
Collaborator

AArnott commented Jun 21, 2018

@clcrutch Any update on the docs? I've fixed #185 so I'm about ready to accept this PR after your doc addition. #Closed

@clcrutch
Copy link
Contributor Author

clcrutch commented Aug 3, 2018

I've added docs. There are a few mistakes I'm working on correcting. #Closed

@clcrutch
Copy link
Contributor Author

clcrutch commented Aug 3, 2018

I've updated the docs. I think I'm content with them. I'd love some feedback though. #Closed

@clcrutch
Copy link
Contributor Author

clcrutch commented Aug 3, 2018

I'm not sure why the build failed. The build failure appears to happen after it has already built my code. It also builds fine on my end.

image
#Closed

@clcrutch
Copy link
Contributor Author

clcrutch commented Aug 6, 2018

All builds now pass. #Closed

@clcrutch clcrutch closed this Aug 9, 2018
@clcrutch clcrutch reopened this Aug 9, 2018
@clcrutch
Copy link
Contributor Author

clcrutch commented Aug 9, 2018

Sorry about the noise. Was reviewing the pull request and accidentally closed the PR.

Otherwise, I think I'm content with the changes here. Is there anything else that I should do on my end?

Edit: fixed the build errors again, so the statement above is true once more. #Closed

@AArnott
Copy link
Collaborator

AArnott commented Aug 9, 2018

Great. I just got back from a vacation where I had no Internet access, so I'm catching up. I look forward to reviewing this again and merging. #Closed

@AArnott
Copy link
Collaborator

AArnott commented Aug 20, 2018

Why are all these markdown files that describe the public API checked into source? If they were generated, they need to be generated during the build--not as a once off plus checkin. Otherwise it becomes a maintenance nightmare. Can that be done?

@AArnott
Copy link
Collaborator

AArnott commented Aug 20, 2018

(just to be clear, when I asked for documentation I was only asking for the doc/cake.md file that you added.)

EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "nbgv", "nbgv\nbgv.csproj", "{EF4DAF23-6CE9-48C5-84C5-80AC80D3D07D}"
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Cake.GitVersioning", "Cake.GitVersioning\Cake.GitVersioning.csproj", "{1F267A97-DFE3-4166-83B1-9D236B7A09BD}"
EndProject
Copy link
Collaborator

@AArnott AArnott Aug 20, 2018

Choose a reason for hiding this comment

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

It looks like you resolved a merge conflict by blowing away the competing addition. I need nbgv to stick around. :) #Closed

Copy link
Collaborator

Choose a reason for hiding this comment

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

I fixed this.


In reply to: 211299223 [](ancestors = 211299223)

@@ -0,0 +1,84 @@

Microsoft Visual Studio Solution File, Format Version 12.00
Copy link
Collaborator

@AArnott AArnott Aug 20, 2018

Choose a reason for hiding this comment

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

This file doesn't belong here. #Closed

Copy link
Collaborator

Choose a reason for hiding this comment

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

I fixed this.


In reply to: 211302974 [](ancestors = 211302974)

@AArnott AArnott merged commit 0762cb5 into dotnet:master Aug 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants