Skip to content

Permit a task to disable MSB4181 Fixes #5203 #5207

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

Jump to bottom
Merged
Forgind merged 4 commits into from
Apr 28, 2020
Merged

Permit a task to disable MSB4181 Fixes #5203 #5207

Forgind merged 4 commits into from
Apr 28, 2020

Conversation

@Forgind
Copy link
Contributor

@Forgind Forgind commented Mar 27, 2020 •
edited
Loading

Using IBuildEngine7, one can disable MSB4132 in a task.

Fixes #5203
Also fixes #2036

@Forgind Forgind mentioned this pull request Mar 27, 2020
Closed
sae42
sae42 reviewed Mar 30, 2020
View reviewed changes
Copy link

@sae42 sae42 left a comment

Choose a reason for hiding this comment

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

A few places are returning IBuildEngine6 rather than IBuildEngine7.

src/Utilities/Task.cs Outdated
Copy link

@sae42 sae42 Mar 30, 2020

Choose a reason for hiding this comment

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

Shouldn't this return IBuildEngine7?

ref/Microsoft.Build.Utilities.Core/netstandard/Microsoft.Build.Utilities.Core.cs Outdated
Copy link

@sae42 sae42 Mar 30, 2020

Choose a reason for hiding this comment

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

Shouldn't this return IBuildEngine7?

ref/Microsoft.Build.Utilities.Core/net/Microsoft.Build.Utilities.Core.cs Outdated
Copy link

@sae42 sae42 Mar 30, 2020

Choose a reason for hiding this comment

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

Shouldn't this return IBuildEngine7?

rainersigwald
rainersigwald requested changes Mar 31, 2020
View reviewed changes
src/Build/BackEnd/Components/RequestBuilder/TaskHost.cs Outdated
Copy link
Member

@rainersigwald rainersigwald Mar 31, 2020

Choose a reason for hiding this comment

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

Why is this an explicit set method rather than a setter?

Copy link
Contributor Author

@Forgind Forgind Mar 31, 2020

Choose a reason for hiding this comment

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

You mean a property with {get; set;}? I don't think you can access that with reflection.

Copy link
Member

@rainersigwald rainersigwald Mar 31, 2020

Choose a reason for hiding this comment

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

You can, with Type.GetProperty

src/Build/Resources/Strings.resx Outdated
Copy link
Member

@rainersigwald rainersigwald Mar 31, 2020

Choose a reason for hiding this comment

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

This appears to have been used, too:

MSB4134: DefaultToolsVersion cannot be set after a project has been loaded into the Engine.

That's coming entirely from the Deprecated\Engine folder, but we still shouldn't recycle.

src/Framework/IBuildEngine7.cs Outdated
Copy link
Member

@rainersigwald rainersigwald Mar 31, 2020

Choose a reason for hiding this comment

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

Don't include the error code in the name of this method. Give it a descriptive name instead, like maybe

Suggested change
public void SetEnableMSB4134(bool errorEnabled);
public void AllowFailureWithoutError(bool errorEnabled);

?

src/Tasks.UnitTests/AssemblyDependency/AssemblyFoldersFromConfig_Tests.cs Outdated
Copy link
Member

@rainersigwald rainersigwald Mar 31, 2020

Choose a reason for hiding this comment

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

Please separate this into its own PR.

Copy link
Member

@rainersigwald rainersigwald Apr 28, 2020

Choose a reason for hiding this comment

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

Ping

@Forgind Forgind changed the title Permit a task to disable MSB4132 Fixes #5203 Permit a task to disable MSB4181 Fixes #5203 Apr 3, 2020
@rainersigwald
Copy link
Member

rainersigwald commented Apr 3, 2020

Team triage: we'd like to reserve IBuildEngine7 for some changes going into 16.7 that are already planned. Let's delay this change until then as well.

@rainersigwald rainersigwald added this to the MSBuild 16.7 milestone Apr 3, 2020
@sae42
Copy link

sae42 commented Apr 6, 2020

@rainersigwald If this fix has to be deferred to 16.7 is there any chance of reverting the original change that caused this issue until 16.7 also? Failing that, please at least correct the error code so that it will be consistent between releases.

@benvillalobos benvillalobos mentioned this pull request Apr 7, 2020
Merged
@Forgind Forgind mentioned this pull request Apr 13, 2020
Closed
@Forgind Forgind force-pushed the MSB4132-opt-out branch 2 times, most recently from 551aea7 to 583561a Compare April 17, 2020 19:38
rainersigwald
rainersigwald requested changes Apr 28, 2020
View reviewed changes
Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

Can you rebase onto current master instead of merging?

src/Tasks.UnitTests/AssemblyDependency/AssemblyFoldersFromConfig_Tests.cs Outdated
Copy link
Member

@rainersigwald rainersigwald Apr 28, 2020

Choose a reason for hiding this comment

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

Ping

Forgind added 4 commits April 28, 2020 08:48
@Forgind
Revert "Reverted and rebuilt for localizable strings." (dotnet#5246)
f569389 
This adds back support for logging an error when a task returns false
without logging an error. This was originally added in dotnet#4940 but was
reverted because of multiple difficulties.
@Forgind
Changed error code
88506b5
@Forgind
Add escape hatch
989f35d
@Forgind
Fix typo
db1feb2
rainersigwald
rainersigwald approved these changes Apr 28, 2020
View reviewed changes
Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

Merge please, don't squash.

@Forgind Forgind merged commit f4dacba into dotnet:master Apr 28, 2020
@Forgind Forgind deleted the MSB4132-opt-out branch April 28, 2020 21:20
@nohwnd nohwnd mentioned this pull request Sep 2, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rainersigwald rainersigwald rainersigwald approved these changes

+1 more reviewer

@sae42 sae42 sae42 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

MSBuild 16.7

Development

Successfully merging this pull request may close these issues.

MSB4132 is emitted when a task logs errors in custom format Log our own error if a task returned false but didn't log any errors

3 participants

@Forgind @rainersigwald @sae42