Fix warning when newline in hintpath - #4598
Conversation
wli3
changed the title
wli3
changed the title
|
I don't understand how the repro project I saw in 16.1 would have been quiet, but not resolve to the path. I would have expected a build warning for unresolved reference if the path was not interpreted as trimmed in 2.1. Did it resolve from another resolver than HintPath? |
|
I definitely don't want to have no warning, but also ignore the hint path. I'm assuming either warn and ignore or don't warn and trim. |
|
Do you have a binlog, or can you make a binlog for 16.1 building this: #4593 (comment) |
|
I agree: I'd like to know exactly what caused the 16.1 behavior before we decide how to fix this. |
|
Oh, I see the difference. The reference ItemSpec is to System.Net.Http so it resolves to target framework and hint path with new lines is treated as just not found which is not a warning if another resolver finds it. |
|
So this goes back to what I said originally: we should not trim in sdk conflict resolution if rar does not. What a mess. |
|
So which way we should we choose? I prefer a. sounds closer to what customer want |
|
a is better behavior, but I think since it's late in 16.3, we should just revert to the 16.1 behavior. Then there's no risk of breaking people with incorrect hint paths that weren't being respected because of whitespace. |
|
So take this change as is? I will later change the sdk part. |
wli3
changed the title
|
Also do i need ask mode? 16.3 is master? |
|
|
| ) | ||
| { | ||
| if (!string.IsNullOrEmpty(hintPath)) | ||
| if (!string.IsNullOrEmpty(hintPath) && !FileUtilities.PathIsInvalid(hintPath)) |
There was a problem hiding this comment.
I think this deserves a comment. Probably with a link to the bug to consider trimming HintPath in 17.0.
There was a problem hiding this comment.
Added comment, fc762c4
and I do need to have an ask mode write up
wli3
changed the title
wli3
changed the title
|
get approved from email. merging |
3 participants
Description
If there are newlines or white spaces passed to
Path.GetFullpath, it will throw on fullframework. AddingPath.GetFullpathin previous change caused extra warning from Resolve Assembly Reference. This change added "skip when the path is not valid" to being back the "no warning but skip" behavior of 16.1.Customer Impact
If the customer has warning as error enabled, a build would be successful in 16.1 but would be fail in the latest version
Regression?
Yes. by #4414, since 16.2
Risk
Low risk.
Test changes in this PR
Additional unit tests and manual end to end tests.
fix #4593
This is a strict fix for the warning problem. Please look at
CannotNormalizePathWithNewLineAndSpacetest.FileUtilities.NormalizePathwill get garbage output like (c:\work\test \r\n c:\work\referenced\file.dll) when called with newline. SoFile.Existscannot find the correct file anyway. The fix is only to make it quiet.It behaves differently in core because Path.GetFullpath no longer throws when there is invalid char
To be discuss