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.

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

Enable max path detection #3631

Merged
merged 3 commits into from
Dec 2, 2018
Merged

Enable max path detection #3631

merged 3 commits into from
Dec 2, 2018

Conversation

ccastanedaucf
Copy link
Contributor

Also might be useful to have the ability to revert to old behavior if necessary. Should this be controlled via environment variable?

rainersigwald
rainersigwald reviewed Aug 20, 2018
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.

Yes, I think we should have an escape hatch for this. Can you add one in src/shared/Traits.cs, modeled after for example

https://github.com/Microsoft/msbuild/blob/7b3a457540ab87cf02f6d41ce6095e6ecbbb6ea7/src/Shared/Traits.cs#L152-L155

?

@ccastanedaucf ccastanedaucf reopened this Aug 20, 2018
@ccastanedaucf
Copy link
Contributor Author

Well that's a new one. Test ForceSomeOutOfDate kept failing on .NET Core both locally and on CI for a few hours while I couldn't figure out what could have possibly changed in behavior. Then it started passing locally and on CI again.

@rainersigwald
Copy link
Member

I looked at the test and the failing assertion and I don't think I can explain that 😕

@ccastanedaucf ccastanedaucf force-pushed the enable-maxpath-detection branch from 52417a7 to 74ed9af Compare September 24, 2018 17:00
@ccastanedaucf ccastanedaucf force-pushed the enable-maxpath-detection branch from 74ed9af to 5bab36f Compare November 26, 2018 21:43
@ccastanedaucf ccastanedaucf merged commit 45bc74d into dotnet:master Dec 2, 2018
@ccastanedaucf ccastanedaucf mentioned this pull request Dec 7, 2018
Merged
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

@AndyGerlicher AndyGerlicher Awaiting requested review from AndyGerlicher

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

Successfully merging this pull request may close these issues.
2 participants
@ccastanedaucf @rainersigwald