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

Fix build error after update the SDK submodule in BinSkim #915

Merged
merged 9 commits into from
Dec 8, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions NuGet.Config
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<?xml version="1.0" encoding="utf-8"?>
<configuration>
<config>
<add key="globalPackagesFolder" value="./src/packages" />
Copy link
Collaborator Author

@shaopeng-gh shaopeng-gh May 31, 2023

Choose a reason for hiding this comment

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

we have similar settings in SDK with 2 settings, based on building and testing only this one is needed. #Closed

</config>
</configuration>
1 change: 1 addition & 0 deletions ReleaseHistory.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

## UNRELEASED
* DEP: Update `Microsoft.CodeAnalysis.NetAnalyzers` package from 7.0.0 to 7.0.1 to resolve build warnings. [#903](https://github.com/microsoft/binskim/pull/903)
* DEP: Update Sarif.Sdk submodule from [bc8cb57 to dd8b7b8](https://github.com/microsoft/sarif-sdk/compare/bc8cb57...dd8b7b8). Full [SARIF SDK Release History](https://github.com/microsoft/sarif-sdk/blob/dd8b7b8/ReleaseHistory.md).
Copy link
Collaborator Author

@shaopeng-gh shaopeng-gh May 31, 2023

Choose a reason for hiding this comment

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

this is the current latest SDK (when this PR created) #Closed


## **v4.1.0**
* NEW: `CompilerInformation` telemetry now emits the last modified date of the scan target. [#873](https://github.com/microsoft/binskim/pull/873)
Expand Down
File renamed without changes.
2 changes: 1 addition & 1 deletion src/BinSkim.Driver/BinSkim.Driver.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@

<ItemGroup>
<Reference Include="dia2lib">
<HintPath>..\..\refs\dia2lib.dll</HintPath>
<HintPath>..\..\refs\Dia2Lib.dll</HintPath>
Copy link
Collaborator Author

@shaopeng-gh shaopeng-gh May 31, 2023

Choose a reason for hiding this comment

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

The name changed is needed because in Linux it is case sensitive. #Resolved

Copy link
Collaborator

Choose a reason for hiding this comment

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

@shaopeng-gh What exactly is the issue when the SARIF SDK Dia2Lib.dll is used?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

misssing classes: IDiaEnumSegments, IDiaSectionContrib etc.
conflict classes: IDiaDataSource2, IDiaDataSource3
image

Choose a reason for hiding this comment

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

If there isn't a reason not to change the Reference itself (as opposed to the hint), can you please change it to agree with the hint?

Choose a reason for hiding this comment

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

Same comment for lines below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated the
<Reference Include="dia2lib">
to
<Reference Include="Dia2Lib">.
I have searched the whole repo there is 0 lower case of dia2lib.

<EmbedInteropTypes>False</EmbedInteropTypes>
</Reference>
</ItemGroup>
Expand Down
2 changes: 1 addition & 1 deletion src/BinSkim.Rules/BinSkim.Rules.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

<ItemGroup>
<Reference Include="dia2lib">
<HintPath>..\..\refs\dia2lib.dll</HintPath>
<HintPath>..\..\refs\Dia2Lib.dll</HintPath>
<EmbedInteropTypes>False</EmbedInteropTypes>
</Reference>
</ItemGroup>
Expand Down
14 changes: 12 additions & 2 deletions src/BinaryParsers/BinaryParsers.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@

<ItemGroup>
<Reference Include="dia2lib">
<HintPath>..\..\refs\dia2lib.dll</HintPath>
<HintPath>..\..\refs\Dia2Lib.dll</HintPath>
<EmbedInteropTypes>False</EmbedInteropTypes>
</Reference>
</ItemGroup>
Expand All @@ -43,4 +43,14 @@
</EmbeddedResource>
</ItemGroup>

</Project>
<PropertyGroup>
<PreBuildEvent Condition="$(OS) == 'Windows_NT'">
echo Copying file from "$(MSBuildProjectDirectory)\..\..\refs\Dia2Lib.dll" to "$(MSBuildProjectDirectory)\..\..\src\packages\microsoft.diagnostics.tracing.traceevent\3.1.3\lib\netstandard2.0\"
copy "$(MSBuildProjectDirectory)\..\..\refs\Dia2Lib.dll" "$(MSBuildProjectDirectory)\..\..\src\packages\microsoft.diagnostics.tracing.traceevent\3.1.3\lib\netstandard2.0\"
Copy link
Collaborator Author

@shaopeng-gh shaopeng-gh May 31, 2023

Choose a reason for hiding this comment

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

This is to replace with our version of Dia2Lib.dll that works with our code. #Closed

Copy link
Collaborator

Choose a reason for hiding this comment

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

@shaopeng-gh I am not sure this is the best fix here. So you are essentially hard coding the Dia2Lib.dll to use? From the PR description, it seems like this dll is used for the new events model. Would hard-coding our version of the dll affect the generation of events?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is good point if it will break the generation of events.
I am not familiar about it,
I see the code we are doing check like:
if (!string.IsNullOrEmpty(globalContext.EventsFilePath))
to enable generation of events, and the setting of Context.EventsFilePath is:
context.EventsFilePath = Environment.GetEnvironmentVariable("SPMI_ETW") ?? options.EventsFilePath ?? context.EventsFilePath;
Is this only enabled in SPMI?

</PreBuildEvent>
<PreBuildEvent Condition="$(OS) != 'Windows_NT'">
echo Copying file from "$(MSBuildProjectDirectory)\..\..\refs\Dia2Lib.dll" to "$(MSBuildProjectDirectory)\..\..\src\packages\microsoft.diagnostics.tracing.traceevent\3.1.3\lib\netstandard2.0\"
cp -f "$(MSBuildProjectDirectory)\..\..\refs\Dia2Lib.dll" "$(MSBuildProjectDirectory)\..\..\src\packages\microsoft.diagnostics.tracing.traceevent\3.1.3\lib\netstandard2.0\"
</PreBuildEvent>
</PropertyGroup>
</Project>
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@

<ItemGroup>
<Reference Include="Dia2Lib">
<HintPath>..\..\refs\dia2lib.dll</HintPath>
<HintPath>..\..\refs\Dia2Lib.dll</HintPath>
</Reference>
</ItemGroup>

Expand Down
Empty file added src/packages/.gitkeep
Empty file.
2 changes: 1 addition & 1 deletion src/sarif-sdk
Submodule sarif-sdk updated 40 files
+6 −0 ReleaseHistory.md
+1 −0 lgtm.yml
+555 −0 src/Sarif.Driver/DumpEventsCommand.cs
+26 −0 src/Sarif.Driver/DumpEventsOptions.cs
+41 −0 src/Sarif.Driver/ExportEventsCommand.cs
+19 −0 src/Sarif.Driver/ExportEventsOptions.cs
+1 −24 src/Sarif.Driver/OrderedFileSpecifier.cs
+1 −0 src/Sarif.Driver/Sarif.Driver.csproj
+17 −25 src/Sarif.Driver/Sdk/AnalyzeOptionsBase.cs
+5 −5 src/Sarif.Driver/Sdk/CommonOptionsBase.cs
+21 −0 src/Sarif.Driver/Sdk/ConsoleEventsListener.cs
+30 −0 src/Sarif.Driver/Sdk/DriverEventId.cs
+37 −0 src/Sarif.Driver/Sdk/DriverEventNames.cs
+221 −0 src/Sarif.Driver/Sdk/DriverEventSource.cs
+1 −1 src/Sarif.Driver/Sdk/ExportConfigurationOptions.cs
+1 −1 src/Sarif.Driver/Sdk/ExportRulesMetadataOptions.cs
+144 −32 src/Sarif.Driver/Sdk/MultithreadedAnalyzeCommandBase.cs
+0 −6 src/Sarif/AggregatingArtifactsProvider.cs
+80 −10 src/Sarif/AnalyzeContextBase.cs
+3 −5 src/Sarif/ArtifactProvider.cs
+2 −5 src/Sarif/Core/SarifLog.cs
+36 −18 src/Sarif/EnumeratedArtifact.cs
+3 −1 src/Sarif/IAnalysisContext.cs
+0 −2 src/Sarif/IArtifactProvider.cs
+1 −1 src/Sarif/IEnumeratedArtifact.cs
+31 −1 src/Sarif/Notes.cs
+29 −28 src/Sarif/RuntimeConditions.cs
+6 −1 src/Sarif/Sarif.csproj
+30 −0 src/Sarif/SarifExtensionMethods.cs
+31 −4 src/Sarif/SdkResources.Designer.cs
+13 −4 src/Sarif/SdkResources.resx
+24 −0 src/Sarif/Warnings.cs
+25 −25 src/Sarif/Writers/MemoryStreamSarifLogger.cs
+1 −3 src/Test.FunctionalTests.Sarif/Multitool/ValidateCommandTests.cs
+36 −1 src/Test.UnitTests.Sarif.Driver/Sdk/AnalyzeCommandBaseTests.cs
+14 −1 src/Test.UnitTests.Sarif.Driver/TestRule.cs
+7 −4 src/Test.UnitTests.Sarif/Core/SarifLogTests.cs
+71 −0 src/Test.UnitTests.Sarif/EnumeratedArtifactTests.cs
+2 −2 src/Test.UnitTests.Sarif/NotesTests.cs
+3 −1 src/build.props