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.

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

Remove unnecessary GetBackgroundCheckResultsForFileInProject #977

Merged
merged 7 commits into from Aug 16, 2022

Conversation

Krzysztof-Cieslak
Copy link
Member

This is some residue of when Checker was implicitly queuing type checking in the background. Since we're handling all checking explicitly now, we don't need it. I suspect it may have been causing some performance issues in the last few FSAC releases.

@razzmatazz
Copy link
Contributor

@Krzysztof-Cieslak I cannot build this on macos/m1 (where I experience perf/cpu heat problems) due to dotnet fake build failing with

Script is not valid:
	unknown (1,0)-(1,0): Error FS0193: The specified file name or path is too long, or a component of the specified path is too long.

#979 (review) could fix this -- will check now

@razzmatazz
Copy link
Contributor

razzmatazz commented Jul 31, 2022

@Krzysztof-Cieslak I cannot build this on macos/m1 (where I experience perf/cpu heat problems) due to dotnet fake build failing with

Script is not valid:
	unknown (1,0)-(1,0): Error FS0193: The specified file name or path is too long, or a component of the specified path is too long.

#979 (review) could fix this -- will check now

ok, that did not help; the underlying issue for macos/aarch64 brekage is apparently in dotnet runtime:

that seems to only have been applied to dotnet/7 (and not dotnet/6)

@razzmatazz
Copy link
Contributor

I have registered an issue on FAKE repo:

I cannot even build FAKE itself to upgrade runtime requirement to dotnet/7-preview since fake uses fake itself to build :(

@baronfel baronfel force-pushed the perfFix/removeBackgroundCheckResultsCall branch from 430f6dc to fce2b85 Compare July 31, 2022 14:33
@baronfel
Copy link
Contributor

rebased this on top of latest main to get the fix for FAKE

@baronfel
Copy link
Contributor

This seems to be failing tests because the gototests.fsproj project isn't being built correctly/successfully:

Error reading assets file: Error loading lock file 'C:\\Users\\chethusk\\OSS\\fsac-fast\\test\\FsAutoComplete.Tests.Lsp\\TestCases\\GoToTests\\obj\\project.assets.json' : The type initializer for 'NuGet.ProjectModel.JsonUtility' threw an exception.

We might have an out-of-sync nuget dependency. We should be treating any nuget deps the same as we do msbuild - pulling them from the SDK instead of packaging locally.

@baronfel
Copy link
Contributor

Now we're past the loading issue, and back to the root issue - for some reason the checker doesn't see any tcInfoExtras for these files when performing partialCheckResults.TryPeekTcInfoWithExtras() inside the GetUsesOfSymbol(Symbol, CTok) method.

@Krzysztof-Cieslak Krzysztof-Cieslak force-pushed the perfFix/removeBackgroundCheckResultsCall branch from 9a5b08d to c2195ac Compare August 16, 2022 12:20
@@ -20,6 +20,7 @@ nuget Microsoft.Build copy_local:false
nuget Microsoft.Build.Framework copy_local:false
nuget Microsoft.Build.Utilities.Core copy_local:false
nuget Microsoft.Build.Tasks.Core copy_local: false
nuget NuGet.Frameworks
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we still need this set as copy local false like the MSBuild deps, so that we load the assembly from the SDK when we use msbuild via the SDK. It's used by msbuild internally and if we ship a version that is out of sync with the version shipped in the SDK and used by Msbuild then projects stop loading.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants