strong signing build tools binaries by shahabhijeet · Pull Request #5071 · Azure/azure-sdk-for-net

shahabhijeet · 2018-12-09T01:14:01Z

Strong signing build tools binaries

weshaggard · 2018-12-10T23:17:26Z

tools/BuildAssets/BuildTasks/Microsoft.Azure.Sdk.Build.Tasks/buildTools.props

@@ -0,0 +1,11 @@
+<Project ToolsVersion="15.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+	<PropertyGroup>


nit: Most msbuild xml files use 2 space indention without tabs.

@weshaggard I need to fix my indentation just for xml files in all the various editors I use. I have not been able to get set those defaults just for xml across VS, VSCode and notepad++
I will fix this very soon.

weshaggard · 2018-12-10T23:18:21Z

...s/Microsoft.Azure.Sdk.Build.Tasks/BootstrapTasks/Microsoft.Azure.Build.BootstrapTasks.csproj

  <PropertyGroup>
    <TargetFramework>net46</TargetFramework>
    <OutputPath>$(TaskBinaryOutput)</OutputPath>
+    <DelaySign>false</DelaySign>


You shouldn't need this because you set it in buildtools.props

@wedebols good catch, will remove this.

weshaggard · 2018-12-10T23:19:00Z

tools/BuildAssets/BuildTasks/Microsoft.Azure.Sdk.Build.Tasks/buildTools.props

+		<IsBuildToolsProject>true</IsBuildToolsProject>
+	</PropertyGroup>
+
+	<PropertyGroup Condition=" '$(IsBuildToolsProject)' == 'true' ">


Why condition this here? I don't see any way that IsBuildToolsProject isn't true from this spot.

@weshaggard there are often times when the build scripts evolve and properties get rearranged.
This is just to keep clear that this property group is applicable only under the certain condition.
So in case ever the property group above (line 3) ever get's into any other file, having the condition check right now will save me lot of headache, if I ever forget to add that condition back in.

OK that seems a little overkill to me but I don't have an issue with it. I would have just put both of them in the same property group to indicate that they are connected.

weshaggard · 2018-12-10T23:19:52Z

tools/BuildAssets/targets/common.Build.props

-		There are certain projects that needs internals visible for test purpose
+	
+	<!--  AND ('$(CodeSign)' == 'true')
+	<PropertyGroup Condition=" ('$(IsBuildToolsProject)' == 'false' OR '$(IsBuildToolsProject)' == '') ">


If you only ever set IsBuildToolsProject to true I would change this condition to '$(IsBuildToolsProject)' != 'true'

@weshaggard sounds good

Sorry I did not realized in the PR that the code was comment out.
There is no condition for IsBuildToolsProject == false anymore. The code that you see is commented out, so we are good.

weshaggard · 2018-12-10T23:21:02Z

tools/BuildAssets/targets/common.Build.props

 	</PropertyGroup>
+-->
 	<PropertyGroup>
-		<GenerateAssemblyInfo>false</GenerateAssemblyInfo>


Do you still need to set GenerateAssemblyInfo = false?

weshaggard · 2018-12-10T23:23:05Z

tools/BuildAssets/targets/common.Build.props

@@ -18,38 +18,29 @@
 		<BuildWideDebugTrace Condition=" '$(DebugTrace)' == '' ">false</BuildWideDebugTrace>


You probably need to condition DelaySign property above to make sure it doesn't override something setting it before like the buildtools.props.

@weshaggard you mean, check for if it's set to false and if yes, set it to true? Is that what you mean by conditioning the setter for the DelaySign?
Also buildtools.props is not included in any build workflow, unless you are building the buildtools projects. Do you still anticipate buildtools.prop overriding any defaults for normal workflow?
Also buildtools.props will never be merged to the master repo.

I was meaning something like <DelaySign Condition="'$(DelaySign)' == ''>true</DelaySign> . Just default it to true if it isn't already set to something else.

@weshaggard but dont' we want to just set it to true by default regardless of what the existing value is?
And it will get overridden while building build tools project to false.
Maybe I am missing to understand why it has to be conditioned in the global scope?

Assuming nothing imported before this is expected to set DelaySign then it is fine to always set it explicitly.

shahabhijeet · 2018-12-11T08:00:28Z

@weshaggard I will merge #5071 once all your PRs are merged. Please let me know if there is anything that I have not addressed to your feedback.
I am already getting merge conflicts, will resolve once all your PRs are in.

weshaggard · 2018-12-11T23:38:06Z

...ldTasks/Microsoft.Azure.Sdk.Build.Tasks/SdkBuildTasks/Microsoft.Azure.Sdk.Build.Tasks.csproj

  <PropertyGroup>
    <TargetFramework>net46</TargetFramework>
    <OutputPath>$(TaskBinaryOutput)</OutputPath>
+    <AssemblyOriginatorKeyFile>D:\myFork\LTSBuild\tools\BuildTools.snk</AssemblyOriginatorKeyFile>


Isn't this redundant with what is is in buildtools.props?

shahabhijeet · 2018-12-12T00:34:28Z

Closing this PR in lieu of #5076

strong signing build tools binaries

a3f406c

shahabhijeet requested a review from weshaggard December 10, 2018 22:23

weshaggard reviewed Dec 10, 2018

View reviewed changes

addressing PR feedback

3a432b3

shahabhijeet mentioned this pull request Dec 11, 2018

Add ignore scopes to props file for local builds #5074

Merged

weshaggard reviewed Dec 11, 2018

View reviewed changes

shahabhijeet closed this Dec 12, 2018

shahabhijeet mentioned this pull request Dec 12, 2018

strong signed build tools assemblies. Creating a new target for devop… #5076

Merged

shahabhijeet deleted the snSign branch March 27, 2019 23:52

leonardoadame mentioned this pull request Dec 20, 2023

[Snyk] Security upgrade cspell from 6.12.0 to 8.2.0 leonardoadame/azure-sdk-for-net#9

Merged

		@@ -0,0 +1,11 @@
		<Project ToolsVersion="15.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
		<PropertyGroup>

		@@ -18,38 +18,29 @@
		<BuildWideDebugTrace Condition=" '$(DebugTrace)' == '' ">false</BuildWideDebugTrace>

Conversation

shahabhijeet commented Dec 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shahabhijeet commented Dec 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shahabhijeet commented Dec 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants