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

Compiler api harmonise #639

Merged
merged 4 commits into from
Oct 11, 2016
Merged

Compiler api harmonise #639

merged 4 commits into from
Oct 11, 2016

Conversation

7sharp9
Copy link
Member

@7sharp9 7sharp9 commented Sep 22, 2016

This PR addresses the fact the SimpleSourceCodeServices has compile specific API's that are not present in SourceCodeServices.

The benefits of this PR is that a single FSharpChecker can be created and used to do compile operations, rather than having to move between SimpleSourceCodeServices and SourceCodeServices.

@7sharp9
Copy link
Member Author

7sharp9 commented Sep 22, 2016

Fixes / Implements #516

Copy link
Contributor

@dsyme dsyme left a comment

Choose a reason for hiding this comment

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

Great work. Could you also please deprecate the methods in SimpeServices.fsi? And adjust any documentation (if there is any for these).

@@ -2697,6 +2807,66 @@ type FSharpChecker(projectCacheSize, keepAssemblyContents, keepAllBackgroundReso

member ic.TryGetRecentTypeCheckResultsForFile(filename, options, ?source) = ic.TryGetRecentCheckResultsForFile(filename,options,?source=source)

member ic.Compile(argv: string[]) =
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 these operations should all go via the backgroundCompiler and the Reactor thread - that's more obvious now that they are all part of the proper service API. Could you add that as part of this PR? Or we could do that as a separate item of work.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think a separate PR as that would be a change of functionality, Im not entirely sure on the interaction of the compile action with an ongoing background compilation. Maybe that in itself is a good reason for a reactor lock.

@7sharp9
Copy link
Member Author

7sharp9 commented Sep 23, 2016

Do we need to depreciate the compile methods in SimpleSourceCodeServices?

Maybe SimpleSourceCodeServices should be depreciated entirely? The only reason Ive ever used it was to use the CompileFromAsts method!

@dsyme
Copy link
Contributor

dsyme commented Sep 26, 2016

Maybe SimpleSourceCodeServices should be depreciated entirely? The only reason Ive ever used it was to use the CompileFromAsts method!

Yes, I believe it can now be deprecated (though please double check, maybe there's something dangling).

@7sharp9
Copy link
Member Author

7sharp9 commented Sep 26, 2016

Theres quite a few uses around on github, I think the bast bet is to mark it as depreciated for now, I can add that to this PR.

@dsyme
Copy link
Contributor

dsyme commented Sep 26, 2016

Theres quite a few uses around on github, I think the bast bet is to mark it as depreciated for now, I can add that to this PR.

Great, thanks

@dsyme
Copy link
Contributor

dsyme commented Sep 26, 2016

Here's the AppVeyor failure:

[00:05:40] C:\Program Files\dotnet\dotnet.exe compile-fsc @C:\projects\fsharp-compiler-service\src\fsharp\FSharp.Compiler.Service\obj\Release\netstandard1.6\dotnet-compile.rsp returned Exit Code 1
[00:05:40] C:\projects\fsharp-compiler-service\src\fsharp\vs\service.fs(2189,31): error FS0039: The namespace or module 'AssemblyBuilder' is not defined
[00:05:40] C:\projects\fsharp-compiler-service\src\fsharp\vs\service.fs(2190,29): error FS0072: Lookup on object of indeterminate type based on information prior to this program point. A type annotation may be needed prior to this program point to constrain the type of the object. This may allow the lookup to be resolved.

@7sharp9
Copy link
Member Author

7sharp9 commented Sep 26, 2016

Hmm, dotnetcore issue?

@7sharp9
Copy link
Member Author

7sharp9 commented Sep 26, 2016

At the moment I have to disable dotnetcore to get mono to compile. Maybe #637 needs a closer look to get the CI stable.

@dsyme
Copy link
Contributor

dsyme commented Sep 26, 2016

@7sharp9 Yes. Either a missing reference, or changed namespace, or need to #if out the functionality, or something.

@dsyme
Copy link
Contributor

dsyme commented Sep 26, 2016

@7sharp9 I think FX_NO_APP_DOMAINS is defined on .NET Core and you may need to open System.Reflection.Emit namespace to find AssemblyBuilder.

@7sharp9
Copy link
Member Author

7sharp9 commented Sep 28, 2016

@dsyme Appveyor is ok now.

@dsyme dsyme merged commit 2637859 into fsharp:master Oct 11, 2016
@dsyme
Copy link
Contributor

dsyme commented Oct 11, 2016

Thanks for this @7sharp9

@7sharp9
Copy link
Member Author

7sharp9 commented Oct 12, 2016

I'll do a further PR later to add the depreciate flag in simplesourcecodeservces and the reactor thread.

@dsyme
Copy link
Contributor

dsyme commented Oct 13, 2016

@7sharp9 Thanks. Let's aim to nuke SimpleSourceCodeServices and bring it all into FSharpChecker.

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.

2 participants