-
Notifications
You must be signed in to change notification settings - Fork 140
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
Repair PrepareRelease tool, publish in line. #624
Conversation
deploy/Datadog.Trace.ClrProfiler.WindowsInstaller/Files.Managed.Net461.wxs
Outdated
Show resolved
Hide resolved
@@ -1,4 +1,4 @@ | |||
<Project Sdk="Microsoft.NET.Sdk"> | |||
<Project Sdk="Microsoft.NET.Sdk"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We keep switching back and forth between having and not having the UTF-8 BOM. Why does the tool even modify this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Versions replacement logic does this. You suggested we add this option to the versioning tool when it was implemented: new UTF8Encoding(encoderShouldEmitUTF8Identifier: false)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yeah. I remember. I wonder what keeps adding it, then. Visual Studio? VSCode? Maybe it would be easier to always add the BOM instead of removing it? Level of care is low, as long as we can be consistent and avoid the extra whitespace noise in commit diffs.
src/Datadog.Trace.ClrProfiler.Managed/Datadog.Trace.ClrProfiler.Managed.csproj
Outdated
Show resolved
Hide resolved
86543e7
to
275e42c
Compare
test/Datadog.Trace.TestHelpers/Datadog.Trace.TestHelpers.csproj
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,32 @@ | |||
|
|||
REM SET SOLUTION_DIR=C:\Github\dd-trace-dotnet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For stand alone tests
tools/PrepareRelease/Program.cs
Outdated
Environment.SetEnvironmentVariable("TRACER_HOME_OUTPUT_DIR", tracerHomeOutput); | ||
|
||
var publishBatch = Path.Combine(solutionDir, "tools", "PrepareRelease", "publish-all.bat"); | ||
ExecuteCommand(publishBatch); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make the publish part an optional "Job" like versions and integrations? I would also opt to make it off by default because we don't need to run it when we do a new release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The plan was to have it be a pre-req for the integrations and MSI steps below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll nest it within the MSI section for now, as that's the only place it's really used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still WIP, but I'm ok merging this for now.
* Repair PrepareRelease tool, publish for dependency list
dotnet publish within the prepare release tool for accurate msi management and drop it to netframework target
@DataDog/apm-dotnet