-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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.
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
Shorten the path always when actually passing the long paths #9223
base: main
Are you sure you want to change the base?
Conversation
@JaynieBai Could you comment on this PR and fill the description? |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looking good!
One comment left probably nit
@@ -314,14 +314,14 @@ public async Task<bool> ExecuteInternal() | |||
{ | |||
ITaskItem project = Projects[i]; | |||
|
|||
string projectPath = FileUtilities.AttemptToShortenPath(project.ItemSpec); | |||
string projectPath = FileUtilities.GetFullPathNoThrow(project.ItemSpec); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like we shorten the path always after this change. I guess the explanation for the change is "So, when project path includes multiple reductant "..\" not longer than the max path, it will report MSBuild.ProjectFileNotFound.". I am a bit confused: if the path is not too long, how "MSBuild.ProjectFileNotFound" is thrown? I would think that the file would be found in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When project path includes multiple reductant ".." not longer than the max path. The code here FileSystems.Default.FileExists(projectPath)
will return false.
msbuild/src/Build/BackEnd/Components/RequestBuilder/IntrinsicTasks/MSBuild.cs
Lines 343 to 392 in b59f07e
if (FileSystems.Default.FileExists(projectPath) || (skipNonExistProjects == SkipNonExistentProjectsBehavior.Build)) | |
{ | |
if (FileUtilities.IsVCProjFilename(projectPath)) | |
{ | |
Log.LogErrorWithCodeFromResources("MSBuild.ProjectUpgradeNeededToVcxProj", project.ItemSpec); | |
success = false; | |
continue; | |
} | |
// If we are building in parallel we want to only make one call to | |
// ExecuteTargets once we verified that all projects exist | |
if (!BuildInParallel) | |
{ | |
singleProject[0] = project; | |
bool executeResult = await ExecuteTargets( | |
singleProject, | |
propertiesTable, | |
undefinePropertiesArray, | |
targetLists, | |
StopOnFirstFailure, | |
RebaseOutputs, | |
BuildEngine3, | |
Log, | |
_targetOutputs, | |
UnloadProjectsOnCompletion, | |
ToolsVersion, | |
SkipNonexistentTargets); | |
if (!executeResult) | |
{ | |
success = false; | |
} | |
} | |
else | |
{ | |
skipProjects[i] = false; | |
} | |
} | |
else | |
{ | |
if (skipNonExistProjects == SkipNonExistentProjectsBehavior.Skip) | |
{ | |
Log.LogMessageFromResources(MessageImportance.High, "MSBuild.ProjectFileNotFoundMessage", project.ItemSpec); | |
} | |
else | |
{ | |
ErrorUtilities.VerifyThrow(skipNonExistProjects == SkipNonExistentProjectsBehavior.Error, "skipNonexistentProjects has unexpected value {0}", skipNonExistProjects); | |
Log.LogErrorWithCodeFromResources("MSBuild.ProjectFileNotFound", project.ItemSpec); | |
success = false; | |
} |
It looks like the root of the problem here is that our custom Windows-only implementation of msbuild/src/Framework/NativeMethods.cs Lines 1725 to 1731 in 053feb0
BCL API accepts paths like @JaynieBai, can you please check why exactly |
I Find FileSystems.Default.FileExists calls the unmanage function
.NET Framework Output
.Net Output
|
I agree with @ladipro here that we need to make sure a change happens on a correct abstraction layer and isolated only to the failing scenario, given the perf concerns. @rainersigwald do you have some knowledge why we have a custom Windows-only implementation of File.Exists here? |
Synced with @AR-May on this - our conclusion: The root of the problem is in our custom implementation of FileExists (
|
BenchmarkDotNet v0.13.12, Windows 11 (10.0.22631.4037/23H2/2023Update/SunValley3)
No big difference in time, but managed implementation allocates. |
Thanks @MichalPavlik! We should probably file a bug with Runtime team. I'm wondering how does this allocation influence overall build perf. @JaynieBai - can you run OrchardCore build with current version (custom FileExists impl) vs version where we'd switch to |
@JanKrivanek, it was a little bit misleading as I forgot to test BenchmarkDotNet v0.13.12, Windows 11 (10.0.22631.4037/23H2/2023Update/SunValley3)
This one doesn't allocate :)
|
@JanKrivanek, @JaynieBai Replacing the |
I have no strong preference between the 2 ( |
@JaynieBai, please rewrite the #if NETFRAMEWORK
return Microsoft.IO.File.Exists(path);
#else
return File.Exists(path);
#endif This simple focused change is enough for now. We will talk about the rest of P/Invokes later. |
@MichalPavlik Microsoft.Build.UnitTests.XMakeAppTests.ConfigurationInvalid failed since not find the project file when build with copied MSBuild in the temp folder. |
This was tricky. The reason why is MS.IO.Redist returning
This exception is silently ignored as @rainersigwald Is this test critical enough to have such a complicated functional test? If so, then we have these options:
I would personally prefer to find a way how to simplify the test if we really need this one. These kind of tests can fail on magnitude of reasons, and investigation is costly. Edit: Bug report created - dotnet/maintenance-packages#117 |
No opinions so far. Let's use the simplest way :) @JaynieBai, please replace the XElement configRuntimeElement = XDocument.Load(RunnerUtilities.PathToCurrentlyRunningMsBuildExe + ".config").Root.Element("runtime");
string configContent = $@"<?xml version =""1.0""?>
<configuration>
<configSections>
<section name=""msbuildToolsets"" type=""Microsoft.Build.Evaluation.ToolsetConfigurationSection, Microsoft.Build, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a"" />
<foo/>
</configSections>
<startup>
<supportedRuntime version=""v4.0""/>
</startup>
<foo/>
<msbuildToolsets default=""X"">
<foo/>
<toolset toolsVersion=""X"">
<foo/>
<property name=""MSBuildBinPath"" value=""Y""/>
<foo/>
</toolset>
<foo/>
</msbuildToolsets>
{configRuntimeElement}
</configuration>"; It adds all assembly redirects to the config file and the |
Ah sorry. I like that plan @MichalPavlik. |
Fixes #4272
Context
https://github.com/Microsoft/msbuild/blob/94e11e0a773bc8956caf128335433231bb06fed5/src/Build/BackEnd/Components/RequestBuilder/IntrinsicTasks/MSBuild.cs#L314 .
msbuild/src/Shared/FileUtilities.cs
Lines 1192 to 1199 in 85b7177
In the function AttemptToShortenPath, It is only dealing with the path is long than the max path. So, when project path includes multiple reductant "..\" not longer than the max path, it will report MSBuild.ProjectFileNotFound.
Changes Made
Get the project full path to shorten the path always
Testing
Notes