-
Notifications
You must be signed in to change notification settings - Fork 676
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
v2 crashes on run with missing dependency JetBrains.Annotations #3544
Comments
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
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 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.1 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.2 Footnotes
|
...... 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 😞 |
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. |
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. |
Currently (at least since .net 7), all you typically have to do is Have the package reference and the 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. |
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. |
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. |
I plan to at least partially (but probably completely) address this as part of #3572. |
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 |
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. |
@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. |
After I removed the JETBRAINS_ANNOTATIONS constant in the base project config, I'm getting the following exception with
With |
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. |
It's almost like lock mode is now default or something for nuget. That's how it's behaving, anyway. |
It's not related. It's due the limitations of not be allowed reflection. In the win -x64 it's some type not found. |
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: |
Also, if any of our referenced dependencies aren't trim-compatible, we'll need to add Or we can target more granularly with an XML file and feed it to a <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. 🤷♂️ |
I've managed to get |
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. |
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 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. |
Here's the issue: dotnet/runtime#29538 Resolved here: dotnet/runtime#78556 |
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. |
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. |
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.
|
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. |
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 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. |
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! |
Describe the bug
When referencing the latest v2 Terminal.Gui nuget package, starting up crashes:
To Reproduce
Create a new project:
Set Program.cs to:
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.
The text was updated successfully, but these errors were encountered: