Fixed: Mod can be compiled with assemblies that are not available ingame #64
Conversation
System.Data, System.Xml.Linq, System.Data.DataSetExtensions and System.Net.Http aren't loaded by Parkitect but are used by the ParkitectNexus mod compiler. If something inside one of these assemblies is used in a mod, the mod compiles but throws an exception when used: "Could not load file or assembly 'System.Xml.Linq ...". The assemblies used by Parkitect are in /Parkitect_Data/Managed. The ModCompiler already scans for assemblies in this directory. Therefore the ModCompiled doesn't have to treat System, System.Core and System.Xml extra: It doesn't matter if the compiler uses the assemblies installed on the system or the assemblies from Parkitect. Consequently the full SystemAssemblies array can be removed. Additionally IgnoredAssemblies was removed too: Assemblies that aren't used by the code are automatically ignored during compilation. In fact "Microsoft.CSharp" doesn't even get resolve because it isn't a dll inside the /Parkitect_Data/Managed folder. Note: This is a breaking change: Previously a mod could still contain code that uses stuff from "System.Data, System.Xml.Linq, System.Data.DataSetExtensions, System.Net.Http" -- unless these code parts get loaded (and therefore one of the missing assemblies) the Mod doesn't fail. With the changes in this commit the Mod doesn't even compile. But since those parts can't only "live" in the Mod as dead code and cannot be used, I think a fast fail during compilation is useful to indicate a wrong way. And additional an error while compiling is certainly better than a runtime error.
|
At this point I really don't want to make breaking changes. We should at least find a way to make it backwards compatible. One way to do this would to look if any of these system assemblies are references, ignoring these and then adding the Unity equivalent from the /Managed folder |
|
The commit already uses only the assemblies from the Managed folder. It's just that folder doesn't contain all of the assemblies specified in the SystemAssemblies array (like System.Xml.Linq). Therefore a mod using such an assembly can be compiled but will fail at runtime. That's a serious bug in the mod -- that the compiler should catch, not the runtime. Take this sample mod. All logic is in the Mod.cs file. In the current state the mod compiles, but fails at runtime because it can't find/load System.Xml.Linq (because it's not distributed with Parkitect): That's a serious bug: the mod compiles but fails at runtime. The compiler should (and can!) definitely catch a reference to an assembly a mod can't use. That's what the proposed commit for the ParkitextNexusClient does. Catch the usage of assemblies that aren't in the Managed folder. Why I call it a breaking change: [Uncomment the line with the call to `GetXml()` (line 16)](https://github.com/Craxy/Parkitect-Linq2Xml/blob/36f75e10efa0c1de9558f5fa555c1dc35a2a9bce/Parkitect-Linq2Xml/Mod.cs#L16). The Mod still compiles, but this time it doesn't fail at runtime. That's because assemblies are loaded when needed: no call to something that uses something from System.Xml.Linq and therefore the runtime doesn't try to load System.Xml.Linq. This has several issues: - The usage of something that isn't available during runtime should highlighted by the compiler. Even if I don't use it -- if I have code to uses something from such an assembly I plan to use it in the future. I want to be notified quite early on in development about this, not after I invested some time into development.\* So I still call it a serious bug in a mod when it uses Linq2Xml - the lazy loading of assemblies is an implementation detail. It can change any time on any OS. Not very like but still. And Unity is working a native-ish compiler, so who know what will happen... With the proposed commit neither the first nor the second version will compile. On how "practical" breaking the change is: I searched through a couple of mods for usage of System.Xml.Linq (that's the assembly that's most likely used of all of the "faulty" assemblies) -- I found no mod that used System.Xml.Linq (and why should a mod use Linq2Xml. Only use I can think of is for exporting settings or state without using Parkitect features, and none of the current mod is close to doing this). I really think there's no mod out there that uses one of the assemblies -- and if one does it is a bug in that mod because it can't be used a runtime.
|
|
I agree the compiler should warn you preemptively about problems such as these and you are right in saying that we should resolve this issue. The problem is that every single mod has a reference to these libraries. These libraries are added by default by Visual Studio to projects and therefore appear nearly everywhere. I must admit that leaving these references in in the templates we've published was a bit of an oversight as well. Nevertheless, I don't want to make a breaking change, causing mods that did just fine to stop working. The proper solution, in my opinion, is to add a warning to the compile log to indicate these references should be removed and then the compiler should simply ignore them. |
|
The reference in the project files are no issue: they get just ignored. ParkitectNexusClient uses its own compilation system and just reads the references from the csproj file. Afterwards it tries to resolve these entries (looking into the Managed folder and previously additional via that SystemAssemblies array). If it can't resolve an assembly this assembly isn't passed to the compiler and therefore ignored. Ergo: the csproj file can contain as many reference to whatever assemblies it wants -- as long as the code doesn't use any assembly that isn't in the Managed folder it works fine. Else it crashes. I really shouldn't have it called "Breaking change"...that indicated so much negative side effects..... What it is: The compiler has a bug where it lets Mods compile that use inaccessible assemblies (by "use" I mean "used in code" not just "named in the project file"). To be precise the compiler compiles when code used something out of "System.Data, System.Xml.Linq, System.Data.DataSetExtensions, System.Net.Http" but it'll fail during runtime. And again: as far as I can tell there are currently no mods that will break -- simply because no mod uses stuff out of the previously mentioned assemblies. Again: if they are referenced in the csproj file doesn't matter, as long as they are not used by any code.
In fact that's already the case: Unknown assemblies (assemblies that are neither in the Managed folder or in the SystemAssemblies array) are ignored and output a message in the log: "Info: IGNORING assembly reference |
|
Sorry for the delay - Been quite busy, will check this out before the next release is planned (which isn't for a few weeks) |
System.Data, System.Xml.Linq, System.Data.DataSetExtensions and System.Net.Http aren't loaded by Parkitect but are used by the ParkitectNexus mod compiler. If something inside one of these assemblies is used in a mod, the mod compiles but throws an exception when used: "Could not load file or assembly 'System.Xml.Linq ...".
The assemblies used by Parkitect are in /Parkitect_Data/Managed. The ModCompiler already scans for assemblies in this directory. Therefore the ModCompiled doesn't have to treat System, System.Core and System.Xml extra: It doesn't matter if the compiler uses the assemblies installed on the system or the assemblies from Parkitect. Consequently the full SystemAssemblies array can be removed.
Additionally IgnoredAssemblies was removed too: Assemblies that aren't used by the code are automatically ignored during compilation. In fact "Microsoft.CSharp" doesn't even get resolve because it isn't a dll inside the /Parkitect_Data/Managed folder.
Note: This is a breaking change: Previously a mod could still contain code that uses stuff from "System.Data, System.Xml.Linq, System.Data.DataSetExtensions, System.Net.Http" -- unless these code parts get loaded (and therefore one of the missing assemblies) the Mod doesn't fail. With the changes in this commit the Mod doesn't even compile. But since those parts can't only "live" in the Mod as dead code and cannot be used, I think a fast fail during compilation is useful to indicate a wrong way. And additional an error while compiling is certainly better than a runtime error.