Skip to content

Conversation

@teo-tsirpanis
Copy link
Contributor

There is already a ToolPath property in the base ToolTask class.
This PR removes Fsc's duplicate definition.

@cartermp
Copy link
Contributor

There are some failed tests here, could you take a look? https://dev.azure.com/dnceng/public/_build/results?buildId=815659&view=ms.vss-test-web.build-test-results-tab

@teo-tsirpanis
Copy link
Contributor Author

The test's logic is flawed. It assumes that ToolTask.GenerateFullPathToTool is the single source of truth for the task's tool path, but it's actually the opposite: according to documentation, GenerateFullPathToTool is a fallback that gets called if the ToolTask.ToolPath property is empty.

I think this test should be either removed or adapted to the correct MSBuild behavior.

@dsyme
Copy link
Contributor

dsyme commented Sep 19, 2020

I think this test should be either removed or adapted to the correct MSBuild behavior.

Please adapt the test as part of the PR and we can assess

Copy link
Contributor

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

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

Thanks for this change.

@cartermp cartermp merged commit ac18f76 into dotnet:main Oct 5, 2020
@teo-tsirpanis teo-tsirpanis deleted the patch-1 branch October 5, 2020 05:33
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Feb 23, 2021
* Refactor the Fsc task's toolpath logic.

* Remove invalid test case

Co-authored-by: KevinRansom <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants