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

cache script per file instead of per directory #596

Conversation

DanielEgbers
Copy link

Description

Currently only one script per directory is cached.

This PR adds the script file name into the cache directory path, therefore caching scripts per file instead of per directory.

Example:

Script Current Cache New Cache
D:\scriptA.csx %tmp%\dotnet-script\D\execution-cache\script.dll %tmp%\dotnet-script\D\scriptA.csx\execution-cache\script.dll
D:\scriptB.csx %tmp%\dotnet-script\D\execution-cache\script.dll %tmp%\dotnet-script\D\scriptB.csx\execution-cache\script.dll

Motivation and Context

I would like to benefit from the execution speed up by caching without having to separate every script into its own directory.

@filipw
Copy link
Member

filipw commented Nov 24, 2020

Thanks, that looks reasonable.

The folder concept is a bit complicated in scripting, because e.g. in nuget intellisense handling, package availability in OmniSharp is handled per folder. If I recall correctly, this was also a reason why caching behaves this way.

@seesharper do you have any objections to change this?

@DanielEgbers
Copy link
Author

Interesting, I did not know about that.

I now tested my changes in combination with VSCode/OmniSharp.

Usability wise, everything seems to work normal including intellisense for nuget packages.

Technically wise, this PR only changes the location of the execution-cache.
The script project cache path (that folder named after the TargetFramework) was not changed and is therfore still directory based. VSCode/OmniSharp only seems to use this script project cache.

Temp-Folder content before this PR:

%tmp%\dotnet-script\D\execution-cache\script.dll
%tmp%\dotnet-script\D\net5.0\script.csproj
%tmp%\dotnet-script\D\net5.0\obj\
%tmp%\dotnet-script\D\net5.0\compilation\

Temp-Folder content after this PR:

%tmp%\dotnet-script\D\scriptA.csx\execution-cache\script.dll
%tmp%\dotnet-script\D\scriptB.csx\execution-cache\script.dll
%tmp%\dotnet-script\D\net5.0\script.csproj
%tmp%\dotnet-script\D\net5.0\obj\
%tmp%\dotnet-script\D\net5.0\compilation\

@DanielEgbers
Copy link
Author

I have used a custom dotnet-script version including this change for the last 3 months. Debugging and writing (with autocomplete and so on) in VSCode works normal just like before.

@filipw can this get merged?

Copy link
Collaborator

@seesharper seesharper left a comment

Choose a reason for hiding this comment

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

Looks good to me. Could you just rebase this from master and we should be good to go 👍

@DanielEgbers DanielEgbers force-pushed the feature/cache-per-file-instead-of-directory branch from c236c3b to 2a098f2 Compare March 1, 2021 23:55
@DanielEgbers
Copy link
Author

I rebased it. Thanks

@seesharper seesharper requested a review from filipw March 2, 2021 22:41
@seesharper
Copy link
Collaborator

@filipw Can you take a last look at this. I think it is good to go 👍

@filipw filipw merged commit 5418aee into dotnet-script:master Mar 8, 2021
@filipw
Copy link
Member

filipw commented Mar 8, 2021

sorry for the delay - and thanks!

@mungojam
Copy link

I would guess that this fixes my issue too #540, but I probably won't get time to check

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

Successfully merging this pull request may close these issues.

None yet

4 participants