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

Support GitVersioning in .vcxprojs #135

Merged
merged 4 commits into from Jun 18, 2017

Conversation

Projects
None yet
3 participants
@robmen
Contributor

robmen commented Jun 15, 2017

Generate a .rc file and include it into the build pipeline for native
code in a fashion similar to the GenerateAssemblyVersionInfo task.

Fixes #94

robmen added some commits Jun 14, 2017

Support GitVersioning in .vcxprojs
Generate a .rc file and include it into the build pipeline for native
code in a fashion similar to the `GenerateAssemblyVersionInfo` task.

Fixes #94
@robmen

This comment has been minimized.

Show comment
Hide comment
@robmen

robmen Jun 15, 2017

Contributor

Build failure is related to deprecated NPM module, not my code:

.\node_modules.bin\typings : typings WARN deprecated 4/27/2016: "registry:dt/camel-case#0.0.0+20160316155526" is deprecated (updated, replaced or removed)
At C:\projects\nerdbank-gitversioning\init.ps1:33 char:13

  •         .\node_modules\.bin\typings install
    
  •         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    
    • CategoryInfo : NotSpecified: (typings WARN de...ced or removed):String) [], RemoteException
    • FullyQualifiedErrorId : NativeCommandError

typings WARN deprecated 11/21/2016: "registry:dt/node#6.0.0+20160915134512" is deprecated (updated, replaced or removed

Contributor

robmen commented Jun 15, 2017

Build failure is related to deprecated NPM module, not my code:

.\node_modules.bin\typings : typings WARN deprecated 4/27/2016: "registry:dt/camel-case#0.0.0+20160316155526" is deprecated (updated, replaced or removed)
At C:\projects\nerdbank-gitversioning\init.ps1:33 char:13

  •         .\node_modules\.bin\typings install
    
  •         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    
    • CategoryInfo : NotSpecified: (typings WARN de...ced or removed):String) [], RemoteException
    • FullyQualifiedErrorId : NativeCommandError

