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

Conversation

@jmarolf
Copy link
Member

jmarolf commented Jun 2, 2017

Customer scenario

User wants to have their pre-build and post build events use MSBuild variables but they are evaluated as empty strings without this fix

Bugs this fixes:

#1569
#1879

Workarounds, if any
User can manually modify their project file.

Risk

this only affects the project file if the user writes pre or post build events from the properties pages. risk is low.

Performance impact

Code is only called when the user changes the value in the property page. Risk is low.

Is this a regression from a previous update?

It is a regression from the native project system

Root cause analysis:

Targets are now implicitly imported to MSBuild variables do not exist. The fix is to add a target to the project file so that when the user add pre and post build events, by the time they are evaluated the MeBuild variables exist.

How was the bug found?

customer reported - This is one of the top VS Feedback reports with 20+ votes.

@jmarolf jmarolf requested review from srivatsn and basoundr Jun 2, 2017
@jmarolf
Copy link
Member Author

jmarolf commented Jun 2, 2017

@dotnet/project-system please review. Let me know if you have a better idea for project file format.

@jmarolf jmarolf changed the title Bugfix/make pre build events post build events work using targets make pre build events post build events work using targets Jun 2, 2017
@srivatsn
Copy link
Contributor

srivatsn commented Jun 2, 2017

Tagging @rainersigwald @AndyGerlicher @cdmihai for ideas as well. The problem here is that prebuildevent and postbuildevent used to be specificed after import of csharp.targets so that variables like $(OutDir) can be used in the events. with the SDK attribute there's no easy way to do that.

@jmarolf here is making the property pages spit out a

<Target Name="PreBuild" BeforeTargets="PreBuildEvent">
   <Exec Command="echo $(OutDir)" />
</Target>

to the project file but that's not the best experience. Any better ideas?

@davkean suggested that atleast it should be

<Target Name="PreBuild" BeforeTargets="PreBuildEvent">
      <PreBuildEvent>echo $(OutDir)</PreBuildEvent>
</Target>

This will require changes to msbuild though because the PreBuildEvent target is conditioned on the existence of the PreBuildEvent property.

@rainersigwald
Copy link

rainersigwald commented Jun 2, 2017

but that's not the best experience

Can you elaborate, @srivatsn? I like it a lot better. As the original reasoning here reveals, defining properties that get fed into an exec task later has . . . confusing behavior.

If you really love the current behavior, you could rewrite the project to have an explicit imports instead of implicit, and add the property definition in the old place.

@jmarolf
Copy link
Member Author

jmarolf commented Jun 5, 2017

We can do this:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>netcoreapp1.1</TargetFramework>
    <RunPostBuildEvent>Always</RunPostBuildEvent>
  </PropertyGroup>

  <Target Name="PreBuild" BeforeTargets="PreBuildEvent">
    <Exec Command="echo $(ProjectDir)" />
  </Target>

  <Target Name="PostBuild" AfterTargets="PostBuildEvent">
    <Exec Command="echo $(ProjectDir)" />
  </Target>

</Project>

or this:

<Project>

  <Import Sdk="Microsoft.NET.Sdk" Project="Sdk.props" />
  
  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>netcoreapp1.1</TargetFramework>
    <RunPostBuildEvent>Always</RunPostBuildEvent>
  </PropertyGroup>

  <Import Sdk="Microsoft.NET.Sdk" Project="Sdk.targets" />

  <PropertyGroup>
    <PreBuildEvent>echo $(ProjectDir)</PreBuildEvent>
    <PostBuildEvent>echo $(ProjectDir)</PostBuildEvent>
  </PropertyGroup>
  
</Project>

Both of which add six lines. I chose the first because the complexities of deciding when to change implicit/explicit target and props imports seemed like a drawback. Any other option would need to add special target or msbuild logic. Is that what we want to do?

@jmarolf
Copy link
Member Author

jmarolf commented Jun 5, 2017

@srivatsn I think this PR needs to handle

  1. Converting the old property syntax to the new one
  2. Persisting the explicit imports style is a user enters that into the project file manually

Anything else?

@rainersigwald
Copy link

rainersigwald commented Jun 5, 2017

@jmarolf Does MSBuild not do the latter already? Seems like we should.

@jmarolf
Copy link
Member Author

jmarolf commented Jun 5, 2017

@rainersigwald MSBuild does, I am talking about when the user navigates to the property page to set this value. The property page should know this alternate format and not change it.

@srivatsn
Copy link
Contributor

srivatsn commented Jun 5, 2017

I don't like unrolling the SDK attribute for the questions mentioned above - someone adds a prebuilevent and removes it, now we have to move it back to a Project SDK attribute but only if users haven't edit the csproj and add more stuff above\below the imports. The targets solution is better than the unrolling but it's more verbose and so I was just wondering if there was something better here (assuming we can change whatever we want in the core targets)

[ExportInterceptingPropertyValueProvider(_preBuildEventString, ExportInterceptingPropertyValueProviderFile.ProjectFile)]
internal class PreBuildEventValueProvider : AbstractBuildEventValueProvider
{
private const string _preBuildEventString = "PreBuildEvent";

This comment has been minimized.

@basoundr

basoundr Jun 10, 2017 Contributor

PreBuildEventString. Similarly for all the other private const strings.

private const string _execTaskName = "Exec";
private const string _commandString = "Command";

protected Helper(string buildEventString,

This comment has been minimized.

@basoundr

basoundr Jun 10, 2017 Contributor

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

public abstract class Helper
{
private const string _execTaskName = "Exec";
private const string _commandString = "Command";

This comment has been minimized.

@basoundr

basoundr Jun 10, 2017 Contributor

nit: String in the variable name seems redundant

{
public abstract class Helper
{
private const string _execTaskName = "Exec";

This comment has been minimized.

@basoundr

basoundr Jun 10, 2017 Contributor

nit: just ExecTask maybe?

SetTargetDependencies = setTargetDependencies;
}

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

This comment has been minimized.

@basoundr

basoundr Jun 10, 2017 Contributor

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


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

This comment has been minimized.

@basoundr

basoundr Jun 10, 2017 Contributor

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

return commandText;
}

return null; //unreachable

This comment has been minimized.

@basoundr

basoundr Jun 10, 2017 Contributor

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

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

This comment has been minimized.

@basoundr

basoundr Jun 10, 2017 Contributor

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

{
var targetArray = projectXml.Targets
.Where(target =>
GetTargetString(target) == BuildEventString &&

This comment has been minimized.

@basoundr

basoundr Jun 10, 2017 Contributor

case insensitive comparison here as well


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

This comment has been minimized.

@basoundr

basoundr Jun 10, 2017 Contributor

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

jmarolf added 4 commits Jun 10, 2017
Copy link

rainersigwald left a comment

I'm unreasonably excited about this!

<Target Name=""PostBuild"">
</Target>
<Target Name=""PostBuild1"" AfterTargets=""PostBuildEvent"">
<Exec Command=""echo &quot;post build $(OutDir)&quot;"" />

This comment has been minimized.

@rainersigwald

rainersigwald Jun 12, 2017

This is pretty awesome. Is there equivalent removal logic?

This comment has been minimized.

@jmarolf

jmarolf Jun 12, 2017 Author Member

I'll add a test

This comment has been minimized.

@jmarolf

jmarolf Jun 13, 2017 Author Member

test has been added

{
public abstract class AbstractBuildEventHelper
{
private const string _execTask = "Exec";

This comment has been minimized.

@basoundr

basoundr Jun 12, 2017 Contributor

private const string are named ExecTask

public string GetProperty(ProjectRootElement projectXml)
{
// Check if project file already has props in place for this.
var result = TryGetFromProperty(projectXml);

This comment has been minimized.

@basoundr

basoundr Jun 12, 2017 Contributor

I feel tuples are unnecessary. We can just check for null. I will leave it to you.

This comment has been minimized.

@jmarolf

jmarolf Jun 12, 2017 Author Member

I don't like the pattern of a method returning null to indicate failure. null is a valid thing to return from GetProperty itself. Someone could mistakenly assume that they should return null if one of the methods they call gives them null, instead of continuing. Roslyn uses Optional to indicate whether a returned value is valid, but this pattern was established before tuples.

{
var property = projectXml.Properties
.Where(prop => StringComparer.OrdinalIgnoreCase.Compare(prop.Name, BuildEvent) == 0)
.FirstOrDefault();

This comment has been minimized.

@basoundr

basoundr Jun 12, 2017 Contributor

Following the Msbuild evaluation order, we should be picking the Last. So we need to use LastOrDefault instead FirstOrDefault

{
var property = projectXml.Properties
.Where(prop => StringComparer.OrdinalIgnoreCase.Compare(prop.Name, BuildEvent) == 0)
.FirstOrDefault();

This comment has been minimized.

@basoundr

basoundr Jun 12, 2017 Contributor

LastOrDefault()

using (var access = await _projectLockService.WriteLockAsync())
{
var projectXml = await access.GetProjectXmlAsync(_unconfiguredProject.FullPath).ConfigureAwait(true);

This comment has been minimized.

@basoundr

basoundr Jun 12, 2017 Contributor

Remove Linebreak

namespace Microsoft.VisualStudio.ProjectSystem.VS.Properties
{
[ProjectSystemTrait]
public class PostBuildEventValueProviderTests

This comment has been minimized.

@basoundr

basoundr Jun 12, 2017 Contributor

These are the tests for the helper, which we can keep them in a separate file and add some basic tests for the Provider itself. Constructor validations, Get & Set calling the helpers properly

This comment has been minimized.

@jmarolf

jmarolf Jun 12, 2017 Author Member

Unless I am missing something ProjectLockAwaitable is un-mockable being a struct. Can you point me to other classes that use ReadLockAsync() and GetProjectXmlAsync()?

{
var root = @"
<Project Sdk=""Microsoft.NET.Sdk"">

This comment has been minimized.

@basoundr

basoundr Jun 12, 2017 Contributor

What happens to all the line breaks? Seems like we are modifying the format of the project file. This could be an issue unrelated to your change. Are we tracking it anywhere? If not, we should

This comment has been minimized.

@jmarolf

jmarolf Jun 12, 2017 Author Member

root.Save(TextWriter) and root.Save(string path) use completely different code paths. I'll change the tests to write out a file so the newlines are not changed

}

[Fact]
public static void SetPropertyTest_RemoveTarget_NewlineCharacter()

This comment has been minimized.

@basoundr

basoundr Jun 12, 2017 Contributor

We are not removing the target in this testcase. What is the expected behavior when the property is set to \r\n?

This comment has been minimized.

@jmarolf

jmarolf Jun 12, 2017 Author Member

In the old project system, newlines were persisted but spaces or tabs were not. I am preserving this behavior

This comment has been minimized.

@basoundr

basoundr Jun 13, 2017 Contributor

ok. Rename the test name accordingly. Test still says RemoveTarget

namespace Microsoft.VisualStudio.ProjectSystem.VS.Properties
{
[ProjectSystemTrait]
public class PostBuildEventValueProviderTests

This comment has been minimized.

@basoundr

basoundr Jun 12, 2017 Contributor

We need some tests to show that we handle case insensitivity as well

This comment has been minimized.

@jmarolf

jmarolf Jun 13, 2017 Author Member

done

namespace Microsoft.VisualStudio.ProjectSystem.VS.Properties
{
[ProjectSystemTrait]
public class PreBuildEventValueProviderTests

This comment has been minimized.

@basoundr

basoundr Jun 12, 2017 Contributor

Since the tests are a mirror of the PostBuildEvent, all the comments, above, apply here as well

return GetFromTargets(projectXml);
}

public void SetProperty(string unevaluatedPropertyValue, ProjectRootElement projectXml)

This comment has been minimized.

@basoundr

basoundr Jun 12, 2017 Contributor

Get and Set Property are doing different things. Get does not care if there are more than one Task and always uses the First Task. Set, on the other hand, when trying to remove, looks for the target with just one Exec task.

This behavior accomadates the common scenario. But we need to handle the other scenario of having more than one task because this behavior silently passes without telling the user something was wrong.

This comment has been minimized.

@basoundr

basoundr Jun 12, 2017 Contributor

We need not address this issue in this PR. We can open an issue to track this though.

This comment has been minimized.

@jmarolf

jmarolf Jun 12, 2017 Author Member

My logic is:

  • if the pre-build or post-build target has multiple exec tasks there is no way for us to know which one to use so we select the first one. That way we can at least make the behavior observably deterministic
  • If the user deletes the text of a post or pre build event (or sets it to just spaces and tabs) we remove the target only if there is only one exec task. This ensures that we do not remove targets that have custom logic written by the developer.

Both cases, the user has written something custom so we err on the side of not changing things. What do we need to communicate to the user in the scenario:

But we need to handle the other scenario of having more than one task because this behavior silently passes without telling the user something was wrong.

jmarolf added 5 commits Jun 12, 2017
…uildEvents-work-using-targets' into bugfix/Make-PreBuildEvents-PostBuildEvents-work-using-targets
@jmarolf
Copy link
Member Author

jmarolf commented Jun 13, 2017

@balajikris @basoundr can you do another review pass? thanks!

@davkean
Copy link
Member

davkean commented Jun 13, 2017

@jmarolf Can you summarize the new behavior?

@jmarolf
Copy link
Member Author

jmarolf commented Jun 13, 2017

@davkean here are the basics. I can write a more formal doc.

  1. If the user has PreBuildEvent or PostBuildEvent properties defined already in their project we use those properties and do not attempt to convert them. So this proj file is not modified even though the msbuild variable ProjectDir will not be correctly evaluated
<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>netcoreapp1.1</TargetFramework>
    <PreBuildEvent>echo $(ProjectDir)</PreBuildEvent>
    <PostBuildEvent>echo $(ProjectDir)</PostBuildEvent>
  </PropertyGroup>
</Project>

image

  1. If the user has no PreBuildEvent or PostBuildEvent properties defined we will add a target to their project with an exec task when they modify the build events tab.

So this:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>netcoreapp1.1</TargetFramework>
  </PropertyGroup>
</Project>

Becomes this:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>netcoreapp1.1</TargetFramework>
  </PropertyGroup>
  <Target Name="PreBuild" BeforeTargets="PreBuildEvent">
    <Exec Command="echo $(ProjectDir)" />
  </Target>
  <Target Name="PostBuild" AfterTargets="PostBuildEvent">
    <Exec Command="echo $(ProjectDir)" />
  </Target>
</Project>

When the build events tab looks like this:
image

On a new project

  1. In the event that the user has a target defined in their project that does not match

This for prebuild

<Target Name="PreBuild" BeforeTargets="PreBuildEvent">
  <Exec Command="echo $(ProjectDir)" />
</Target>

Or this for postbuild:

<Target Name="PostBuild" AfterTargets="PostBuildEvent">
  <Exec Command="echo $(ProjectDir)" />
</Target>

We will add a new target with a unique name (Post/Pre Build + N) whenever they edit the build events tab

  1. If the developer deletes their text from the build events tab we will remove the targets iff they match the rules for 3
@tannergooding
Copy link
Member

tannergooding commented Jun 13, 2017

So, if you have two targets with the same 'BeforeTargets':

  <Target Name="PreBuild1" BeforeTargets="PreBuildEvent">
    <Exec Command="echo Hello" />
  </Target>

  <Target Name="PreBuild2" BeforeTargets="PreBuildEvent">
    <Exec Command="echo World" />
  </Target>

We will only display the first:
image

Modifying it (and saving) updates the first target:

  <Target Name="PreBuild1" BeforeTargets="PreBuildEvent">
    <Exec Command="echo Hello, " />
  </Target>

  <Target Name="PreBuild2" BeforeTargets="PreBuildEvent">
    <Exec Command="echo World" />
  </Target>

Fully deleting the text (and saving) causes the first target (PreBuild1) to be deleted from the csproj file.

  <Target Name="PreBuild2" BeforeTargets="PreBuildEvent">
    <Exec Command="echo World" />
  </Target>

Then, if you modify the text box again (and save), we now end up modifying the second target:

This results in us overwriting user targets (without them being aware that it switched the target is was matched against).

private (bool success, string property) TryGetFromProperty(ProjectRootElement projectXml)
{
// Choose last or default as that matches the evaluation order for MSBuild
var property = projectXml.Properties

This comment has been minimized.

@srivatsn

srivatsn Jun 15, 2017 Contributor

Don't use the construction model to get properties - if the property was inside a condition or if it's for a certain configuration then this will be wrong. Instead, import ProjectPropertiex and get the PreBuildEvent property from GeneralBrowseObject - however you should set it's SourceOfDefaultValue=BeforeContext here so that it doesn't give you the final value.

This comment has been minimized.

@srivatsn

srivatsn Jun 15, 2017 Contributor

Ah actually just noticed that you are implementing an intercepter for this property anyway. So just check if evaluatedProeprtyValue here is non-null. You can avoid this whole TryGet\SetFromProperty thing. You'd still need to change SourceOfDefaultValue.

private bool TrySetProperty(string unevaluatedPropertyValue, ProjectRootElement projectXml)
{
// Choose last or default as that matches the evaluation order for MSBuild
var property = projectXml.Properties

This comment has been minimized.

@srivatsn

srivatsn Jun 15, 2017 Contributor

Same as above - go through ProjectProperties.


}

private string FindNonCollidingName(string buildEvent, HashSet<string> targetNames)

This comment has been minimized.

@srivatsn

srivatsn Jun 15, 2017 Contributor

Let`s not do this - if the exact name doesn't match then that should be an error - this just complicates things without much value.

- removing target duplicate detection
- restricting target build name
@jmarolf jmarolf force-pushed the jmarolf:bugfix/Make-PreBuildEvents-PostBuildEvents-work-using-targets branch from 1c319de to f5e4e00 Jun 16, 2017
@srivatsn
Copy link
Contributor

srivatsn commented Jun 19, 2017

Tagging @MattGertz for Preview4


public static string SaveAndGetChanges(this ProjectRootElement root)
{
var tempFile = Path.GetTempFileName();

This comment has been minimized.

@vshapenko

vshapenko Jun 20, 2017

Why cant we simply use root.Save(TextWriter) here?

This comment has been minimized.

@jmarolf

jmarolf Jun 20, 2017 Author Member

@vshapenko saving to a string will cause a different code path to be used that writes out the file differently. I am writing to disk here because I want the same behavior in the unit tests and what MSBuild actually does. I can look for other ways around this and revisit this is a subsequent PR

@srivatsn
Copy link
Contributor

srivatsn commented Jun 20, 2017

@MattGertz tagging again in case you missed this - this is one of the top vsfeedback issues we've had.

@jmarolf jmarolf merged commit 682e420 into dotnet:master Jun 20, 2017
2 checks passed
2 checks passed
Windows Debug Build finished.
Details
Windows Release Build finished.
Details
@jmarolf jmarolf deleted the jmarolf:bugfix/Make-PreBuildEvents-PostBuildEvents-work-using-targets branch Oct 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.