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

shaopeng-gh · 2023-05-31T02:21:02Z

Found an issue, when Binskim update SDK Submodule to latest will break the build. I narrow down to it is caused by this PR: microsoft/sarif-sdk#2669
The added "Microsoft.Diagnostics.Tracing.TraceEvent" in SDK contains a Dia2Lib.dll which will be brought into BinSkim and conflict with our existing Dia2Lib.dll. Can't seem to remove it, and this version will not work with our code missing classes. Not sure how to resolve this.
The Dia2Lib.dll in TraceEvent has a different public key than the existing one we have, and is version 2.0.0.0.
The existing one we have don't have version number.

---- found a possible fix, to replace TraceEvent Dia2Lib.dll with our own.

The build will pass after the other issue is fixed, so need to go after this PR:
#896

shaopeng-gh · 2023-05-31T02:24:14Z

NuGet.Config

+<?xml version="1.0" encoding="utf-8"?>
+<configuration>
+  <config>
+    <add key="globalPackagesFolder" value="./src/packages" />


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

shaopeng-gh · 2023-05-31T02:25:21Z

ReleaseHistory.md

@@ -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).


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

shaopeng-gh · 2023-05-31T02:26:02Z

src/BinSkim.Driver/BinSkim.Driver.csproj

@@ -37,7 +37,7 @@

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


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

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

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

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?

Same comment for lines below.

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

shaopeng-gh · 2023-05-31T02:27:23Z

src/BinaryParsers/BinaryParsers.csproj

+  <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\"


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

@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?

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?

suvamM · 2023-06-02T15:20:45Z

src/BinSkim.Driver/BinSkim.Driver.csproj

@@ -37,7 +37,7 @@

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


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

suvamM · 2023-06-02T15:22:16Z

src/BinaryParsers/BinaryParsers.csproj

+  <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\"


@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?

michaelcfanning

shaopeng-gh · 2023-06-14T17:39:17Z

Note: We will explore upgrading of the lib before revisit this. #Closed

shaopeng-gh · 2023-12-05T09:30:57Z

ReleaseHistory.md

@@ -16,6 +16,7 @@
 - NEW => new feature 

 ## UNRELEASED
