gui-cs / Terminal.Gui

Cross Platform Terminal UI toolkit for .NET
MIT License
9.32k stars 673 forks source link

v2 crashes on run with missing dependency JetBrains.Annotations #3544

Open tznind opened 2 weeks ago

tznind commented 2 weeks ago

Describe the bug When referencing the latest v2 Terminal.Gui nuget package, starting up crashes:

System.IO.FileNotFoundException: 'Could not load file or assembly 'JetBrains.Annotations, Version=4242.42.42.42, Culture=neutral, PublicKeyToken=1010a0d8d6380325'. The system cannot find the file specified.'

To Reproduce Create a new project:

dotnet new console -n "repro"
dotnet add package Terminal.Gui --prerelease

Set Program.cs to:

using Terminal.Gui;

Application.Init();

Application.Run(new Window());

Application.Shutdown();

Expected behavior Program should start up, instead you get the above exception

As a workaround you can manually add the JetBrains.Annotations dependency into your project.

dodexahedron commented 2 weeks ago

Multiple simple ways to handle that from our end. Most ideal is probably a little bit of a couple of them.

Just got home, but I'll take a look at it once I'm all settled.

dodexahedron commented 2 weeks ago

Finally got in. Sheesh. Ok, taking a look.

@tznind

Was that the actual version it threw at you, verbatim, or was that just silliness?

dodexahedron commented 2 weeks ago

Nevermind on that. Seems that's the actual version in the assembly. 😆

Red Herring anyway.

As with any analysis package, dev dependencies like that are dev-time only for the consumer and don't become part of their application.

All that needs to happen is having the dependency listed in the nuspec, which is just minor project file modification, since the nuspec is auto-generated from that.

I see a few minor issues with the project file aside from that, some of which are just legacy stuff or redundant, and a couple places we can make the builds a bit better/cleaner.

There's one element in the Terminal.Gui.csproj that I'm pretty sure can go but which I wanted to ask you about first, @tig :

What's that <GitRepositoryRemoteName>upstream</GitRepositoryRemoteName> element in the nuget packaging PropertyGroup at the bottom all about? I don't see anything in the build that would require that and all it really does anyway is makes local builds throw warnings that are just noise.

If it is needed by something, for some reason, there are a couple of available options to handle it cleanly. But, if it's not needed, I'll just take it out. Naming git remotes isn't a particularly common step of build pipelines, since it's only really relevant when you have more than one remote defined.

If there's a desire for the github workflow runners to be able to target multiple different remotes for test builds or whatever, that's something better handled in the workflow itself, through stuff like:

I actually prefer to do a little of all of the above, since they're not remotely exclusive approaches and complement each other quite nicely.[^AllTheThings]

In fact, there's a lot of that sort of thing in the various workflows I'm putting together, at least one of which I'm going to be publishing on the github actions "marketplace" for general public use, soon, once I finish polishing it up.[^Oooo-Shiny]

[^Oooo-Shiny]: That one does a whole bunch of things that make workflows a whole lot easier to work with, especially for multi-platform jobs. I've got most of the org-specific stuff in that one removed, now, but there's still a little left, plus documentation and whatnot. [^AllTheThings]: For example, environments can be handy to provide known baselines for certain sets of variables, and then a workflow can optionally override them, if desired.

dodexahedron commented 2 weeks ago

......

Visual Studio really frustrates me sometimes....

I wasted so much time today fighting it, because it was randomly refusing to load random projects in random solutions. And just recently? Out of nowhere, it worked perfectly for like 20 minutes. Then it went back to being weird again... While I was reviewing an internal PR, so not even changing things!

le sigh 😞

tig commented 2 weeks ago

GitRepositoryRemoteName is related to SourceLink.

I think I put it in to try to fix the stupid build warnings. Go ahead and take it out for now.

BDisp commented 2 weeks ago

Visual Studio really frustrates me sometimes....

I wasted so much time today fighting it, because it was randomly refusing to load random projects in random solutions. And just recently? Out of nowhere, it worked perfectly for like 20 minutes. Then it went back to being weird again... While I was reviewing an internal PR, so not even changing things!

