Skip to content
This repository has been archived by the owner on Apr 20, 2023. It is now read-only.

Remove temp project path from tool install warnings and errors. #8734

Merged
merged 2 commits into from
Mar 8, 2018
Merged

Remove temp project path from tool install warnings and errors. #8734

merged 2 commits into from
Mar 8, 2018

Conversation

peterhuene
Copy link

This commit attempts to filter the diagnostic messages emitted during tool
installation. The diagnostic messages may be prefixed with the temporary
project; since this is an implementation detail that only causes confusion and
clutter in the diagnostic messages, the prefix is removed if present.

Fixes #8707.

@peterhuene peterhuene requested review from wli3 and a team March 7, 2018 00:21
nguerrera
nguerrera reviewed Mar 7, 2018
View reviewed changes
src/dotnet/commands/dotnet-install/dotnet-install-tool/ProjectRestorer.cs Outdated
line = line ?? "";

// Remove the temp project prefix if present
if (line.StartsWith($"{project.Value} : ", StringComparison.CurrentCultureIgnoreCase))

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

nguerrera
nguerrera reviewed Mar 7, 2018
View reviewed changes
src/dotnet/commands/dotnet-install/dotnet-install-tool/ProjectRestorer.cs Outdated
// Remove the temp project prefix if present
if (line.StartsWith($"{project.Value} : ", StringComparison.CurrentCultureIgnoreCase))
{
line = line.Substring(project.Value.Length + 3);

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

wli3
wli3 reviewed Mar 7, 2018
View reviewed changes
src/dotnet/commands/dotnet-install/dotnet-install-tool/ProjectRestorer.cs Outdated
{
command = command
.OnOutputLine((line) => _reporter.WriteLine(line))
.OnErrorLine((line) => _reporter.WriteLine(line));
.OnOutputLine(line => WriteLine(_reporter ?? Reporter.Output, line, project))

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

wli3
wli3 approved these changes Mar 7, 2018
View reviewed changes
Copy link

@wli3 wli3 left a comment

Choose a reason for hiding this comment

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

Other than reporter could be null. But it does not worth blocking

Remove temp project path from tool install warnings and errors.
e2d9c2f 
This commit attempts to filter the diagnostic messages emitted during tool
installation.  The diagnostic messages may be prefixed with the temporary
project; since this is an implementation detail that only causes confusion and
clutter in the diagnostic messages, the prefix is removed if present.

Fixes #8707.
@peterhuene
Copy link
Author

The RHEL failure occurred again (from the list tool command changes, this now manifests as a "failure to locate the staged package" following the restore). As a theory, I've pushed up a commit to remove the reliance of setting the current working directory from the tool package installer tests in the hopes of improving the stability of the tests.

I still need to setup a RHEL VM to see if I can manually reproduce the failure with what I suspect the issue to be.

@peterhuene
Copy link
Author

peterhuene commented Mar 7, 2018 •
edited
Loading

Oh right, the broken mock implementation on Windows when using TempRoot. Let me fix that.

Stop modifying current working directory from test.
52478e8 
This commit fixes the ToolPackageInstaller tests so that they no longer modify
the current working directory.  The directory being set is now being properly
passed in as an argument to override the default of the current working
directory.

Additionally, this commit also changes the package root to a temp location
rather than based off of the current working directory.
@peterhuene
Copy link
Author

I tracked down the Windows failure to TempRoot.Root containing both / (including a double //) and \ in the path. This caused the mock file system to enumerate incorrectly because no path normalization was being performed and certain Path functions were returning differing results from what was stored in the internal Dictionary.

I first attempted to fix the mock file system to try to normalize all the path interactions, but this was a more invasive change that impacted other tests. I decided to just normalize the use of TempRoot.Root to fix this particular issue.

@nguerrera @wli3 can you review the second commit that was pushed? Hopefully it will bring some stability to the tool package installer tests.

@nguerrera
Copy link
Contributor

Second commit looks good. Thanks.

@wli3
Copy link

wli3 commented Mar 8, 2018

ack, let me check

@wli3
Copy link

wli3 commented Mar 8, 2018

Good to go, but not perfect.

52478e8#diff-514fdb6bf1ab21b1a21adfd2ccf82145R121 is a test induced design damage. And the test is just testing the nuget flag. I do think use a test to lock nuget's behavior that we depend on is important since they are in rapid development. A regression is very likely. But adding another optional just for test making it further away from production.

Let me also see if there is other alternative. One way is to make is closer to real. Find a better mock library. Or make is less dependent on file system state.

@wli3
Copy link

wli3 commented Mar 8, 2018

I mean please checkin. I'll keep digging on my own

@peterhuene
Copy link
Author

Agreed, an option just for testing purposes is not ideal. Let's work out a proper solution after this change.

@peterhuene peterhuene merged commit 4c2b070 into dotnet:master Mar 8, 2018
@peterhuene peterhuene deleted the tool-install-message-cleanup branch March 8, 2018 23:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@nguerrera nguerrera nguerrera approved these changes

@wli3 wli3 wli3 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Tool install with nuget error should not display the temp project in output
3 participants
@peterhuene @nguerrera @wli3