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

add functionality for writing compilation output to a Stream #44

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

add functionality for writing compilation output to a Stream #44

wants to merge 2 commits into from

Conversation

h3x4d3c1m4l
Copy link

fixes issue #42

@h3x4d3c1m4l
Copy link
Author

h3x4d3c1m4l commented Oct 13, 2019

I'm not entirely sure about the API yet. This PR has minimal consequences to the API but can be a littie confusing...

I would like to argue for non-async counterparts but that would make the code a little more confusing to maintain. Perhaps all Compile methods should just be named something like is? This makes more use of overloading. So you would have something like:

  • CompileSourcesAsync(string assemblyName, params string[] sources) (won't produce output, just writes to Stream.Null)
  • CompileSourcesAsync(string assemblyName, Stream stream, params string[] sources)
  • CompileSourcesAsync(string assemblyName, string outputPath, params string[] sources)
  • CompileFilesAsync(string assemblyName, Stream stream, params string[] sourceFilePaths)
  • CompileFilesAsync(string assemblyName, string outputPath, params string[] sourceFilePaths)

And possibly their non-async/sync counterparts

@MilleBo
Copy link
Contributor

MilleBo commented Oct 13, 2019

Yeah I guess the API could use some streamlining here. I like your suggestions - it's a bit harder to grab by just looking at the methods but it's more clean so it should work with proper documentation.

You wanna try to do the changes in this PR? If not I will try to make the changes next week.

@h3x4d3c1m4l
Copy link
Author

h3x4d3c1m4l commented Oct 13, 2019

While trying to get compilation working in my Blazor project I came across another compiler issue.
The MetadataReferences are automatically created by providing the compiler with a runtime directory, but this does not work on Blazor (WebAsssembly runtime) as there is no filesystem. Roslyn accepts Stream too instead, but I'm not yet sure how to expose in the API of Testura.Code.

I think for normal frameworks it should by default reference all loaded assemblies.

In the current situation, if I try to compile a simple file on .NET Core without specifying the runtimeDirectory it will fail because it will try to compile using the NF 4.5.1 assemblies.

This default wouldn't work for Blazor/WASM projects, for these to work there need to be a possibility to add a MetadataReference by Stream.

Any ideas on this?

@MilleBo
Copy link
Contributor

MilleBo commented Oct 14, 2019

I see, maybe we should split it up to a separate Compiler class than? We could add an additional constructor that require the stream you need but it feels like it would get messy after a while.

But I have two different ideas for now:

  • A new abstract base class with most of the logic of Compiler.cs
  • Two new classes - One compiler that work with references from FileSystem and one from Steam?

Or:

  • Change Constructor.cs so it takes a ICSharpCompilationProvider (or something) instead of referencedAssemblies and runtimeDirectory. Then it's easy to provide different configurations of CSharpCompilation and it's also possible for others to extend with their own if needed. Then we could even keep the current constructor but just make it initialize one of the CSharpCompilationProviders (so we don't break the API).

Maybe the second option is better but it's just some early ideas.

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