typings WARN deprecated 11/21/2016: "registry:dt/node#6.0.0+20160915134512" is deprecated (updated, replaced or removed

@heaths

This comment has been minimized.

Show comment
Hide comment
@heaths

heaths Jun 16, 2017

Contributor

Recommend also pulling in the migration/initialization scripts from master...heaths:issue94diff-82f8d5aaf8a67007cec2e056c725cfea, or I can do so later and test.

Contributor

heaths commented Jun 16, 2017

Recommend also pulling in the migration/initialization scripts from master...heaths:issue94diff-82f8d5aaf8a67007cec2e056c725cfea, or I can do so later and test.

private const string FileHeaderComment = @"------------------------------------------------------------------------------
<auto-generated>
This code was generated by a tool.
Runtime Version:4.0.30319.42000

This comment has been minimized.

@heaths

heaths Jun 16, 2017

Contributor

May as well populate the runtime version with the actual version at runtime.

@heaths

heaths Jun 16, 2017

Contributor

May as well populate the runtime version with the actual version at runtime.

This comment has been minimized.

@robmen

robmen Jun 16, 2017

Contributor

Meh. This header matches AssemblyVersionInfo.cs (actually tried to share the code but decided it made things more complicated). I'll leave as is for consistency and both could be fixed in future.

@robmen

robmen Jun 16, 2017

Contributor

Meh. This header matches AssemblyVersionInfo.cs (actually tried to share the code but decided it made things more complicated). I'll leave as is for consistency and both could be fixed in future.

Show outdated Hide outdated src/Nerdbank.GitVersioning.Tasks/NativeVersionInfo.cs Outdated
@robmen

This comment has been minimized.

Show comment
Hide comment
@robmen

robmen Jun 16, 2017

Contributor

Also, I'll leave migration/initialization scripts to others. I don't actually use that functionality (actively avoid it) so I'm not a good candidate to improve them. Looks like you already have a huge head start on that work.

Contributor

robmen commented Jun 16, 2017

Also, I'll leave migration/initialization scripts to others. I don't actually use that functionality (actively avoid it) so I'm not a good candidate to improve them. Looks like you already have a huge head start on that work.

@heaths

This comment has been minimized.

Show comment
Hide comment
@heaths

heaths Jun 16, 2017

Contributor

@AArnott I can submit a PR for the scripts after you merge this if you'll hold out on a release (I'll watch for the merge notification and submit a PR based on @robmen's branch).

Contributor

heaths commented Jun 16, 2017

@AArnott I can submit a PR for the scripts after you merge this if you'll hold out on a release (I'll watch for the merge notification and submit a PR based on @robmen's branch).

@AArnott

This comment has been minimized.

Show comment
Hide comment
@AArnott

AArnott Jun 17, 2017

Owner

Build failure is related to deprecated NPM module

No, it's your code. Don't let AppVeyor's red highlighting distract you: it's just a warning.
The actual break is due to the failure of test BuildIntegrationTests.NativeVersionInfo_CreateNativeResourceDll later down the log. And that looks very related to your PR.

Owner

AArnott commented Jun 17, 2017

Build failure is related to deprecated NPM module

No, it's your code. Don't let AppVeyor's red highlighting distract you: it's just a warning.
The actual break is due to the failure of test BuildIntegrationTests.NativeVersionInfo_CreateNativeResourceDll later down the log. And that looks very related to your PR.

@@ -2,7 +2,11 @@
<Project DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003" ToolsVersion="4.0">
<PropertyGroup>
<MSBuildAllProjects>$(MSBuildAllProjects);$(MSBuildThisFileFullPath)</MSBuildAllProjects>
<VersionSourceFile>$(IntermediateOutputPath)\$(AssemblyName).Version$(DefaultLanguageSourceExtension)</VersionSourceFile>

This comment has been minimized.

@AArnott

AArnott Jun 18, 2017

Owner

I'm curious why this had to be moved. I can't think of anyone who depends on this property being defined in MSBuild's evaluation model, but it's a risk. So is it justified?

@AArnott

AArnott Jun 18, 2017

Owner

I'm curious why this had to be moved. I can't think of anyone who depends on this property being defined in MSBuild's evaluation model, but it's a risk. So is it justified?

This comment has been minimized.

@robmen

robmen Jun 18, 2017

Contributor

I moved it because the value is now different for managed vs. native code. The $(DefaultLanguageSourceExtension) for C++ code is .cpp but we're generating a .rc file so I localized and specialized the value in the targets that used them.

If you're worried about the breaking change I could keep this value up here and override it in the native code target. Or could just use a different variable local to the target or global here (although I don't know what I'd call it 😄 ).

Do you want me to do one of those?

@robmen

robmen Jun 18, 2017

Contributor

I moved it because the value is now different for managed vs. native code. The $(DefaultLanguageSourceExtension) for C++ code is .cpp but we're generating a .rc file so I localized and specialized the value in the targets that used them.

If you're worried about the breaking change I could keep this value up here and override it in the native code target. Or could just use a different variable local to the target or global here (although I don't know what I'd call it 😄 ).

Do you want me to do one of those?

This comment has been minimized.

@AArnott

AArnott Jun 18, 2017

Owner

At this point, no. Thanks for the background of this change. I'm ok to wait to hear that it broke someone before changing it. But now that I know the reasoning, we can change it with confidence should the need arise.

@AArnott

AArnott Jun 18, 2017

Owner

At this point, no. Thanks for the background of this change. I'm ok to wait to hear that it broke someone before changing it. But now that I know the reasoning, we can change it with confidence should the need arise.

Show outdated Hide outdated src/NerdBank.GitVersioning.Tests/BuildIntegrationTests.cs Outdated

@AArnott AArnott merged commit 7cb9894 into AArnott:master Jun 18, 2017

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@AArnott

This comment has been minimized.

Show comment
Hide comment
@AArnott

AArnott Jun 18, 2017

Owner

Thanks.

Owner

AArnott commented Jun 18, 2017

Thanks.

@robmen robmen deleted the robmen:94-nativebinaries branch Jun 18, 2017

@robmen

This comment has been minimized.

Show comment
Hide comment
@robmen

robmen Jun 18, 2017

Contributor

For my education, what was the unit test failure that broke AppVeyor? I ran all the tests locally and had them pass so I'm curious what I missed.

Contributor

robmen commented Jun 18, 2017

For my education, what was the unit test failure that broke AppVeyor? I ran all the tests locally and had them pass so I'm curious what I missed.

@AArnott

This comment has been minimized.

Show comment
Hide comment
@AArnott

AArnott Jun 18, 2017

Owner

The failure was here : https://ci.appveyor.com/project/AArnott/nerdbank-gitversioning/build/2.0.25-beta+ga6ab42abce#L164

But I've never seen it before or since. I didn't fix it either. It just passed on rerun. 😦

Owner

AArnott commented Jun 18, 2017

The failure was here : https://ci.appveyor.com/project/AArnott/nerdbank-gitversioning/build/2.0.25-beta+ga6ab42abce#L164

But I've never seen it before or since. I didn't fix it either. It just passed on rerun. 😦

@robmen

This comment has been minimized.

Show comment
Hide comment
@robmen

robmen Jun 18, 2017

Contributor

Hmm. That's a pattern I reused from the tests in the same file. The NerdBank.GitVersioning.targets are extracted at test time. I'm not able to reproduce it locally... timing issue?

Something to keep an eye out.

Contributor

robmen commented Jun 18, 2017

Hmm. That's a pattern I reused from the tests in the same file. The NerdBank.GitVersioning.targets are extracted at test time. I'm not able to reproduce it locally... timing issue?

Something to keep an eye out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment