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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
329ed88
Fix update Submodule error
shaopeng-gh May 31, 2023
2669700
Merge branch 'main' into users/shaopeng-gh/fixupdatesubmodule_ok
shaopeng-gh Jun 2, 2023
539a4e5
Merge branch 'main' into users/shaopeng-gh/fixupdatesubmodule_ok
shaopeng-gh Jun 6, 2023
1252971
merge from main
shaopeng-gh Jul 26, 2023
428c916
Merge branch 'main' into users/shaopeng-gh/fixupdatesubmodule_ok
shaopeng-gh Dec 5, 2023
74998fb
Update to Latest SDK
shaopeng-gh Dec 5, 2023
29a14fe
Update Release History
shaopeng-gh Dec 5, 2023
4e717ff
Fix casing for Dia2Lib
shaopeng-gh Dec 5, 2023
a1f113e
Format Release History Update `Sarif.Sdk` submodule
shaopeng-gh Dec 5, 2023
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
View file
Open in desktop
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 •
edited
Loading

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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • 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.

Copy link

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

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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


Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

## **v4.2.1**
* FPS: `BA2004.EnableSecureSourceCodeHashing` now will no longer generate false positives on precompiled headers, they are always without hash. [#965](https://github.com/microsoft/binskim/pull/965)
Expand Down
0 refs/dia2lib.dll → refs/Dia2Lib.dll
View file
Open in desktop
File renamed without changes.
2 changes: 1 addition & 1 deletion src/BinSkim.Driver/BinSkim.Driver.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,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 •
edited
Loading

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

Copy link

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?

Copy link

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
View file
Open in desktop
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
View file
Open in desktop
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 •
edited
Loading

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>
2 changes: 1 addition & 1 deletion src/Test.UnitTests.BinaryParsers/Test.UnitTests.BinaryParsers.csproj
View file
Open in desktop
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
View file
Open in desktop
Empty file.
2 changes: 1 addition & 1 deletion src/sarif-sdk
Open in desktop
Submodule sarif-sdk updated 180 files
Loading