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

Enable nerdbank #267

Merged
merged 13 commits into from
Nov 12, 2020
Merged

Enable nerdbank #267

merged 13 commits into from
Nov 12, 2020

Conversation

eddynaka
Copy link
Collaborator

@eddynaka eddynaka commented Nov 11, 2020

Fixes #204

Changes:

  • based on the version.json file, each commit will automatically generate a new version starting from 2.1.20. With that, we could ship automatically after every merge.
  • added a hack based on our style project which won't affect performance / way it works. Just the way it builds.

@eddynaka eddynaka marked this pull request as ready for review November 11, 2020 17:56
@eddynaka eddynaka self-assigned this Nov 11, 2020
@eddynaka eddynaka requested review from a user, GabeDeBacker and michaelcfanning November 11, 2020 17:56
@@ -167,8 +167,6 @@ if (-not $NoRestore) {
}

if (-not $NoBuild) {
Update-VersionConstantsFiles
Update-VsixManifests
Copy link

@ghost ghost Nov 11, 2020

Choose a reason for hiding this comment

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

VsixManifests [](start = 11, length = 13)

Yay! #Closed

}

dotnet tool install --global nbgv --version 3.3.37
$version = nbgv get-version -p src -v Version
Copy link

@ghost ghost Nov 11, 2020

Choose a reason for hiding this comment

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

-p [](start = 32, length = 2)

If there are long-form versions of these command line parameters -p and -v, use them to improve the readability of the script. #Closed

Copy link

Choose a reason for hiding this comment

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

There is another script New-VersionConstantsFile.ps1 that you should be able to remove.


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

@@ -167,8 +167,6 @@ if (-not $NoRestore) {
}

if (-not $NoBuild) {
Update-VersionConstantsFiles
Update-VsixManifests
Copy link

@ghost ghost Nov 11, 2020

Choose a reason for hiding this comment

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

VsixManifests [](start = 11, length = 13)

Remove the functions since they are no longer called. #Closed

Copy link

Choose a reason for hiding this comment

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

Reactivating. As of iteration 10, these functions are still defined. Everything from lines 68-90 can go away.


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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just removed! :)


In reply to: 521587394 [](ancestors = 521587394,521542752)

@@ -61,15 +61,10 @@ function New-NuGetPackageFromNuspecFile($configuration, $project, $version, $fra
}

function New-NuGetPackages($configuration, $projects, $frameworks) {
$versionPrefix, $versionSuffix = & $PSScriptRoot\Get-VersionConstants.ps1
Copy link

@ghost ghost Nov 11, 2020

Choose a reason for hiding this comment

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

Get-VersionConstants.ps1 [](start = 53, length = 24)

The file Get-VersionConstants.ps1 should no longer be needed and can be removed. (If it is still needed, then we missed something.) #Closed

src/build.props Outdated
@@ -37,10 +37,10 @@
<TreatWarningsAsErrors>true</TreatWarningsAsErrors>
</PropertyGroup>

<PropertyGroup Label="AssemblyAttributes">
<!--<PropertyGroup Label="AssemblyAttributes">
Copy link

@ghost ghost Nov 11, 2020

Choose a reason for hiding this comment

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

[assembly: AssemblyTitle("Sarifer test data producer extension for Visual Studio")]
[assembly: AssemblyDescription("Generates test data in SARIF format and provides it to the SARIF viewer extension, upon which it depends.")]

[assembly: NeutralResourcesLanguage("en")]

[assembly: AssemblyVersion(VersionConstants.FileVersion)]
[assembly: AssemblyFileVersion(VersionConstants.FileVersion)]
Copy link

@ghost ghost Nov 11, 2020

Choose a reason for hiding this comment

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

VersionConstants [](start = 31, length = 16)

Remove all the VersionConstants.cs files from the solution. #Closed

@ghost
Copy link

ghost commented Nov 11, 2020

Remove this whole comment. #Closed


Refers to: src/Sarif.Viewer.VisualStudio.Interop/Properties/AssemblyInfo.cs:32 in 263d958. [](commit_id = 263d958, deletion_comment = False)

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

🕐

@@ -5,6 +5,8 @@
<VSToolsPath Condition="'$(VSToolsPath)' == ''">$(MSBuildExtensionsPath32)\Microsoft\VisualStudio\v$(VisualStudioVersion)</VSToolsPath>
</PropertyGroup>
<Import Project="$(MSBuildExtensionsPath)\$(MSBuildToolsVersion)\Microsoft.Common.props" Condition="Exists('$(MSBuildExtensionsPath)\$(MSBuildToolsVersion)\Microsoft.Common.props')" />
<Import Project="$(MSBuildProjectExtensionsPath)$(_TargetAssemblyProjectName)$(MSBuildProjectExtension).nuget.g.props"
Copy link
Collaborator Author

@eddynaka eddynaka Nov 11, 2020

Choose a reason for hiding this comment

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

Hack: dotnet/Nerdbank.GitVersioning#404, since its a vsix with pages #Closed

Copy link

Choose a reason for hiding this comment

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

Add this comment to the project file itself.


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

@@ -330,4 +336,6 @@
</ItemGroup>
<Import Project="$(MSBuildToolsPath)\Microsoft.CSharp.targets" />
<Import Project="$(VSToolsPath)\VSSDK\Microsoft.VsSDK.targets" Condition="'$(VSToolsPath)' != ''" />
<Import Project="$(MSBuildProjectExtensionsPath)$(_TargetAssemblyProjectName)$(MSBuildProjectExtension).nuget.g.targets"
Copy link
Collaborator Author

@eddynaka eddynaka Nov 11, 2020

Choose a reason for hiding this comment

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

Hack: dotnet/Nerdbank.GitVersioning#404, since its a vsix with pages #Closed

Copy link

Choose a reason for hiding this comment

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

Add this comment to the project file itself.


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

@ghost
Copy link

ghost commented Nov 11, 2020

CodeFlow for GitHub does not download executable files. If you trust this file and would like to run it, please download it yourself through GitHub.

This file was accidentally committed. Add *.binlog to .gitignore. #Closed


Refers to: src/msbuild.binlog:1 in bcf4c8c. [](commit_id = bcf4c8c, deletion_comment = False)

public const string FileVersion = AssemblyVersion + ".0";
public const string Version = AssemblyVersion + Prerelease;
}
}
Copy link

@ghost ghost Nov 11, 2020

Choose a reason for hiding this comment

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

I don't see you deleting VersionConstants.cs from the Sarifer project. #Closed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

looking here, it's deleted. sarif.sarifer, sarif.viewer.visualstudio and sariv.viewer.visualstudio.interop


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

Copy link

Choose a reason for hiding this comment

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

I see what happened. I never committed that file in that project. I didn't notice since it's regenerated on every build. Arguably I should never have committed them in any project in the first place.


In reply to: 521587263 [](ancestors = 521587263,521585988)

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Member

@michaelcfanning michaelcfanning left a comment

Choose a reason for hiding this comment

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

:shipit:

@michaelcfanning michaelcfanning merged commit c319575 into master Nov 12, 2020
@michaelcfanning michaelcfanning deleted the users/ednakamu/testing-nerdbank branch November 12, 2020 21:14
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.

Build pipeline should index sources and publish symbols
2 participants