Feature/create and execute dll #315

Sharpiro · 2018-06-16T01:24:58Z

This feature continues with my previous publish to .exe PR #312 as well as Filip's POC e677cd6 to create and execute a .dll .

I made improvements to the way we were emitting the streams in ScriptEmitter:

Debug information is now embedded in the PeStream so we have 1 less file to worry about (and apparently I hear it might be more performant?).
If the publish option for -c release is supplied, then no debug information will be created.
It tuns out Filip was right and I did not need to actually rename the assembly to get the publishing working, so I removed that bit of code.

.dll generation considerations:

when the --dll flag is passed to the publish command, the script is outputted to a .dll in a publish directory in the working directory (configurable), along w/ any .dll's the script referenced via #r. Getting Nuget packages working was trickier. I elected to not move over the assembly files to the publish directory during a publish, instead moving 2 files from the "obj" folder, "project.assets.json", and "script.csproj.nuget.g.props" to determine the necessary nuget information when the generated .dll is executed. 1 caveat to doing this, is no nuget restore takes place at execution time. I figured this would be ok after I tested the dotnet test.dll command and saw that it does not restore nuget packages either. This does mean though that if you do a publish to dll, delete your nuget packages (or execute from a machine missing the packages), then try to execute the dll, you will get a missing reference error.
A dll is generated and named based upon the current working directory. This was not possible to do with .exe generation w/o some breaking changes.
After a dll has been generated, it can then be run through dotnet script via the exec command. This command calls the ScriptRunner, creates the dependency map from the obj information, runs the main script <Factory> method, and then resolves missing nuget references.

misc:

it seems d93a557 may have broken the Dotnet.Script.Desktop.Tests tests

Looking forward to your feedback. Thanks

release config now emits no debug info copying over obj info @ publish time for nuget package info

adding publish dll tests

filipw · 2018-06-19T12:14:35Z

src/Dotnet.Script/Program.cs

@@ -133,17 +133,20 @@ private static int Wain(string[] args)
                c.Description = "Creates an executable from a script";


please update the description here (or DLL)

filipw · 2018-06-19T12:15:22Z

