Closed yazeedobaid closed 2 years ago
@matthid can you please take a look at this?
@yazeedobaid
The second difference is the AppContext OnExitProcess event. The new FSharp.DependencyManager.Nuget has an event that is registered on exit to delete the script directory, Please see this got triggered from another thread after the main thread exist and cause the files to be deleted.
I fail to see why this would be problematic. It is only related to a temporary directory the same module created and it shouldn't interfere with our code?
I have debugged the issue, compare the run from this branch and release/next, and found the following; An unauthorized access error is thrown when running the script, but this doesn't affect the script run, but it started to happen on this branch after the upgrade.
After searching for my old comment and looking at this place It seems like we write the intellisense file in parallel and always. Does your debugger say you get an unauthorized access error from this background thread trying to write the file? I can't think of any reason why that would fail with an access error. Maybe it helps to synchronize the task temporarily to debug the issue more easily (but iirc that will slow down FAKE significantly, it is only a temporary debugging approach)
Hope this helps somewhat :)
@matthid
I fail to see why this would be problematic. It is only related to a temporary directory the same module created and it shouldn't interfere with our code?
Yes, this is what I cannot understand why! The debugger led me to this point and this is the only place that deletes stuff. The workingDirectory
that the deleteScript
is trying to delete is the script FAKE directory; .fake/test.fsx
in my case.
Does your debugger say you get an unauthorized access error from this background thread trying to write the file?
The unauthorized access is on the delete of the script assembly not on the write of the IntelliSense file. Here is the exception message I got:
System.UnauthorizedAccessException: Access to the path 'test_56C5C301F0A564985414718182BEBBAE8E75A97D03826DE3FDFCFD85F11A002D.dll' is denied. at System.IO.FileSystem.RemoveDirectoryRecursive(String fullPath, WIN32_FIND_DATA& findData, Boolean topLevel) at System.IO.FileSystem.RemoveDirectory(String fullPath, Boolean recursive) at System.IO.Directory.Delete(String path, Boolean recursive) at FSharp.DependencyManager.Nuget.FSharpDependencyManager.deleteScripts() in D:\a_work\1\s\src\fsharp\FSharp.DependencyManager.Nuget\FSharp.DependencyManager.fs:line 191
I think maybe I got this unauthorized access because the main thread is still using the script assembly somehow, and when this thread tries to delete it, it fails, because it is still used, so if we didn't get this error, the whole directory would be deleted. Not sure if this is a correct analysis for the issue, but this is what I currently can think of.
This issue of deleting the files has occurred with me before as you pointed out in the upgrade of Paket.Core
. Back then I managed to skip it by selectively pinning and upgrading the required dependencies for the new version of Paket.Core
. But we cannot do it again since we need to free the dependencies from specific versions, to make it easy for future updates for the dependencies.
Maybe it helps to synchronize the task temporarily to debug the issue more easily
I'll try that and see what I got. Please note that all the files in the script FAKE directory got created correctly and written to disk. Prior to the exit point of the runner, they are all in the directory, but after that, they are deleted except the script assembly.
For reference, prior to exit point of FAKE runner, following is the content of the script directory; .fake/test.fsx
in my case:
.paket/Paket.Restore.targets
paket-files/.gitignore
paket-files/paket.restore.cached
dependencies.cached
dependencies
fake-hash
fake-hash-contents
fake-hash-files
fake-section.cached
fake-section
intellisense
intellisense_lazy
paket.dependencies
paket.lock
test_56C5C301F0A564985414718182BEBBAE8E75A97D03826DE3FDFCFD85F11A002D.dll
test_56C5C301F0A564985414718182BEBBAE8E75A97D03826DE3FDFCFD85F11A002D.pdb
test_56C5C301F0A564985414718182BEBBAE8E75A97D03826DE3FDFCFD85F11A002D.warnings
After the exit point and run ends, the following is the content:
test_56C5C301F0A564985414718182BEBBAE8E75A97D03826DE3FDFCFD85F11A002D.dll
Thanks
Ah thanks, now I see, so the directory variable is set to our directory https://github.com/dotnet/fsharp/blob/20693815ee19e5a1e8f07efbd555b765e6980170/src/fsharp/FSharp.DependencyManager.Nuget/FSharp.DependencyManager.fs#L187
I'm pretty sure the parameter of this function is the output path we give to the compiler which is the directory in question. So I have several ideas what might be going on:
I'm not sure what the easiest way is to set a breakpoint into this specific location of the compiler, maybe we should open an issue there and ask the gurus :)
I'm pretty sure the parameter of this function is the output path we give to the compiler which is the directory in question
Yes, this is exactly what is happening, I found this issue, and this pull request on the compiler repository for the case. So, it was deleting the directory. The workaround suggested in the issue to use relative paths has worked for the integration tests, now nothing got deleted and no unauthorized exception is thrown as well.
It might be that the code we see on GitHub is already a fixed version without the issue...
I compared the generated code by the de-compiler on my machine and one in this PR and they are different. So, yes the fix in this PR is still not released yet. However, the new code seems to be an improvement to the previous one, but also still deletes the directory. And the workaround of using relative paths is the one still used to overcome the issue of deleting the script directory.
So, using relative paths will make the dependency manager use a temp directory as in this line other than the ones FAKE uses.
Does the use of a relative path for the script DLL affect FAKE in any way? Also, not sure what the dependency manager is generating in that temp directory, but could it affect FAKE, from the performance side maybe?
I have another issue caused by an upgrade for integration tests running using NetStandard2.0 SDK reference assemblies. Will look and see what is causing it.
Nice find, incredible that such a bug went into the release :D
Does the use of a relative path for the script DLL affect FAKE in any way?
Not directly, but it can be a bit difficult to calculate the correct relative directory. Note that there are 3 relevant directorylies, one where fake.exe lives, one where the script lives and the current directory which should not be changed by fake (as that wound be unexpected). All 3 can be different or even be on different drives! We write the cache alongside the script directory iirc, this is to reuse the cache no matter the working directory. So I'd not try to calculate a relative path as it is not trivial and potentially error prone, but it is probably doable.
Also, not sure what the dependency manager is generating in that temp directory, but could it affect FAKE, from the performance side maybe?
I don't think it will (besides the bugs like the one you encountered here :D), we have our own files, as long as it doesn't interfere with our files it should be fine. It might even be a good thing if this package manager thing creates its own reusable cache in the same place
I did a test and run the script from another directory using FAKE as a global tool and indeed it wrote the assembly in another place. And I cannot determine where! All the other files are in the script directory except the script assembly and program debug database (.pdb)
So, yeah it is not easy to set/use a relative path!
Do you have any suggestions on what the next steps should be? Or should I go with opening an issue in the compiler repository as you suggested?
maybe we should open an issue there and ask the gurus :)
One workaround might be to tell the compiler to built into a temporary path and copy our stuff (instead of building directly into the directory) so the f# compiler doesn't delete pur directory. Obviously we first want to know what's going on and report a bug to the compiler...
Is this not an option? After building we copy/move the dll and the compiler can delete the directory for us (or we already delete it to be future proof). Question is if the old version chokes when the directory no longer exists, if yes we can leave the empty directory until we have a fix...
One workaround might be to tell the compiler to built into a temporary path and copy our stuff (instead of building directly into the directory) so the f# compiler doesn't delete pur directory. Obviously we first want to know what's going on and report a bug to the compiler...
Yes, it worked, I added a temporary directory to the compiler output path argument and after a successful compilation the .DLL and .PDB file moved to script directory. After exit, the compiler can delete the temporary directory and no unauthorized exception is thrown anymore.
I have added an assertion in integration tests to assert that DLL exists in the script directory. Also, I tested it as a global tool, and all was good.
After the upgrade of FSharp.Core
to v6.0 and FSharp.Compiler.Service
to v41.0.0, it seems that the FAKE runner that uses NetStandard.Library
v2.0 reference assemblies is broken. The issue is that when running the script the FSharp.Core
assembly cannot be found hence the error in the build:
MissingMethodException: Method not found: 'Void Microsoft.FSharp.Core.PrintfFormat`5..ctor(System.String)'.)
FAKE runner treats FSharp.Core
in a special way that it always uses the one that is shipped with it, hence it will use the v6.0. And that seems to not work as expected. Searching about the issue and it seems that FSharp.Core
v6.0 now targets both netstandard2.0
and netstandard2.1
. Please see this comment from release notes. So, the FSharp.Core
that targets netstandard2.0
is the one that needs to be used when using the NetStandard.Library
v2.0 reference assemblies. However, I tried to do that but with no luck, I got the same error!
From the notes in the code, it seems that loading multiple versions of FSharp.Core
in memory is problematic, hence the special treatment to use the one that is shipped with the FAKE runner and the one that is already loaded, netstandard2.1
.
This is my current analysis of the issue, can anyone please advise on this? Is my analysis correct, and do we have a workaround for it?
Thanks
Regarding FSharp.Core
, I can say a couple of things. Generally, we don't really have a "hard" dependency between the "Runtime" and the "Script Environment", besides the compiler itself (IIRC sometimes the compiler has a implicit requirement to have at least a specific versions when compiling specific code with a specific compiler version. For example: I think you cannot compile with the latest compiler a program depending on an ancient FSharp.Core
version, but I might be wrong here).
Regarding FAKE itself, the two environments (Fake Runtime and Script Runtime) are connected by a couple of primitives to forward settings and arguments. This interop layer currently uses F# Lists, that is why I decided to force the Script runtime to use the same FSharp.Core
version... However, that design decision is not technically required, so we can review that if we can get rid of F# types (if required for the issue you encountered here).
Now to the issue at hand: First we need to check in what AssemblyLoadContext
the issue occurs (I assume this happens in the Script Environment). We should also check which versions of FSharp.Core
are involved in that particular scenario (Which one was the compiler call with, which one was tried to be loaded, which one did we actually load). Then we can see with ilspy or dotpeek whe the MissingMethodException
is happening (this happens usually because at compilation the method was found but at runtime it was not).
Generally, this issue shouldn't occur because of FSharp.Core
upgrades (because FSharp.Core
doesn't do binary breaking changes). So this either slipped through and needs to be fixed in FSharp.Core
and we should report an issue. Or this happens - as you already assumed - because we changed the TFM, (for example from netstandard2.1 to netstandard2.0). If this is in fact the reason I see two options:
So the difference between the first two options: In the first we ensure ourself that interop still works and change our current strategy to forward data to the script LoadContext and the second options depends on a particular compatibility of FSharp.Core
.
Sorry for the wall of text, when this was designed we had issues like this for every dependency (in previous FAKE versions). At the time I thought because FSharp.Core
is binary backwards compatible, as long as we restrict the script runtime FSharp.Core
-version nothing bad should happen, obviously I haven't considered that scenario with different TFMs because I locked that to a particular one ('netstandard20`)
@matthid Thanks for all the details. I'll follow your notes and see what is causing the issue and try the options you listed. Thanks again
Still cannot find why this is happening. I tried to disable the current design limitation in which the FSharp.Core
that ships with FAKE is the one that is used. Also, hard loading the Netstandard2.0 release of FSharp.Core
.But got the same issue. What I found instead is that, if we used another construct/types/methods from FSharp.Core
then the test passes by using Netstandard reference assemblies.
For example, if the script for Netstandard reference assemblies were replaced by the following script:
#r "paket:
storage: none
source https://api.nuget.org/v3/index.json
source ../../../release/dotnetcore
nuget Fake.Runtime prerelease
nuget FSharp.Core 4.7.2"
open Fake.Runtime
open System
//printfn "Starting Build."
//Trace.traceFAKE "Some Info from FAKE"
//printfn "Ending Build."
let ss = [|4;5|]
let x = 5
let opt = Some("this is option")
Console.WriteLine(x)
Console.WriteLine(ss)
Console.WriteLine(opt.Value)
Which just replaces printfn
calls with Console.WriteLine
and uses some other construct from FSharp.Core
, like not
function and option
. The script compiles and runs successfully without any issues.
Not sure if it is an issue with printfn
method. But what I found is that a new enhancement has been added to this method in F# 6 releases as described in this page.
Not sure if this has anything to do with the issue we have in FAKE.
Hm, sounds strange. Don't get me wrong it is a bit of detective work, but I have debugged several instances of this error in the past. And the error always was that different signatures were used on compilation- vs run-time. The root cause why this happens is often different. And it should be clearly visible in any decompiler (ilspy, dotpeet). Can you reproduce this case, run in verbose mode and
Maybe I can help identify what is going on.
Sorry I didn't get the chance to send the information earlier. But this is what I got;
The FSharp.Core
used has version 6.0.121.52202 and it's path on disk from Paket graph is C:\Users\YazeedObaid\.nuget\packages\fsharp.core\6.0.1\lib\netstandard2.1
Following is the decompiled assembly IL code for printfn
Can you also run with -vv (I can't remember how many levels we have)
The relevant F#-Core logs:
- lib: C:\Users\YazeedObaid\.nuget\packages\fsharp.core\6.0.1\lib\netstandard2.1\FSharp.Core.dll (6.0.0.0)
- ref: C:\Users\YazeedObaid\.nuget\packages\fsharp.core\6.0.1\lib\netstandard2.1\FSharp.Core.dll (6.0.0.0)
"-r:C:\Code\ExternalLibraries\FAKE\release\dotnetcore\Fake.netcore\current\FSharp.Core.dll";
Trying to resolve: FSharp.Core, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a
Redirect assembly load to known assembly: FSharp.Core, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a (None)
This is indeed a bit strange, because it looks like we use the same assembly when compiling and when running. However it seems like we use an ancient FSharp.Compiler.Service. (https://www.nuget.org/packages/FSharp.Compiler.Service/37.0.0) It was built against quite an old FSharp.Core (4.6), I'm not sure if that compatibility direction is officially supported. I'd suggest upgrading Fsharp.Compiler.Service and if that doesn't help try to reproduce this in a small standalone sample and report an issue to the FSharp.Compiler...
FSharp.Compiler.Service
has been updated to version 41.0.1 in this PR. The only references to an older version are due to FSharp.Formatting
. I removed them and tried but still the same. I'm looking at paket.dependencies
and paket.lock
files. is this the same reference you saw?
Ah sorry I looked into release/next branch :(. To be honest I have no further Ideas it really looks like the compiler compiles something with the assembly which is later not runnable. Can you attach the dlls (C:\Code\ExternalLibraries\FAKE\release\dotnetcore\Fake.netcore\current\FSharp.Core.dll, and the built fsx.dll)? Then I can take a look with the decompiler, but honestly I don't expect to find anything. I guess we need to reproduce and report an issue?
Yeah sure,
I guess we need to reproduce and report an issue?
Yeah, sure we can report this to the compiler. We can attach the packed runner (from this branch) and add a script that references that runner version.
However, I'm somehow hesitant to spend more time on it, we could go with the easy solution and drop the support for Netstandard2.0 reference assemblies. My argument for this is that some of the reported issues are a mix between what FAKE runner use as reference assemblies and version script is written in. In most cases, people write the script with Net6 in mind and FAKE runner end up using Netstandard2.0 assemblies. Now this has the downside of making the runner require Net6 as a minimum
That would be an option as well, if the change is breaking we can consider increasing the major version
Great, will try to work toward that. I'm also thinking of fixing all the warnings from API deprecations. Which will also produce a breaking change. So, we can add it when releasing FAKE v6. Do you have any other things to consider while doing that?
@yazeedobaid Historically, I created a todo list as "github project" (https://github.com/fsprojects/FAKE/projects/2), But I don't think it's very up to date or a lot of work so feel free to delete them :D
BTW: I also looked at the .dlls. Indeed a very interesting issue because that particular overload has been in FSharp.Core since forever:
Which is exactly the method in the error message: MissingMethodException: Method not found: 'Void Microsoft.FSharp.Core.PrintfFormat`5..ctor(System.String)'.
I highly doubt there exists any version of the dll where this particular method is actually missing so indeed something very strange is going on. I might take a look at this on the weekend out of curiosity, but don't wait for it and feel free to update as planned.
Closing this PR in favor of work on FAKE v6. This PR has been merged to release/v6
branch and from it, we will continue working on the next major version of FAKE
Description
This PR organizes and updates FAKE dependencies to the latest versions. The changes are:
paket.dependencies
filenetcore
->fakemodule
used by all FAKE modules projectsnetcorerunner
->fakerunner
used by FAKE Runner projectsMove theDependencies need to be in script since they use re-release versions from release dirbuild.fsx
dependencies topaket.dependencies
file under Build groupStartBootstrapBuild
from build script since it is not used anymoreFSharp.Compiler.Service
breaking changes. Mostly include modules renamingI have encountered an issue in integration tests that I cannot know why it is happening. The integration tests that fail are:
Fake.Core.IntegrationTests.use external paket.dependencies
Fake.Core.IntegrationTests.issue #2007 - native libs work
Fake.Core.IntegrationTests.issue #2025
Fake.DotNet.sdkAssemblyResolverTests.Runner run script with 6.0.100 SDK version assemblies
Fake.DotNet.sdkAssemblyResolverTests.Runner run script with 6.0.100-preview.3.21202.5 SDK version assemblies
Fake.DotNet.sdkAssemblyResolverTests.Runner run script with NETStandard2.0 SDK assemblies
All due to the same error:
Expect intellisense.fsx to exist. Actual value was false but had expected it to be true.
What I found is that, for these tests the generated files from the FAKE runner in script directory
.fake/scriptName.fsx
got deleted after the runner exited. They all got generated correctly and the script runs without any issues, but after the exit point, they are automatically deleted.I have debugged the issue, compare the run from this branch and
release/next
, and found the following; An unauthorized access error is thrown when running the script, but this doesn't affect the script run, but it started to happen on this branch after the upgrade. The second difference is theAppContext
OnExitProcess
event. The new FSharp.DependencyManager.Nuget has an event that is registered on exit to delete the script directory, Please see this got triggered from another thread after the main thread exist and cause the files to be deleted.To see this error, the CLR exception type needs to be active in the debugger.
Another note is that other integration tests which have the same structure passes and files are not deleted.
Can anyone help in this regard?
TODO
Feel free to open the PR and ask for help
[ ] New (API-)documentation for new features exist (Note: API-docs are enough, additional docs are in
help/markdown
)[ ] unit or integration test exists (or short reasoning why it doesn't make sense)
[x] boy scout rule: "leave the code behind in a better state than you found it" (fix warnings, obsolete members or code-style in the places you worked in)
[ ] (if new module) the module has been linked from the "Modules" menu, edit
help/templates/template.cshtml
, linking to the API-reference is fine.[ ] (if new module) the module is in the correct namespace
[ ] (if new module) the module is added to Fake.sln (
dotnet sln Fake.sln add src/app/Fake.*/Fake.*.fsproj
)[x] Fake 5 API guideline is honored