This actually happens to me often. Sometimes I try to pull from upstream but I only have origin available and all other remotes are disabled. I am forced to close VS2022 and open it again for it to work, sometimes more than once. When this happens I try to do this operation with VSCode and the same strange behavior doesn't happen. I think that VS2022 is unable to unblock some processes that are preventing solutions and projects from initializing, as well as blocking git in some situations.

dodexahedron commented 1 week ago

GitRepositoryRemoteName is related to SourceLink.

I think I put it in to try to fix the stupid build warnings. Go ahead and take it out for now.

Currently (at least since .net 7), all you typically have to do is Have the package reference and the <EnableSourceLink>true</EnableSourceLink> element, which we have both of.

But, rather than taking out the GitRepositoryRemoteName element, I had a better thought that I've already done in my working copy: Just make the nuget-related stuff conditional and only active when told to be.

I did that and created a configuration for CI builds that does a few other things local builds don't need to do or that can make local builds harder to debug. I could have made it just do it automatically based on finding the GITHUB_WORKSPACE environment variable or something, but a configuration is easier, clearer, and also allows you to run the CI config locally, if you want/need to.

While doing that, I collected all of the stuff related to nuget down in the same part of the Terminal.Gui.csproj file, so it's all nice and clear.

dodexahedron commented 1 week ago

Visual Studio really frustrates me sometimes.... I wasted so much time today fighting it, because it was randomly refusing to load random projects in random solutions. And just recently? Out of nowhere, it worked perfectly for like 20 minutes. Then it went back to being weird again... While I was reviewing an internal PR, so not even changing things!

This actually happens to me often. Sometimes I try to pull from upstream but I only have origin available and all other remotes are disabled. I am forced to close VS2022 and open it again for it to work, sometimes more than once. When this happens I try to do this operation with VSCode and the same strange behavior doesn't happen. I think that VS2022 is unable to unblock some processes that are preventing solutions and projects from initializing, as well as blocking git in some situations.

Haha yep.

And I even deleted caches at every level I am aware of.

It's a wonderful and powerful piece of software. But man, when it wants to throw a tantrum, it goes all out.

dodexahedron commented 1 week ago

And yep, I do the same as you with Code @BDisp. It's usually my backup environment, since it's pretty much the best text editor on my windows partition. 😅 It's also pretty great for other languages, too, so it's usually running anyway.

But MAN I sure do miss all the goodies I've got just the way I like them in VS, when I can't use it.

dodexahedron commented 6 days ago

I plan to at least partially (but probably completely) address this as part of #3572.

BDisp commented 6 days ago

I'm also facing this same problem when trying to run a self-container (trimmed and single file) even adding the JetBrains.Annotations dependency to the project. This situation should be corrected as quickly as possible because it is making it difficult to speed up development.

image

dodexahedron commented 6 days ago

Self-Contained is going to require additional testing, anyway, but this specific problem, at least, will be resolved by #3572

BDisp commented 5 days ago

Self-Contained is going to require additional testing, anyway, but this specific problem, at least, will be resolved by #3572

I think I've already managed to somewhat overcome the fact that Self-Contained is not functional in v2. But I'm really running into the JetBrains.Annotations problem. I'm glad you're already trying to resolve this and I just hope it's more or less brief so I can confirm that Self-Contained will work. Thanks.

dodexahedron commented 5 days ago

You can make it go away for yourself by not defining the JETBRAINS_ANNOTATIONS constant in the project configuration.

The attributes are conditionally compiled, and only included in compiled output if that symbol is defined. If it isn't they exist only in the project and are not emitted into the assembly on build.

One of the alternate configurations I've made that will be in the PR for #3572 uses exactly that method to exclude them.

dodexahedron commented 5 days ago

@tig, if you wanna just take out the JETBRAINS_ANNOTATIONS constant in the base project config, that won't hurt anything and will make that issue go away for now. It doesn't make them stop working for analysis while working on TG itself.

I'm currently planning to drop the PackageReference in most configurations and use the actual code for them, anyway, which is fully condoned behavior from JetBrains. The only reason they have that suggestion in that settings page to use the nuget package is so that you'll get new stuff whenever it's added. But we don't need that and if something useful were to come up, we can just copy that attribute class, too.

I said "most" configurations, above, because I'm making it conditional, so that in most builds it is just compiled in, but with the ability to reference instead, if the consuming project already has a reference to that library, so that there aren't ambiguous type warnings when they compile their stuff.

Basically, the functionality remains, but the dependency disappears.

BDisp commented 5 days ago

After I removed the JETBRAINS_ANNOTATIONS constant in the base project config, I'm getting the following exception with win-x64, but at least it tried to run.

Unhandled exception. System.TypeLoadException
   at Terminal.Gui.WindowsConsole.WriteConsole(IntPtr, String, UInt32, UInt32& , Object)
   at Terminal.Gui.WindowsConsole.WriteToConsole(Size, ExtendedCharInfo[], Coord, SmallRect, Boolean)
   at Terminal.Gui.WindowsDriver.UpdateScreen()
   at Terminal.Gui.Application.Begin(Toplevel)
   at Terminal.Gui.Application.Run(Toplevel, Func`2 )
   at Terminal.Gui.Application.Run[T](Func`2 , ConsoleDriver )
   at Program.<Main>$(String[]) in E:\Repos\CSharp\gui-cs\Terminal.Gui\SelfContained\Program.cs:line 9

With linux-x64it can run but the Configuration Manager isn't working, not loading with the right color and quit key. I need to find a way to JsonSerializer generate the source code for self-contained.

image

dodexahedron commented 5 days ago

That isn't related to that.

But sometimes it doesn't update the dependency files, which seems to have broken/changed in a recent .net SDK update, because the behavior started recently across a lot of projects I'm working on that have no relation to each other, and some of which don't use the JetBrains.Annotations attributes at all.

Try this:

dotnet clean
dotnet restore --force-evaluate --no-cache
# and then do the build as you normally would

Let me know if that changes anything.

dodexahedron commented 5 days ago

It's almost like lock mode is now default or something for nuget. That's how it's behaving, anyway.

BDisp commented 5 days ago

It's not related. It's due the limitations of not be allowed reflection. In the win -x64 it's some type not found.

dodexahedron commented 5 days ago

Ah. Yeah there's still some of that here and there. We'll get it eventually!

.net 7 can break people by default anyway, including for V1, because all assemblies that don't opt out now default to having the trim options turned all the way on. It was in a breaking change doc for .net 7.

Here's something you could try, though, to see if it is a viable workaround for anyone who absolutely needs to be able to run it and doesn't care that the self-contained will be MASSIVE as a result:

Add this to your publish command: -p:PublishTrimmed=false

dodexahedron commented 5 days ago

Also, if any of our referenced dependencies aren't trim-compatible, we'll need to add <TrimMode>partial</TrimMode> to a PropertyGroup and <TrimmableAssembly Exclude="Name.Of.Assembly.To.Not.Trim" /> or a <TrimmerRootAssembly Include="Name.Of.Assembly.To.Not.Trim" /> in an ItemGroup, in the project file.

Or we can target more granularly with an XML file and feed it to a <TrimmerRootDescriptor Include="SkipTrimForThisStuff.xml" /> instead of rooting them. Example XML for that from the docs is:

<linker>
  <assembly fullname="MyAssembly">
    <type fullname="MyAssembly.MyClass">
      <method name="DynamicallyAccessedMethod" />
    </type>
  </assembly>
</linker>

Of course, the best option is just to avoid it all. But if we end up being unable to, for some component, there are options. Gross ones. But that's why they're there. 🤷‍♂️

BDisp commented 4 days ago

I've managed to get win-x64 to behave like linux-x64. The WriteConsole method has a parameter with the incorrect type. I will submit a PR to fix this. Unfortunately, the ConfigurationManager is not deserializing item values ​​because reflection is no longer allowed in self-container. I will have to try to find a solution by manipulating the TypeInfoResolver of the JsonSerializerOptions. If anyone reads this and already knows how to resolve this situation inherent to System.Text.Json.Serialization, I would greatly appreciate sharing it, thank you.

image

dodexahedron commented 4 days ago

ConfigurationManager is likely to be the main casualty of that, or at least need the most work to adapt.

The Microsoft.Extensions.Configuration binder is all compile-time code generation, so it is trim-friendly (explicitly - that was a goal of it), and supports the standard DI pipeline/service host infrastructure as well. Plus, the type converters would still be relevant - just not any of the actual serializer code. The config file schema itself is already not a problem, as far as I can tell, and should work almost entirely out of the box with MEC, with the help of the converters (mostly for the custom key formatting and whatnot).

Converting to MEC is already something I had planned to do once other more important stuff is done, but is still not at the top of my priorities at the moment, so it's certainly up for grabs if someone wants to tackle it before I get to it.

It came up in an issue or three many months ago, but it was very understandably not a priority back then, and is still kind of a low priority now - at least in my opinion - since it's not a development blocker for other functionality that isn't directly related to trim-compatibility.

Although that time is certainly approaching, as we are now starting to think more about the trim issues.

tig commented 3 days ago

ConfigurationManager is likely to be the main casualty of that, or at least need the most work to adapt.

I am not convinced MEC can replace CM without losing some key functionality.

Regardless, I'm also not convinced CM can't be tweaked to work with trim.

A big part of the reflection it uses is due to limitations that existed in System.Text.Json when I wrote it ... that are now fixed. I don't remember all the details.

I've said it before though: If someone wants to replace CM with something based on MEC please have it... as long as it functions effectively the same.

tig commented 3 days ago

Here's the issue: https://github.com/dotnet/runtime/issues/29538

Resolved here: https://github.com/dotnet/runtime/issues/78556

BDisp commented 2 days ago

I've already managed to attach Self-contained to the debugger using win-x64. However, I cannot attach using linux-x64 and osx-x64 to the debugger, as it gives the following error. If anyone knows how to solve it, I'd be grateful in advance.

image

dodexahedron commented 2 days ago

I've said it before though: If someone wants to replace CM with something based on MEC please have it... as long as it functions effectively the same.

Yep, that's what I referred to earlier (I think I submitted that comment anyway). It's just been not a big deal, with more pressing work to be done elsewhere, so far.

What you wrote largely works the same as the MEC binder, already, so my hope is that, when I sit down to address it, it'll mostly just be replacing the CM class itself, with the other code needing minimal tweaks, where any are needed at all.

Dropping a [JsonSerializable] on serialized types, with the generation mode set to the appropriate value makes it not emit both the source gen and reflection implementations, at compile time, which is the default if you don't specify the mode explicitly. That would probably help things, too, but seems like a waste of time to bother with since it's not like TG V2 is non-functional without that, so long as folks don't try to use it in a known incompatible way, especially for a pre-beta library.

dodexahedron commented 2 days ago

In other words, I think it's totally fair to simply tell people what I've said in the issues that have been posted so far, which is basically "We're aware. It's not a supported use at the moment. It is planned to be a supported use before release. Hold your horses."

The three of us can only do a million things at once. A million +1 is a bit much. 😆

And while I'm certainly happy to play a consultative role most of the time, this one in particular is one that is quite distracting/disruptive to whatever else I'm working on at a given time, and more work for everyone overall, which is why I offered to own the responsibility for it in the first place.

CM, aside from this caveat and the somewhat philosophical thing about not being idiomatic .net 8.0 configuration code, is good and frankly a bit impressive in how close it gets to the standard way created by more people over more time, many of them getting paid to do it.

And it does what it needs to do for the vast majority of existing cases, with the pretty small and totally reasonable pre-beta stipulation that a consumer just can't publish trimmed (yet).

If it were a bigger deal or I had felt it were so urgent to repaint that bike shed, I'd have addressed it directly a long time ago and fairly firmly pushed back on anything since then that expanded code in that subsystem until a replacement were ready for a PR. 🤷‍♂️

