Some questions about 4.0.2

[marcocecchiscmgroup]

Hello Oleg,

remember about the touchscreen issue and the need to offer a way to exclude assemblies? Well, other than patching the pre .NET 5 version, I've been looking at the latest Cs-script version as well, as we have just started evaluating the migration of our whole project.
Here a few suggestions:

1. RefernceDomainAsemblies is mispelled -> ReferenceDomainAssemblies
2. The new method, SetRefAssemblyFilter(), could it be renamed/refactored into a more convenient BlacklistAssemblies(IEnumerable<string>) and BlacklistAssemblies(IEnumerable<Assembly>) (both signatures should be exposed).
3. The client/server architecture is maybe a whole new design? In case, would you mind defaulting CompileOnServer to false? I had a hard time catching compilation errors that were actually due to socket communication errors
4. The Microsoft nuget default server is currently publishing version 4.0.1. When are you planning to upload 4.0.2?

Thanks in advance,
Marco Cecchi

[oleg-shilo]

It is an interesting set of enhancements. So let's go through them:

1. "Misspell" - fixed
2. "Blacklisting"
   It's not that simple. First of all you are not proposing renaming but a change of the signature. However SetRefAssemblyFilter is the most versatile API for filtering. It lets devs to interact directly with the ref assemblies collections. The API you are proposing are less flexible forms of this API that the change would represent a regression not an improvement.
   
   In addition to that one of the signatures accepts IEnumerable<string>. This is quite ambiguous. What are these strings? Assembly files? Assembly names? Assembly partial names? I hope you see my point. The any public API should only contain generic methods that are not domain specific and unambiguous.
   This means that if you need this functionality you can easily implement it for your project with the extension methods:

   static class Extensions
   {
       public static IEvaluator BlacklistAssemblies(this IEvaluator evaluator, IEnumerable<string> forbiddenAssemblies)
            => evaluator.SetRefAssemblyFilter(
                             asms => asms.Where(a => !forbiddenAssemblies.Contains(a.FullName)));
   
       public static IEvaluator BlacklistAssemblies(this IEvaluator evaluator, IEnumerable<Assembly> forbiddenAssemblies)
            => evaluator.SetRefAssemblyFilter(
                             asms => asms.Where(a => !forbiddenAssemblies.Contains(a)));
   }

   Nevertheless I liked the idea of excluding by IEnumerable<Assembly> so the next release will deliver these extension metods to the core IEvaluator API:

   IEvaluator ExcludeReferencedAssemblies(this IEvaluator evaluator, params Assembly[] excludedAssemblies);
   IEvaluator ExcludeReferencedAssemblies(this IEvaluator evaluator, IEnumerable<Assembly> excludedAssemblies);

3. "defaulting CompileOnServer to false"
   It will default to the less performant behavior so it is a bit difficult to agree on but I am open to consider it.
   But a more intriguing question is why it was difficult for you as a user to troubleshoot this problem. This can definitely be improved.
   Can you please share where the exception was thrown and what exception type? The script engine should catch it, rethrow as custom exception and provide the accurate recommendation for fixing (e.g. "You can try to set CompileOnServer to false")
4. "publishing nuget 4.0.2"
   4.0.2 contains no changes to the CSScripLib class library but to CLI only. Thus such a nuget package would be an absolute equivalent of the v4.0.1 package. I only published the v4.0.2 package on GitHub because it was prepared by the CI. But I can remove it if it creates any confusion.
   But with the enhancements triggered by you the next release will publish a new nuget package on nuget.org too.

[marcocecchiscmgroup]

Hello Oleg,

thanks for considering my suggestions.

About point 2, 'blacklisting'. Yes, I do see your criticism about the ambiguity of the 'by string' signature. However, while the one 'by Assembly' is convenient to be offered for completeness in any case (in fact I was suggesting it myself), it is also true that in most cases one wants to be able to filter before loading - that is when a 'Assembly' handle is not available (and it must not) - right because of the side effects that can be triggered (see the original touchscreen issue). So if string is too generic something else should be made available.

About 3, I see that the client/server architecture is meant to be promoted by the latest releases. Our issues were like follows.
First off, a new person that tries out the new version knows typically nothing about this big change. Stupid question: how can it work if I don't have any server (exe) up&running? In some way it does, problem is that on my PC it worked straightaway, while on my colleague's one not. So after narrowing it down, I saw that it was a client/server miscommunication, so that I set CompileOnServer to false and that worked.
The problem he was(is) experiencing is easily replicated by the unit tests in your solution in API_CodeDom.Check_ValidScript and following tests.

EvaluatorTests.API_CodeDom.Check_ValidScript
Source: Evaluator.Api.Test.cs line 144
Duration: 215 ms
Message:
CSScriptLib.CompilerException : C:\Users\jrossi\AppData\Local\Temp\CSSCRIPT\19532.65e94c70-cb1e-4089-80e4-bbfce5eb2a5a.tmp(0,0): error CS0006: Il file di metadati 'C:\Program Files\d' non è stato trovato
Stack Trace:
CodeDomEvaluator.CompileAssemblyFromFileBatch_with_Csc(String[] fileNames, String[] ReferencedAssemblies, String outAssembly, Boolean IncludeDebu[...]

With the path
'C:\Program Files\d' in the error message being truncated randomly at various lengths (it is supposed to be some system dll at 'C:\Program Files\dotnet\shared\Microsoft.NetCoreApp\5.0.6...'

If you need further details, we are still able to reproduce it. One more problem that made it difficult to find it was that the behaviour in debug mode or in usual run mode was different. In debug mode the server didn't even respond, while in run mode it responded with the above error.

4. I'm afraid 4.0.1 on nuget.org misses the SetRefAssemlyFiler() method. In case, just try and push version 4.0.2 on nuget.org as soo as you can.

Thanks.

[marcocecchiscmgroup]

Ah well, I was forgetting another important issue.
Given the latest feature in 4.0.2 to unload assemblies such as:

calc.GetType()
.Assembly
.Unload();

can you confirm that this works with CompileOnServer=false as well? In what domain are the assemblies loaded? Is this automagically done in a sandboxed appdomain?

[oleg-shilo]

My turn now :)

2. I am not sure I follow. If you wan to prevent referencing assembly that is not loaded yet then ... just do not add it as a reference. Am I missing something?
   I do see how the assemblies that are already loaded in the AppDomain can interfere with assembly probing and referencing but in this case SetFilter works just fine. So the question is "how the undesired assemblies are referenced in your case?".

3. Will have a look at the unit test sample. txs. may need your further input on this one.

4. My apologies. I misinterpreted my own release notes. It's actually opposite. The 4.0.2 release has no CLI but class library changes only.
   I just uploaded it. It will take ~20 mins for the server to index the package

[marcocecchiscmgroup]

About point 2. The end result that we achieved with the patch in the pre .NET 5 version was to find a way not to load some assemblies. Not loaded, not even referenced of course.

Regards,
Marco.

[marcocecchiscmgroup]

Ah ok, I see your point. That problem is that some assemblies are chain-loaded, being triggered by dependencies. In this respect, not referencing as 'top-level' deps is unfortunately not enough.

[marcocecchiscmgroup]

Hello again Oleg,

about point 3, just know that my colleague's PC is Win7.

Instead, the previous discussion made me realize that the present SetRefAssemblyFilter() implementation is probably flawed with respect to the original touchscreen issue. I might be wrong, so maybe just a (dis)confirm would be enough. Sticking to the old code,

public string[] GetReferencedAssemblies(string code, params string[] searchDirs);

correctly returned a long list of all referenced assemblies, both directly and indirectly invoked in the code. This worked via assembly names as strings, as the list was statically determined by the cs parser, I guess, without the need to load them in advance. This method, however,

        public Assembly[] GetReferencedAssemblies()
        {
            var assemblies = _referencedAssemblies.Select(r => Assembly.LoadFrom(r)).ToArray();
            return assemblies;
        }

returned the loaded assemblies. I remember filtering after calling this latter didn't work, because fetching the .wmd assemblies in the appdomain address space caused everything to break: the weird system.attribute not found error. In fact, that was actually my first attempt, but it didn't work. To avoid that issue, the fix was to return the assemblies list as strings

        public List<string> GetReferencedAssembliesPaths()
        {
            return _referencedAssemblies;
        }

and filtering before loading. Fortunately the internal representation was as strings,

private List<string> _referencedAssemblies = new List<string>();

but after a quick glance I am afraid the new version is different. It keeps only a list of Assemblies, allowing to filter in case. I'm not sure whether this would work wrt the original touchscreen issue.

Please let me know,
Marco.