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

make pre build events post build events work using targets #2367

Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view

Large diffs are not rendered by default.

Large diffs are not rendered by default.

@@ -0,0 +1,16 @@
using System.Xml;
using Microsoft.Build.Construction;

namespace Microsoft.VisualStudio.ProjectSystem.VS.Utilities
{
public static class StringExtensions
{
public static ProjectRootElement AsProjectRootElement(this string @string)
{
var stringReader = new System.IO.StringReader(@string);
var xmlReader = new XmlTextReader(stringReader);
var root = ProjectRootElement.Create(xmlReader);
return root;
}
}
}
@@ -0,0 +1,136 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.


using System;
using System.Linq;
using Microsoft.Build.Construction;

namespace Microsoft.VisualStudio.ProjectSystem.VS.Properties.InterceptedProjectProperties
{
internal abstract partial class AbstractBuildEventValueProvider
{
public abstract class Helper
{
private const string _execTaskName = "Exec";
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: just ExecTask maybe?

private const string _commandString = "Command";
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: String in the variable name seems redundant


protected Helper(string buildEventString,
Copy link
Contributor

Choose a reason for hiding this comment

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

Helper is a very generic name. This object contains state so I would suggest a better name, like AbstractBuildEventHelper or AbstractBuildEventWorker

string targetNameString,
Func<ProjectTargetElement, string> getTargetString,
Action<ProjectTargetElement> setTargetDependencies)
{
BuildEventString = buildEventString;
TargetNameString = targetNameString;
GetTargetString = getTargetString;
SetTargetDependencies = setTargetDependencies;
}

private Func<ProjectTargetElement, string> GetTargetString { get; }
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I am not too bothered about the redundant naming here. But just wanted to bring it to your notice .

private Action<ProjectTargetElement> SetTargetDependencies { get; }
private string BuildEventString { get; }
private string TargetNameString { get; }

public string GetProperty(ProjectRootElement projectXml)
{
var result = FindExecTaskInTargets(projectXml);
Copy link
Contributor

Choose a reason for hiding this comment

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

why return a tuple here? Instead, cant we do a null check to determine success?


if (result.success == false)
{
return null;
}

if (result.execTask.Parameters.TryGetValue(_commandString, out var commandText))
{
return commandText;
}

return null; //unreachable
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to add a Assert fail so that we get notified when this happens rather than silently passing

}

public void SetProperty(string unevaluatedPropertyValue, ProjectRootElement projectXml)
{
if (string.IsNullOrWhiteSpace(unevaluatedPropertyValue) &&
!unevaluatedPropertyValue.Contains("\n"))
{
var result = FindTargetToRemove(projectXml);
if (result.success)
{
projectXml.RemoveChild(result.target);
}
}
else
{
SetParameter(projectXml, unevaluatedPropertyValue);
}
}

private (bool success, ProjectTaskElement execTask) FindExecTaskInTargets(ProjectRootElement projectXml)
{
var execTask = projectXml.Targets
.Where(target => GetTargetString(target) == BuildEventString)
Copy link
Contributor

Choose a reason for hiding this comment

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

case-insensitive comparison required

.SelectMany(target => target.Tasks)
.Where(task => task.Name == _execTaskName)
Copy link
Contributor

Choose a reason for hiding this comment

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

Msbuild syntax itself is case-insensitive. We should account for it.

.FirstOrDefault();
return (success: execTask != null, execTask: execTask);
}

private (bool success, ProjectTargetElement target) FindTargetToRemove(ProjectRootElement projectXml)
{
var targetArray = projectXml.Targets
.Where(target =>
GetTargetString(target) == BuildEventString &&
Copy link
Contributor

Choose a reason for hiding this comment

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

case insensitive comparison here as well

target.Children.Count == 1 &&
target.Tasks.Count == 1 &&
target.Tasks.First().Name == _execTaskName);
return (success: targetArray.Count() == 1, target: targetArray.SingleOrDefault());
Copy link
Contributor

Choose a reason for hiding this comment

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

It does not make sense to have the same target twice but nevertheless we do allow it. If there is more than one, then we will crash since we are using Single

}

private void SetParameter(ProjectRootElement projectXml, string unevaluatedPropertyValue)
{
var result = FindExecTaskInTargets(projectXml);

if (result.success == true)
{
SetExecParameter(result.execTask, unevaluatedPropertyValue);
}
else
{
var targetName = GetTargetName(projectXml);
var target = projectXml.AddTarget(targetName);
SetTargetDependencies(target);
var execTask = target.AddTask(_execTaskName);
SetExecParameter(execTask, unevaluatedPropertyValue);
}
}

private void SetExecParameter(ProjectTaskElement execTask, string unevaluatedPropertyValue)
=> execTask.SetParameter(_commandString, unevaluatedPropertyValue);

private string GetTargetName(ProjectRootElement projectXml)
{
var targetNames = projectXml.Targets.Select(t => t.Name).ToArray();
Copy link
Contributor

Choose a reason for hiding this comment

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

We are going to allocate a string for all targets. .Contains on the array wont be efficient as well. Instead if we use Hashset then the search is a constant operation

var targetName = TargetNameString;
if (targetNames.Contains(targetName))
{
targetName = FindNonCollidingName(targetName, targetNames);
}

return targetName;

}

private string FindNonCollidingName(string buildEventString, string[] targetNames)
{
var initialValue = 1;
var newName = buildEventString + initialValue.ToString();
while (targetNames.Contains(newName))
{
initialValue++;
newName = buildEventString + initialValue.ToString();
}

return newName;
}
}
}
}
@@ -0,0 +1,51 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System.Collections.Generic;
using System.Threading.Tasks;
using Microsoft.VisualStudio.ProjectSystem.Properties;