But it wasn't, so I didn't, and I certainly have no regrets outside of empathy for @tig in particular, about any work done that ultimately goes poof. But that's how this game goes. 😅 If we somehow could get everything perfect with each keystroke, we'd be gods.

BDisp commented 2 days ago

I'm already fed up with doing the procedure below millions of times, even though I'm always using the same v2 branch just because I'm publishing with a different platform (win-x64, linux-x64 or osx-x64). It's very frustrating because it greatly reduces productivity.

cd Scripts\
import-Module ./Terminal.Gui.PowerShell.psd1
build-Analyzers -AutoClose -AutoLaunch -NoClean -Force
remove-Module Terminal.Gui.Powershell
exit
tig commented 2 days ago

I'm already fed up with doing the procedure below millions of times, even though I'm always using the same v2 branch just because I'm publishing with a different platform (win-x64, linux-x64 or osx-x64). It's very frustrating because it greatly reduces productivity.


cd Scripts\

import-Module ./Terminal.Gui.PowerShell.psd1

build-Analyzers -AutoClose -AutoLaunch -NoClean -Force

remove-Module Terminal.Gui.Powershell

exit

The value the analyzers provide right now is pretty minimal. I suggest if @dodexahedron can't commit to a fix that fixes these qol things in the next week, we back the analyzers out until the issues can be addressed.

tig commented 2 days ago

But it wasn't, so I didn't, and I certainly have no regrets outside of empathy for @tig in particular, about any work done that ultimately goes poof. But that's how this game goes. 😅 If we somehow could get everything perfect with each keystroke, we'd be gods.

No angst here about stuff I've built going byebye. Heh, I built IE, Media Center, WHS, and WP7 and got over their deadness a long time ago.

I just care about the requirements.

dodexahedron commented 1 day ago

I'm actually working on it right now, to give a temporary fix that can be delivered quicker.

Just putting them into a nuget package in a local feed, so they can act like any other.

I'm almost done with that. There are just a couple of small differences in analyzer packages, and I want to test in a few different environments to be sure I haven't missed anything major while splitting it off from the solution.

You'll still be able to build them yourself if you want to, for some reason. But I don't intend to make code changes to them before I publish the general-use package on nuget.org, so there's not really any point in doing that.

Terminal.Gui dev experience then goes back to before, but with the analyzers still working.

The Analyzers projects get removed from the build order in the solution so there's no project dependency for build any more. Or if you use the filters I added in one of the recent PRs, you also won't even be loading those projects anyway.

dodexahedron commented 1 day ago

But it wasn't, so I didn't, and I certainly have no regrets outside of empathy for @tig in particular, about any work done that ultimately goes poof. But that's how this game goes. 😅 If we somehow could get everything perfect with each keystroke, we'd be gods.

No angst here about stuff I've built going byebye. Heh, I built IE, Media Center, WHS, and WP7 and got over their deadness a long time ago.

I just care about the requirements.

I'm not over Windows Phone getting brutally murdered the way it did. My Lumias are still my favorite phones I ever owned. Oh and the HP WP10 phone I had that had a laptop shell to slide it into, turning it into a netbook was fucking awesome.

Maybe now with Windows on ARM and some other stuff going on, MS might take another stab at it at some point? One can dream, anyway.

Losing Media Center was a shame, too. Plex is pretty good, and of course now has features accumulated over time, but it's got some rough edges in some places where WMC felt more polished, still.

I never got on the WHS train, because I always had licenses for home goodies built into my volume licensing contracts, plus technet and MSDN, so I had full Windows Server and AD at home for the past 24 years. And now I spoil myself with Windows DC licensing, for the unlimited virtualization rights that make it cheaper than a bunch of standard pretty quickly, if your core count isn't too high. Moving all the windows enterprise desktop licensing to MS 365 soon, though, dropping them off the Open Value contract, because it's slightly cheaper but includes more than just Windows, so it's a massive savings. Not very often licensing costs go down!