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

Adding Env.ScriptAssembly #1197

Merged
merged 1 commit into from Jan 12, 2017
Merged

Adding Env.ScriptAssembly #1197

merged 1 commit into from Jan 12, 2017

Conversation

glennblock
Copy link
Contributor

@glennblock glennblock commented Jan 12, 2017

This feature adds a new Env.ScriptAssembly and fixes #244. The assembly is accessible in scripts or in the REPL.

@glennblock
Copy link
Contributor Author

@scriptcs/core please review. This is ready to go. I'll merge tomorrow unless I hear back.

@filipw
Copy link
Member

filipw commented Jan 12, 2017

I would advise not to inject custom code into the script itself.
You don't need to emit a fake class to make this work, this would work too:

Env.SetScriptAssembly(System.Reflection.MethodBase.GetCurrentMethod().DeclaringType.Assembly);

However, you don't need to inject anything into the script code or have a set method on the env for assembly at all, you could just add this to the ScriptEnv:

public Assembly ScriptAssembly {
	get {
		return AppDomain.CurrentDomain.GetAssemblies().FirstOrDefault(x => x.FullName.StartsWith == "");
	}
}

Finally, I think the proper way to do it, is to find the assembly up front - at compile time, before execution of the script. Right now in typical scenarios we don't distinguish compile and run steps which we could easily do.

This has great benefits, as it allows us to discover the Assembly before script is executed - but as soon as it's compiled, which can be of great benefit in hosting scenarios, are you scan your script then for types or methods, before the script runs.

I have written that sample code already before. it's here https://github.com/filipw/dotnet-script/blob/194ce42b357c0e6dbbc7647792228652c691f248/src/Dotnet.Script/ScriptStateExtensions.cs#L12

@glennblock glennblock changed the title Adding Env.ScriptAssembly, a beautiful hack Adding Env.ScriptAssembly Jan 12, 2017
@glennblock
Copy link
Contributor Author

@filipw check the latest. I changed it now just use Assembly.CallingAssembly within the ScriptAssembly property. A lot cheaper, and no more injection.

Going to rebase before merging.

@glennblock
Copy link
Contributor Author

Rebased, ready to go.

@glennblock
Copy link
Contributor Author

This new version is MUCH simpler......

@filipw
Copy link
Member

filipw commented Jan 12, 2017

you are right - this is much better. But it also begs the question why do it all, if the user can just call Assembly.GetCallingAssembly() in the script 😄

Anyway, let's punch this in, but ultimately I think we need a solution like this https://github.com/filipw/dotnet-script/blob/194ce42b357c0e6dbbc7647792228652c691f248/src/Dotnet.Script/ScriptStateExtensions.cs#L12 because it gives us the assembly instance before script is executed, rather than only after, which is very valuable.

@filipw filipw merged commit c73d465 into scriptcs:dev Jan 12, 2017
@glennblock
Copy link
Contributor Author

@filipw let's revisit it. As I said, my biggest concern is the cost of iterating through all the assemblies to find "the one".

"But it also begs the question why do it all, if the user can just call Assembly.GetCallingAssembly() in the script"

And this is why we have code reviews :-)

The one disadvantage of this approach (relating to your question) is Assembly.GetCallingAssembly() will only work if it is called from the script or a script library. It won't work if somehow an external assembly invoked it.

My first approach of injecting code solved that, because property caches the result after the first invocation, and because I was invoking it before any other code, it was guaranteed to be called the first time within the script itself.

With the new approach that's not the case. If someone outside the script programmatically got a hold of the IScriptEnvironment and called it, it would not work right (currently).

Now your suggested approach also works, and as you said it happens during compilation. The one downside I see to this is the cost of iterating through assemblies in the App Domain. I haven't profiled this, but because it will grow linearly with the number of assemblies, that concerns me. Do you disagree?

Also why all the concern about code injection? After all, this is how the C# compiler itself works :-)

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.

Expose a reference to the current script assembly
2 participants