src/Dotnet.Script/Program.cs

                var publishDebugMode = c.Option(DebugFlagShort + " | " + DebugFlagLong, "Enables debug output.", CommandOptionType.NoValue);
                c.OnExecute(() =>
                {
+                    var x = debugMode.HasValue();


what's x? 😀

I hate that visual studio didn't give a warning for this type of unused variable...

filipw · 2018-06-19T12:16:40Z

src/Dotnet.Script/Program.cs

@@ -133,17 +133,20 @@ private static int Wain(string[] args)
                c.Description = "Creates an executable from a script";
                var fileNameArgument = c.Argument("filename", "The script file name");
                var publishDirectoryOption = c.Option("-o |--output", "Directory where the published executable should be placed.  Defaults to a 'publish' folder in the current directory.", CommandOptionType.SingleValue);
+                var dllOption = c.Option("--dll", "Publish to a .dll instead of a .exe", CommandOptionType.NoValue);
+                var commandConfig = c.Option("-c | --configuration <configuration>", "Configuration to use for running the script [Release/Debug] Default is \"Debug\"", CommandOptionType.SingleValue);


why do we need an extra option instead of the already existing top-level var configuration = app.Option("-c | --configuration <configuration>"?

I'm glad you mentioned this. I tested trying to access "app" level "options" when executing a sub "command", and those options were not coming through and required me to create new variables in order to access them at the "command" level.

I am barely able to handle this argument parsing library 😀 so I now suspect this is happening because the publish is a command so it creates a new context and all the arguments are parsed within this context, not in the global one?

Yeah that has been the assumption I went with as well.

filipw · 2018-06-19T12:22:43Z

Thanks a lot, great work! 🍻

Getting Nuget packages working was trickier. I elected to not move over the assembly files to the publish directory during a publish, instead moving 2 files from the "obj" folder, "project.assets.json", and "script.csproj.nuget.g.props" to determine the necessary nuget information when the generated .dll is executed.

I think this has certain problems and we will have to change it - like you mentioned, if the packages are gone, or if you move to a different machine where they are not there, or if you move to a different OS (then they might be there, just in different place).
Also, technically speaking, what I think should happen, is that we should move the DLLs that are not part of the .NET Core SDK (runtime package store).

That said, I think this is a reasonable 1st version though, this is more or less how it worked in scriptcs in the past too, and we can describe it as known limitation for now, and improve later.

Also, the way I'd view compile to exe and compile to dll is that compile to exe is to really export to a self contained app that can run elsewhere. Compile to dll is like a local cache for your script so that you keep running on the same machine, but with better performance for subsequent executions.

If the publish option for -c release is supplied, then no debug information will be created.

This is not a bad idea. I want debug info to always be there for standard scripting scenarios, but once we deal with exe/dll I think it makes sense to respect the configuration.

A dll is generated and named based upon the current working directory. This was not possible to do with .exe generation w/o some breaking changes.

why not generate the name of the DLL from the entry CSX file? dotnet script publish foo.csx should create foo.dll? I don't think the name of the folder matters here.

Sharpiro · 2018-06-19T14:13:59Z

So for the generated DLL name, since the name of the script is usually less descriptive than the folder, I went with the folder. Using dotnet script init the intro script is always just "main.csx", whereas the folder might describe the script project. I don't have a strong opinion and can change this to just the script name if you'd like.

filipw · 2018-06-19T14:18:39Z

I think we should use the script name. Perhaps we can have another switch, something like -o | --output where we can override the name?

filipw · 2018-06-19T14:21:26Z

src/Dotnet.Script.Core/ScriptPublisher.cs

        {
            try
            {
-                var emitResult = _scriptEmitter.Emit<int>(context, AssemblyName);
+                var emitResult = _scriptEmitter.Emit<int>(context);


I think we can remove the PdbStream property from the emitResult?

Yea I already removed it since the debug information is embedded

filipw · 2018-06-19T14:25:20Z

src/Dotnet.Script.Core/ScriptEmitter.cs

@@ -16,30 +16,26 @@ public ScriptEmitter(ScriptConsole scriptConsole, ScriptCompiler scriptCompiler)
            _scriptCompiler = scriptCompiler;
        }

-        public virtual ScriptEmitResult Emit<TReturn>(ScriptContext context, string assemblyName = null)
+        public virtual ScriptEmitResult Emit<TReturn>(ScriptContext context)
        {
            try
            {
                var compilationContext = _scriptCompiler.CreateCompilationContext<TReturn, CommandLineScriptGlobals>(context);


I just realized that on other code paths (i.e. script execution) we support using custom host types. This is not very important but in the future the publish code should probably be extended with that

misc review changes

seesharper

Great work. LGTM

filipw

Sharpiro added 6 commits June 15, 2018 09:50

publish and execution of dll now functional

130a73e

converted to embedded pdb

dc15c7d

release config now emits no debug info copying over obj info @ publish time for nuget package info

no longer renaming assembly when publishing

aa9891c

fixed assembly resolve bug

b52344d

self-review

897e955

published dll now named after workspace directory

b6973b0

adding publish dll tests

seesharper requested review from seesharper and filipw June 19, 2018 09:05

filipw reviewed Jun 19, 2018

View reviewed changes

dll now named after script or 'name" option

2356370

misc review changes

seesharper approved these changes Jun 19, 2018

View reviewed changes

filipw approved these changes Jun 20, 2018

View reviewed changes

filipw merged commit 8e11235 into dotnet-script:master Jun 20, 2018

Sharpiro deleted the feature/create_and_execute_dll branch June 20, 2018 14:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Feature/create and execute dll #315

Feature/create and execute dll #315

Uh oh!

Sharpiro commented Jun 16, 2018

Uh oh!

filipw Jun 19, 2018 •

edited

Loading

Uh oh!

filipw Jun 19, 2018

Uh oh!

Sharpiro Jun 19, 2018

Uh oh!

filipw Jun 19, 2018

Uh oh!

Sharpiro Jun 19, 2018

Uh oh!

filipw Jun 19, 2018

Uh oh!

Sharpiro Jun 19, 2018

Uh oh!

filipw commented Jun 19, 2018

Uh oh!

Sharpiro commented Jun 19, 2018

Uh oh!

filipw commented Jun 19, 2018

Uh oh!

filipw Jun 19, 2018

Uh oh!

Sharpiro Jun 19, 2018

Uh oh!

filipw Jun 19, 2018

Uh oh!

seesharper left a comment

Uh oh!

filipw left a comment

Uh oh!

Uh oh!

		@@ -133,17 +133,20 @@ private static int Wain(string[] args)
		c.Description = "Creates an executable from a script";

Feature/create and execute dll #315

Feature/create and execute dll #315

Uh oh!

Conversation

Sharpiro commented Jun 16, 2018

Uh oh!

filipw Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filipw commented Jun 19, 2018

Uh oh!

Sharpiro commented Jun 19, 2018

Uh oh!

filipw commented Jun 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seesharper left a comment

Choose a reason for hiding this comment

Uh oh!

filipw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

filipw Jun 19, 2018 •

edited

Loading