namespace Microsoft.VisualStudio.ProjectSystem.VS.Properties.InterceptedProjectProperties
{
internal abstract partial class AbstractBuildEventValueProvider : InterceptingPropertyValueProviderBase
{
protected readonly IProjectLockService _projectLockService;
protected readonly UnconfiguredProject _unconfiguredProject;
private readonly Helper _helper;

protected AbstractBuildEventValueProvider(
IProjectLockService projectLockService,
UnconfiguredProject unconfiguredProject,
Helper helper)
{
_projectLockService = projectLockService;
_unconfiguredProject = unconfiguredProject;
_helper = helper;
}

public override async Task<string> OnGetEvaluatedPropertyValueAsync(
string evaluatedPropertyValue,
IProjectProperties defaultProperties)
{
using (var access = await _projectLockService.ReadLockAsync())
{
var projectXml = await access.GetProjectXmlAsync(_unconfiguredProject.FullPath).ConfigureAwait(true);
return _helper.GetProperty(projectXml);
}
}

public override async Task<string> OnSetPropertyValueAsync(
string unevaluatedPropertyValue,
IProjectProperties defaultProperties,
IReadOnlyDictionary<string, string> dimensionalConditions = null)
{
using (var access = await _projectLockService.WriteLockAsync())
{
var projectXml = await access.GetProjectXmlAsync(_unconfiguredProject.FullPath).ConfigureAwait(true);

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove Linebreak

_helper.SetProperty(unevaluatedPropertyValue, projectXml);
}

return null;
}
}
}
@@ -0,0 +1,33 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System.ComponentModel.Composition;
using Microsoft.VisualStudio.ProjectSystem.Properties;

namespace Microsoft.VisualStudio.ProjectSystem.VS.Properties.InterceptedProjectProperties
{
[ExportInterceptingPropertyValueProvider(_postBuildEventString, ExportInterceptingPropertyValueProviderFile.ProjectFile)]
internal class PostBuildEventValueProvider : AbstractBuildEventValueProvider
{
private const string _postBuildEventString = "PostBuildEvent";
private const string _targetNameString = "PostBuild";

[ImportingConstructor]
public PostBuildEventValueProvider(
IProjectLockService projectLockService,
UnconfiguredProject unconfiguredProject)
: base(projectLockService,
unconfiguredProject,
new PostBuildEventHelper())
{}

internal class PostBuildEventHelper : Helper
{
internal PostBuildEventHelper()
: base(_postBuildEventString,
_targetNameString,
target => target.AfterTargets,
target => { target.AfterTargets = _postBuildEventString; })
{ }
}
}
}
@@ -0,0 +1,33 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System.ComponentModel.Composition;
using Microsoft.VisualStudio.ProjectSystem.Properties;

namespace Microsoft.VisualStudio.ProjectSystem.VS.Properties.InterceptedProjectProperties
{
[ExportInterceptingPropertyValueProvider(_preBuildEventString, ExportInterceptingPropertyValueProviderFile.ProjectFile)]
internal class PreBuildEventValueProvider : AbstractBuildEventValueProvider
{
private const string _preBuildEventString = "PreBuildEvent";
Copy link
Contributor

Choose a reason for hiding this comment

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

PreBuildEventString. Similarly for all the other private const strings.

private const string _targetNameString = "PreBuild";

[ImportingConstructor]
public PreBuildEventValueProvider(
IProjectLockService projectLockService,
UnconfiguredProject unconfiguredProject)
: base(projectLockService,
unconfiguredProject,
new PreBuildEventHelper())
{ }

internal class PreBuildEventHelper : Helper
{
internal PreBuildEventHelper()
: base(_preBuildEventString,
_targetNameString,
target => target.BeforeTargets,
target => { target.BeforeTargets = _preBuildEventString; })
{ }
}
}
}
Expand Up @@ -69,8 +69,16 @@
<DataSource Persistence="ProjectFile" PersistedName="Win32Resource" HasConfigurationCondition="False" SourceOfDefaultValue="AfterContext" />
</StringProperty.DataSource>
</StringProperty>
<StringProperty Name="PreBuildEvent" DisplayName="Pre Build Event" Visible="False"/>
<StringProperty Name="PostBuildEvent" DisplayName="Post Build Event" Visible="False"/>
<StringProperty Name="PreBuildEvent" DisplayName="Pre Build Event" Visible="False">
<StringProperty.DataSource>
<DataSource Persistence="ProjectFileWithInterception" PersistedName="PreBuildEvent" HasConfigurationCondition="False" SourceOfDefaultValue="AfterContext"/>
</StringProperty.DataSource>
</StringProperty>
<StringProperty Name="PostBuildEvent" DisplayName="Post Build Event" Visible="False">
<StringProperty.DataSource>
<DataSource Persistence="ProjectFileWithInterception" PersistedName="PostBuildEvent" HasConfigurationCondition="False" SourceOfDefaultValue="AfterContext"/>
</StringProperty.DataSource>
</StringProperty>
<EnumProperty Name="RunPostBuildEvent" DisplayName="Run Post Build Event" Visible="False">
<EnumValue Name="Always" DisplayName="Always" />
<EnumValue Name="OnBuildSuccess" DisplayName="On successful build" IsDefault="True" />
Expand Down