+* DEP: Update Sarif.Sdk submodule from [bc8cb57 to fd6e615](https://github.com/microsoft/sarif-sdk/compare/bc8cb57...fd6e615). Full [SARIF SDK Release History](https://github.com/microsoft/sarif-sdk/blob/fd6e615/ReleaseHistory.md).


DEP

Reopen this PR.
The was for: if update SARIF sub module, the Dia will not work. At that moment I created this PR but we did not have any urgent fix we need from SARIF SDK.
Now there is fix that is needed, I reopen this PR and update to point to latest SDK.
There is no new test added, but without this fix many tests will fail.
Please see PR description for some info.

shaopeng-gh · 2023-12-05T09:32:59Z

our code uses the old classes, if re-create the dll, our current code that uses the dll will need a major re-write.

In reply to: 1591721869

stacywray · 2023-12-05T16:26:52Z

ReleaseHistory.md

@@ -16,6 +16,7 @@
 - NEW => new feature 

 ## UNRELEASED
+* DEP: Update Sarif.Sdk submodule from [bc8cb57 to fd6e615](https://github.com/microsoft/sarif-sdk/compare/bc8cb57...fd6e615). Full [SARIF SDK Release History](https://github.com/microsoft/sarif-sdk/blob/fd6e615/ReleaseHistory.md).


Add backticks around Sarif.SDK submodule.

Rewrite second sentence to "Reference full SARIF SDK Release History." #Resolved

Fixed. I left the full word out of the link, let me know if you think we should include it.

stacywray

Left an inline suggestion for ReleaseHistory.md

scottoneil-ms

Thanks!

shaopeng-gh · 2023-12-05T19:30:17Z

updated, thanks.

In reply to: 1765562052

stacywray

No need to include "full" in the link. Looks good.

michaelcfanning · 2023-12-05T20:47:58Z

ReleaseHistory.md

@@ -16,6 +16,7 @@
 - NEW => new feature 

 ## UNRELEASED
+* DEP: Update `Sarif.Sdk` submodule from [bc8cb57 to fd6e615](https://github.com/microsoft/sarif-sdk/compare/bc8cb57...fd6e615). Reference full [SARIF SDK Release History](https://github.com/microsoft/sarif-sdk/blob/fd6e615/ReleaseHistory.md).



This line is inconsistent with other entries in this file: they all need to be consistent. I think Stacy recommended removing the word Full, as below:

if that is correct, please update the entire release history to bring all of these DEP entries into conformance. Also someone needs to make sure our release notes maintenance documentation has the correct standard listed there.

DEP: Update Sarif.Sdk submodule from bc8cb57 to fd6e615. SARIF SDK Release History.

Also, why exactly did you need to make this dependency update? It is a good practice to cite the specific incoming change that's critical to binskim.

I think Stacy recommended removing the word Full --- The original recommendation from Stacy was to add Reference not removing full, I checked with her we both agree removing full looks good. i have updated.

why exactly did you need to make this dependency update? ---- Only needed is the change that we disable the warning for using Newton old versions 9. So that build will not just stop as error. This is only to fix our build process not related to the user.

stacywray

LGTM.

Fix update Submodule error

329ed88

shaopeng-gh requested review from suvamM and schlaman-ms May 31, 2023 02:21

shaopeng-gh requested review from michaelcfanning and marmegh as code owners May 31, 2023 02:21

shaopeng-gh commented May 31, 2023

View reviewed changes

suvamM requested changes Jun 2, 2023

View reviewed changes

shaopeng-gh added 2 commits June 2, 2023 08:49

Merge branch 'main' into users/shaopeng-gh/fixupdatesubmodule_ok

2669700

Merge branch 'main' into users/shaopeng-gh/fixupdatesubmodule_ok

539a4e5

michaelcfanning approved these changes Jun 14, 2023

View reviewed changes

shaopeng-gh marked this pull request as draft June 14, 2023 17:39

shaopeng-gh and others added 3 commits July 25, 2023 22:24

merge from main

1252971

Merge branch 'main' into users/shaopeng-gh/fixupdatesubmodule_ok

428c916

Update to Latest SDK

74998fb

shaopeng-gh requested a review from suvamM December 5, 2023 09:15

shaopeng-gh marked this pull request as ready for review December 5, 2023 09:19

shaopeng-gh commented Dec 5, 2023

View reviewed changes

stacywray reviewed Dec 5, 2023

View reviewed changes

stacywray suggested changes Dec 5, 2023

View reviewed changes

shaopeng-gh added 2 commits December 5, 2023 11:00

Update Release History

29a14fe

Fix casing for Dia2Lib

4e717ff

scottoneil-ms approved these changes Dec 5, 2023

View reviewed changes

stacywray approved these changes Dec 5, 2023

View reviewed changes

michaelcfanning reviewed Dec 5, 2023

View reviewed changes

Format Release History Update Sarif.Sdk submodule

a1f113e

stacywray approved these changes Dec 5, 2023

View reviewed changes

shaopeng-gh requested a review from michaelcfanning December 5, 2023 22:34

michaelcfanning merged commit 0cf0bc8 into main Dec 8, 2023
5 checks passed

michaelcfanning deleted the users/shaopeng-gh/fixupdatesubmodule_ok branch December 8, 2023 20:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

shaopeng-gh commented May 31, 2023

shaopeng-gh May 31, 2023 •

edited

Loading

shaopeng-gh May 31, 2023 •

edited

Loading

shaopeng-gh May 31, 2023 •

edited

Loading

suvamM Jun 2, 2023

shaopeng-gh Jun 2, 2023

scottoneil-ms Dec 5, 2023

scottoneil-ms Dec 5, 2023

shaopeng-gh Dec 5, 2023

shaopeng-gh May 31, 2023 •

edited

Loading

suvamM Jun 2, 2023

shaopeng-gh Jun 5, 2023

suvamM Jun 2, 2023

suvamM Jun 2, 2023

michaelcfanning left a comment

shaopeng-gh commented Jun 14, 2023 •

edited

Loading

shaopeng-gh Dec 5, 2023

shaopeng-gh commented Dec 5, 2023

stacywray Dec 5, 2023 •

edited by shaopeng-gh

Loading

shaopeng-gh Dec 5, 2023

stacywray left a comment

scottoneil-ms left a comment

shaopeng-gh commented Dec 5, 2023

stacywray left a comment

michaelcfanning Dec 5, 2023

michaelcfanning Dec 5, 2023

shaopeng-gh Dec 5, 2023

stacywray left a comment

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

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

Conversation

shaopeng-gh commented May 31, 2023

shaopeng-gh May 31, 2023 • edited Loading

Choose a reason for hiding this comment

shaopeng-gh May 31, 2023 • edited Loading

Choose a reason for hiding this comment

shaopeng-gh May 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaopeng-gh May 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning left a comment

Choose a reason for hiding this comment

shaopeng-gh commented Jun 14, 2023 • edited Loading

Choose a reason for hiding this comment

shaopeng-gh commented Dec 5, 2023

stacywray Dec 5, 2023 • edited by shaopeng-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stacywray left a comment

Choose a reason for hiding this comment

scottoneil-ms left a comment

Choose a reason for hiding this comment

shaopeng-gh commented Dec 5, 2023

stacywray left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stacywray left a comment

Choose a reason for hiding this comment

shaopeng-gh May 31, 2023 •

edited

Loading

shaopeng-gh May 31, 2023 •

edited

Loading

shaopeng-gh May 31, 2023 •

edited

Loading

shaopeng-gh May 31, 2023 •

edited

Loading

shaopeng-gh commented Jun 14, 2023 •

edited

Loading

stacywray Dec 5, 2023 •

edited by shaopeng-gh

Loading