Upgrade dependencies #2632
Closed
Upgrade dependencies #2632
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@matthid can you please take a look at this? |
matthid
approved these changes
Jan 11, 2022
| @@ -5,86 +5,6 @@ open System.Threading | |||
| open System.Threading.Tasks | |||
|
|
|||
| module ExpectoHelpers = | |||
|
|
|||
| let inline internal commaString (i:int) = i.ToString("#,##0") | |||
| // because of https://github.com/haf/expecto/issues/367 | |||
There was a problem hiding this comment.
Just FYI, if everything works. OK, but I doubt that this issue has been solved. So if you ever encounter a build 'hanging', this might be the root cause. (I'd leave it as is and see if bad things happen)
There was a problem hiding this comment.
For Expecto, I read the issue that has caused the inlining of the Expecto printer in FAKE. But as I understand this was caused in Travis-CI builds, and now we are using GitHub for everything and didn't encounter it till now 😅. So, we can try and see. If anything hangs I'll inline the printer as before.
There was a problem hiding this comment.
@matthid It seems that Expecto tests are failing on the release/next branch after the upgrade to .Net 6.0.101 from the preview version. In the PR, the build succeeded but now Expecto gives timeout on unit tests. We are still using the custom printer on the release/next branch. I tried to increase the timeout to 30 minutes and re-run the build multiple times but no luck. I'll investigate more on this, not sure if the update to Expecto 9 and the removal of the custom printer would solve it which are done in this PR. But we will not know that until this PR got fixed.
I just wanted to give an update on this. Will try to look more and see what I got.
Thanks
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?
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 :) |
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
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:
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
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;
After the exit point and run ends, the following is the content:
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 :) |
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.
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
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.
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?
|
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... |
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. |
|
After the upgrade of MissingMethodException: Method not found: 'Void Microsoft.FSharp.Core.PrintfFormat`5..ctor(System.String)'.)FAKE runner treats From the notes in the code, it seems that loading multiple versions of 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 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 Now to the issue at hand: First we need to check in what Generally, this issue shouldn't occur because of
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 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 |
|
@matthid Thanks for all the details. I'll follow your notes and see what is causing the issue and try the options you listed. |
|
Still cannot find why this is happening. I tried to disable the current design limitation in which the #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 |
|
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.
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; This is the output I got from the run against Netstandard2.0 caseThe Following is the decompiled assembly IL code for |
… they use prerelease version.
…ting script directory.
…embly by compiler.
yazeedobaid
force-pushed
the
upgrade-dependencies
branch
from
d6f77eb
to
ccfeffb
Compare
|
Can you also run with -vv (I can't remember how many levels we have) |
This is the run with -vv for the Netstandard test |
|
The relevant F#-Core logs: 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) |
|
|
|
Ah sorry I looked into release/next branch :(. |
|
Yeah sure,
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: 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. |
|
Closing this PR in favor of work on FAKE v6. This PR has been merged to |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR organizes and updates FAKE dependencies to the latest versions. The changes are:
paket.dependenciesfilenetcore->fakemoduleused by all FAKE modules projectsnetcorerunner->fakerunnerused by FAKE Runner projectsMove theDependencies need to be in script since they use re-release versions from release dirbuild.fsxdependencies topaket.dependenciesfile under Build groupStartBootstrapBuildfrom build script since it is not used anymoreFSharp.Compiler.Servicebreaking 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.dependenciesFake.Core.IntegrationTests.issue #2007 - native libs workFake.Core.IntegrationTests.issue #2025Fake.DotNet.sdkAssemblyResolverTests.Runner run script with 6.0.100 SDK version assembliesFake.DotNet.sdkAssemblyResolverTests.Runner run script with 6.0.100-preview.3.21202.5 SDK version assembliesFake.DotNet.sdkAssemblyResolverTests.Runner run script with NETStandard2.0 SDK assembliesAll 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.fsxgot 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 the
AppContextOnExitProcessevent. 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)
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)Fake 5 API guideline is honored