-
Notifications
You must be signed in to change notification settings - Fork 286
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
Moving package management to a csproj #786
Comments
Maybe something like this could be used? |
That could work but would require ppl to manually edit the csproj every time they change a dependency. Would prefer a solution that just works out of the box when using the standard tooling. From both user experience and a maintenance perspective this would be a good goal to keep in mind. Which is why I was thinking of modifying duality so it will just work if all dlls are in the same folder. However @ilexp probably put plugins in a plugin folder for a good reason so iam interested in what his vision is on this. |
Ah, I was more thinking of having an on-install script run when Duality is added to the project, but sure, better to wait word from the boss 😄 |
Mostly discoverability and general tidyness. The But back to the start: Leveraging msbuild / VS package management instead of our own is a great idea, 👍 for that. Having a custom package manager was nice at some point in the past, but by now it's a liability and a constant drain of resources due to maintenance cost. We should get rid of it as soon as possible without downgrading the user experience, especially on the first run. Duality has managed to be very "plug and play", and it should continue to be, both for experienced developers and beginners. If we figure out a way to achieve that, we're good to go. Besides shedding some weight, I also think the "csproj based" way is a great opportunity to be one step closer to the developer who just quickly wants to set up a project. There could even be a VS project template that creates a full, up-to-date Duality setup in a project folder of choice. The editor and launcher would be nothing but local tooling that is installed into a user-created and controlled project for their convenience - not the host of everything they do. Getting to the tech part. On the top of my head, here are a few points we need to figure out:
Overall, I think we can't get away without some msbuild programming here, maybe via What about nuget install scripts? Do they still exist and are they supported long-term? |
Some quick research links on nuget install scripts:
Seems like they're no longer supported in all tooling. The concept of content files looks interesting though and could maybe (?) offer some help: |
The download package will contain a solution with all the required dependencies in it. We could make a small bootstrap script to run nuget restore and build it. The solution + assets should be the source of truth here though and this also means you only have to check in the solution + assets when using source control. No binaries in source control is a nice improvement I think.
We could use the content files feature of nuget for this. One difference is that these files are immutable though. There doesn't seem to be another way to do this with packagereference.
We could leverage a custom output path in the csproj. As packagereference should include all transitive dependencies you could build the the editor or launcher project and it should output everything you need to run it in the (custom) output path. VS does not clean this entire folder when rebuilding or cleaning, only the files that are part of the build. As long as these build artifacts are immutable after a build that should be fine. Still thinking how we will bundle the assets though, I think we should put these in a separate folder? |
Sounds good. Maybe we can get down to just a
I think immutable is generally okay for content and source that comes bundled with a package - it's not meant to be modified, and any modification to them in a project would be gone when updating the package. We should still check what exactly immutable means - does it flag the files as read-only? Are they only read-only in Visual Studio? As long as the immutability is VS only, we might get around it still to allow users to experiment with samples as intended. Content files could be edited as usual in the editor, and bundled sample source files come with their own .csproj anyway, in which they would be editable again because from the bundled .csproj's perspective they're just files, not added via package. We should only make sure to not include the source files in the project that references the package to avoid having them around twice.
Ah, so if VS doesn't touch stuff it didn't put there in the first place, that could actually be fine.
I'd recommend not changing Duality's general directory structure in the first step - it's an option, but as long as we can manage without, that would be the safer route. So let's say we use a custom build output path that is the folder where the .csproj and .sln are itself, that would mean all the required binaries, launcher and editor would end up right there as well. The editor will create its own subfolders as needed when run, and all packages bundling content could target a
I think we could use an overview on the directory structure and files we're going for, so we have something concrete to talk about and iterate on. @Barsonax Can you create one based on prototype and latest thoughts in the thread? |
I think so so this will make the initial package really small and we could probably turn it into a VS template or do something with dotnet new so you can start a new duality project purely from the command line. That's more stuff for the future though but a very nice to have.
Last time I checked you could actually modify them from visual studio. This seemed like a bug to me. Content files are shared (as in they don't actually reside in the solution directory) so I don't think you even want to modify them as that can cause some issues with other projects referencing those files. Even if you do as soon as you push it to a CI server your changes would be completely gone.
Plugins should be a separate csproj but we can add it to the same solution file.
Yup we can try to see how far we can get without changing the directory structure. I don't think we can get to 100% though but it will make the bigger picture more clear.
Ye agreed I can setup a template, it will not work yet as it will require some changes in duality but it would make our idea's more concrete than just a discussion. |
Worked a bit more on the template, its not in a working state but should give a clearer picture of the idea: Issues/things to keep in mind:
|
Looks pretty good so far. We should keep track of all the small issues and todos though, so here's the old progress template that served me well in the past. I actually have that as a saved issue response and can only recommend it: Progress(pretty much all @Barsonax work)
ToDo
|
One thing Iam thinking of now. To properly test this I think we need to release a new nuget package but we don't want to break anything. I think we should make it possible to put a prerelease version (like -alpha suffix) on the nuget package. That way we can test it safely. Shouldn't be too hard I think. |
With the integrated package manager, I just put all the requried packages in a local folder on my machine and set that as a repository URL. I'm not sure nuget supports this as easily via VS, but you can add custom repository URLs. Maybe that's worth looking into? If possible, I'd like to avoid releasing anything for testing. Another alternative would be to set up a custom package repository somewhere if it can't be done locally. Didn't GitHub add this as a feature for projects recently? |
It's definitely possible to have a local nuget repository, it's not that difficult even with command line. |
Ok Ill just test it that way then, thanks for the tip |
So how do I build the duality packages? Getting this error when trying to run nightlybuild
EDIT: So had to disable the documentation build and after that download nuget.exe (because it couldn't find it?). Now I got the nuget packages so testing can start. |
@ilexp how do you update the versions of the packages? You surely don't do this manually for every nuspec iam guessing? I saw there is a VersionUpdater project but it does not work because it depends on sourcetree which I don't have...
I think the build code can also use a bit of a cleanup to make this easier (it should handle all dependencies itself in my opinion). This is also why I prefer to use a tool like https://nuke.build/ because that will just work ootb as it does this all for you (the only thing you really need is .NET pretty much). Not something we should do now though but for the future. After I fixed the path the version updater project doesn't seem to do anything and just exits. |
I can see you have already found some of the build tools, but for clarity's sake I'll back off a few steps and start at the beginning: What you need for building packages and updating package version is the Build\Scripts folder, specifically Update Package Versions
Package Binary
|
None that I know of.
Now that you mention it, there may be one behavior that could cause issues when the plugins are in the main folder: The main folder is also where .NET natively searches for assemblies - so it might skip our custom AssemblyResolve for plugins, because the used AppDomain event is only invoked "when the resolution of an assembly fails." (see here). That means, we would have different and potentially runtime-dependent behavior as soon as we start putting plugins in the main folder, with potentially nasty side effects - for example loading an assembly twice and now having two identically named types that are incompatible to each other, and to make things worse, it would be undefined which of the two is instantiated dynamically when deserializing stuff, or looking it up by name. This definitely needs to be prevented. It might be that we actually need to put the plugins outside the main folder for that reason, simply because that way we can control how types are resolved. Even if it didn't break at runtime, it probably will in the editor when attempting to do a hot-reload of plugins on detected changes. If we can't rule this out as a potential source for trouble, I'd say it's back to having a |
Yeah the dll's are getting loaded twice. Checked it with This caused the same static class to actually have 2 instances, even confusing the debugger to make it even nastier. I think with some changes on the duality side this can be prevented though. We don't really need to load plugin dll's ourselves anymore if .NET already does that for us. EDIT: this code seems to do the actually loading
Like the comment already explains it hides where a assembly is loaded from, thus .NET simply loads the assembly again as it thinks that dll is not loaded yet I think. Since everything is now in the same folder do we even need to do this? |
Loading plugins went through a few iterations and edge cases over the years, which I can't even summarize here for lack of complete memory, and we all know how critical it is for Duality to work. It's also been stable for quite a while - so to ensure stability, I don't think we should change behavior for this issue here to work unless there's no other way. Another point would be that .NET locks the automatically loaded assemblies, so you can no longer do any hot-reloads in the editor, since the loaded plugins can no longer be overwritten. The only way around that is to do a custom load that doesn't lock them. For now it seems having a Plugins folder is the easier way out. And it's kinda nice to have them grouped that way in the file system anyway. |
Meh that kinda breaks the UX as you now have to modify the csproj to exclude the duality dependencies when building the project... Else you end up with all duality dlls in the plugins folder because of transitive dependencies. |
Could we change our approach to not use the binary output folder directly, but instead have it as an uninteresting temp folder somewhere, and then let msbuild copy the interesting parts to their appropriate destinations? |
So loading the assemblies with EDIT: Yup it still locks them |
Have to find a way so that ppl are not required to modify their packagereferences in their csproj for every dependency they add. So we could try to do this in a custom .targets file |
Adding to that, I think the issue really isn't only the loading twice or locking thing - would have to trace back git history for all the details, but this comment in the source you quoted is probably there for a reason: // Due to complex dependency resolve situations intertwined with our hot-reloadable
// plugin system, we should manually resolve all dependencies. [...] I'd just like to heed my own warning from the past here. Reaching the same stability (and confidence of) just takes more energy than we should spare as a side quest of this issue.
I mean, we would give up the ability to just add Duality package references to any new .csproj and end up with a working editor / launcher / game plugin tooling setup, but it reduces complexity for now - we can always improve things later on. Even the most basic implementation of this issue would still be a huge win.
Sounds good. We do have a naming convention to work with as a first step, which is probably easier to access via msbuild targets than the tags of the package where a file was delivered from - instead, we could just put everything that contains |
Seems there is a copytooutput on the file node in the nuspec EDIT: it seems it still does not copy to the output folder.. EDIT2: after some more fiddling with nuget I came up with this
The any/any/DualityEditor.xml should be in the |
Managed to grab the documentation file (and any other files if desired) with some msbuild magic (note iam using Identity instead of Filename now):
which you can then copy to a desired folder:
https://docs.microsoft.com/en-us/visualstudio/msbuild/msbuild-well-known-item-metadata?view=vs-2019 Only thing I noticed it does not do this with transitive dependencies so gonna look into that. EDIT this will grab all xml files in the same folder that the reference (dll file) is in. This also includes any transitive dependencies:
EDIT2: making it a bit more strict to just take the documentation file:
The file might not always exist however but we can solve that edge case as well by adding a condition to the copy task:
EDIT3: duality no longer crashes now, Still some erros in the log though but might be because I modified duality which I need to revert now. |
I have been thinking.. maybe we are going at this from the wrong direction. The "plugin" file remains unlocked, can be renamed / deleted / updated, the program unloads the previous context, loads up the new one and there are no duplicated assemblies present. So, why not try and go along the route of having a .net core editor (made with Avalonia maybe)? If you'd like to try, pressing any key other than Escape will reload ClassLibrary1.dll and print the result of a ToString() method. The default dll prints "SIMPLESTRING" while the -dualityetc version will print a concatenation of an XmlDocument, a json parsed value (123) and a Duality Font resource. See that everytime the font changes, since a new instance is being used. (and the loaded dlls remains constant, without duplicates) netcoreapp3.1.zip (requires .net core 3.1 as you can guess from the name 😄 ) |
This sounds like a total rewrite of the duality editor. A much bigger change than what we are trying to do now. Might be interesting for the future (duality 5.0.0? :P) though. We do want the duality editor to be crossplatform on netcore (or net 5 in the future) eventually. Pretty much solved the path issues already, just running into some issues that is caused by duality nuget packages not following the (newer) standards. |
Yes, it would be 😂 |
Managed to get duality working now with the latest changes on https://github.com/Barsonax/DualityProjectTemplate The only issue is that OpenAL ends up in the root folder instead of the plugin folder. You can copy them yourself for now. If you want to test this yourself you need to build duality yourself using https://github.com/Barsonax/duality/tree/feature/csprojpackagemanagement, generate the nuget packages locally and add a custom nuget source before building the template as it uses a custom 4.0.0 version with the package manager and default project being removed as the most notable changes. I think I still need to cleanup this branch a bit though as I seem to have checked in a bit more than necessary (the build script kept complaining about not committed changes...), furthermore I the version metadata on the dlls itself is not yet updated. TODO
@ilexp Does OpenAL really need to end up in the plugin folder? Its not exactly a plugin. Its a bit hard to figure out with msbuild where to put this dll since I have nothing in the name to work with. In https://github.com/AdamsLair/duality/blob/master/Source/Platform/DefaultOpenTK/Backend/Audio/AudioLibraryLoader.cs#L26 it explicitly looks for this dll in the plugin folder so a possible fix would be to look for it in the root folder (or make a new native folder?). |
That should be fine either way - since they're native binaries, they'll not be loaded as assemblies anyway. We'd just have to change this part of the OpenTK backend to allow them being placed in the root folder. Edit: Ah, seems you were faster to edit than I was to answer.
The version updater should already take care of this. Saves a bit of a headache to get all versions and dependencies to align 🙂 I usually run it as the last step, after cleanup and everything else has been done. |
Is there a way to set the templates package references to "latest available"? This would be ideal for the download .zip package. As a neat side effect, that way we'd be able to test without the "4.0.0" versions (package management is inactive anyway, if there is no package config gile) set up locally for the time being. |
Ok then thats a easy fix.
The version updater only updated some of the packages so had to manually update the others. Seems it doesn't understand transitive dependencies completely? I ended up with like 4-5 different versions of duality being installed until I manually bumped all versions to 4.0.0.
I guess this might be possible with floating versions? Not sure if this is a good idea though. Kinda breaks the whole reproducible build thing. Maybe when nuget lockfiles are more mature this would be a good idea. |
So I experimented a bit, and the closest I could get was a fixed major version number, with a floating minor and patch number, like this:
The issue is that these floating versions don't disappear after the first install, so as you said they're not a good fit here - not sure about the nuget lock file thing either, probably better to not depend on that as well at this point. However, this could mean that we'd need to do a new .zip / download release every few big updates or critical fix and leave all the small and patch updates for the users. We'd no longer have a "new install is latest" by default. Might be fine, but requires a slight shift in perspective and potentially adjusted workflow here and there. Something to keep in mind. Edit: If we have an optional "install me" convenience script that people can just double click, we could sneak in a nuget update though, right? Hypothetically speaking, we'd still need to figure out if this is something we want.
It only updates packages for projects that have been modified, as well as packages that depend on them. If there there no change to a project or dependency, it gets left alone - probably why it didn't offer all of them in the version prompt. If you want a full global update, you can use the ApplyGlobalUpdate option via command line, use "Major", "Minor", or "Patch" as value. Forgot to mention that before, it's probably what you need for this particular update. |
Just tested this (using floating Only thing I had to fix manually was move "Nuget.Core.dll" to the main folder, because it ended up in Plugins, being a false-positive to our naming convention. Not critical for now, because it happens to be in a dependency we're getting rid of though. If a user dependency triggers the script that way and ends up in Plugins, it should be loaded just fine via custom assembly resolve. The reason it fails on NuGet is because this is needed before anything else is in place. |
Nuget.Core.dll shouldn't even be there I think you are still using the old duality code? I remove the package manager and its dependencies in my branch.
Hmm but currently with the zip install you have to update that zip right? We could always just run nuget update or make a script for that. Easy enough now we are using the default tooling :). EDIT: ah you already mentioned this dll is removed :P |
@ilexp I noticed there still some logic in duality related to creating a new project. I don't think we need this anymore since this task will be taken over by the template and tools like dotnet new. Aside from that duality seems to be working now with the openal path fixed. As a bonus we no longer get a (ignored) exception anymore since we were trying to load a native assembly. TODO
EDIT: yup all content packages are currently broken. This has to do with the nuget package having the wrong format though. EDIT2: modified the sample packages so that they now use the contenfiles folder. Tested the physics sample and it works. Removed the source files from the package as this does not work with contenfiles. Might be better to put in a link to github for ppl that want to see the source code? EDIT3: installing Singularity now works as well. The plugin ends up in plugins and the core library ends up in the root folder. |
Sounds great, seems there's quite a bit of progress 🙂
Yeah, I used the
Yep, that's what I meant by the "install me" convenience script. We will need an updated .zip at least once, and I'd probably update it every major version step in case a user ignores the script, so they aren't too far off, but that could be a good solution.
Yeah, take it out as well. That part is from before we even had the builtin package management. It's super legacy. When you do, make sure to check if that makes some editor utility code paths unused, which could be removed as well.
That kind of takes away a big part of why you'd have a sample package in the first place - to show users how stuff can be done, right next to their own code, and even allow them to play around with it. Only distributing binaries with a GitHub link is way less usable. I think we should at least investigate if there is some way to keep this. Otherwise, we should check how to mitigate the drawbacks. Do you see any way how source code could be distributed this way? What exactly do you mean by "does not work with contentfiles"?
Let's do this as a step two after we've released this the old way / via downloadable |
There doesn't seem to be a update all command in the dotnet cli (NuGet/Home#4103). However we can probably achieve a similar result with a script using
Ok gonna remove that code then. DONE
You could include the (immutable) source files and not the dll so that when you build the gamelauncher/gameeditor the code is still there. Playing around with it is simply not possible anymore as nuget does not support this, there is no way to modify the source files. That also changes the assembly qualified name though so that probably does not even work :(. Including them both is quite confusing and in that case might be better to put a link somewhere to the source code. Another option is to include the cs files but set the build action to none and copytooutput to false so that it only appears in the project view and not in the build output. Thats kinda faking it as the included source code is not actually used.. So basically I do not see a way to do this properly with the newer nuget. Might as well be explicit about it then and direct users to a github link. This is also why I opted to just include the binary.
Ok I will keep it super simple then. Made a new issue #788 to start discussing possible solutions to this. TODO
|
Yeah, I agree - none of this sounds good. However:
Don't we copy around stuff from a temp folder to the actual folder now anyway? I thought the immutable part is only valid when it's a file that is included via the builtin automatism? As long as we aggregate in temp folder A and copy to actual output folder B, we should be in control. For the sample projects, we also don't really need to do anything with the source files - they don't need to show up anywhere or be included in any project, since the samples bring along their own project that compiles to their own plugin. Only thing we'd still need to do is set up the sample project in a way that it will work out of the box without processing the csproj. (Automatically adding any copied .csproj to the solution file somehow would be an added bonus on top, though I'm not yet sure how that would reasonably work.)
👍
Yeah, we should do that before the release, but maybe not a dependent part of this issue? I'd suggest the following:
Since we'd have (Thinking about that, might be worth considering to "official" git flow at some point in the future, since it avoids that kind of release cycle lock - but that's a topic for another day) |
Yes we copy the build output to a temp folder. Its considered immutable because it gets overwritten on every build. The source of truth resides in the (immutable) nuget package.
I don't know how we could do this. If you want endusers to be able to modify the code you have to do a rebuild of the csproj at some point . Also since its in the build output it will be overwritten by the next build. Better to just make it easy for users to view the source code in github in my opinion. Maybe nuget will support this scenario again in the future.
Ye it should be done in separate issues/PR's, already did most of the work for alot of these issues just have to update the branches and fix the merge conflicts once this issue is done. |
Ah, got it - overlooked that part 🙂 Thanks!
I agree, that's probably for the best given the options we have. We should add (or just refer to) the link in the package summary, so users can't miss it at least. On the plus side, the immutability issue was always there and it's reasonable design that the sample packages kind of broke - at least now we align with that. We'll find a proper way to improve the affected aspects in time. Moving on!
👍 |
Solved in #789 |
Is there already an issue for updating sample packages to have a link to their source path on GitHub, so people can browse it easily? If not, please create and link to this one. |
There is now #794, still open for creative ideas. |
In order to move forward with duality we have to find a way to make dualities way of managing packages compatible with .netnstandard en .netcore.
As it already turned out the nuget upgrade is very painful, so much that I don't really see this upgrade happening anymore.
This issue is to research a completely different approach in which we actually remove the build in package manager from duality. Instead managing packages will be done like any other .NET project, from a csproj.
This has a number of advantages:
Some disadvantages:
Most optimal solution would be to stay as close to a default csproj as possible, which means I would lean more towards modifying duality to fit with the default instead of csproj wizardry.
Work has begun on a poc duality template https://github.com/Barsonax/DualityProjectTemplate
The text was updated successfully, but these errors were encountered: