Skip to content
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

Fixed asp.net core project #3067

Merged
merged 7 commits into from
Jan 10, 2024
Merged

Conversation

cschuchardt88
Copy link
Member

@cschuchardt88 cschuchardt88 commented Jan 7, 2024

Tested
image
image

Closes #3066
Closes neo-project/neo-modules#862

@vncoelho
Copy link
Member

vncoelho commented Jan 7, 2024

I am building here.
If so, will still aspnet be needed in the Module config as well?

@cschuchardt88
Copy link
Member Author

If your talking neo-modules you shouldnt need them ill show contents

RestServer

image

RpcServer

image

WebSocketServer

image

vncoelho
vncoelho previously approved these changes Jan 7, 2024
Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Now need to double check if we revert the inclusion on modules or it is needed there.

@cschuchardt88
Copy link
Member Author

revert it

@vncoelho
Copy link
Member

vncoelho commented Jan 7, 2024

uldnt need them ill sho

I was talking about this PR neo-project/neo-modules@e9e7a07

I tried here, but the inclusion of the Framework Reference on RpcServer.csproj is still needed. The framework reference is needed just for building, right?

@cschuchardt88
Copy link
Member Author

That is correct.

@vncoelho
Copy link
Member

vncoelho commented Jan 7, 2024

Maybe the Swagger will work now, I will try to double check there tomorrow.

@cschuchardt88
Copy link
Member Author

Just FYI, swagger microsoft library has some bugs.
RicoSuter/NSwag#4506
domaindrivendev/Swashbuckle.AspNetCore#2702

shargon
shargon previously approved these changes Jan 8, 2024
@shargon
Copy link
Member

shargon commented Jan 8, 2024

With this solution plugins only can work with neo-node, with my revert #3066 (already closed) with neo and other clients (like neo-gui) @vncoelho please choose you

@cschuchardt88
Copy link
Member Author

cschuchardt88 commented Jan 8, 2024

We are going to netstandard for libraries, right? Applications that ONLY support framework APIs should include the framework and use net7.0 and higher. We shouldn't be adding dependencies when they are not needed. The community as you see in the netstandard PRs; Where it talks about issues that would be one of the problem. The architecture is important here, and not doing so will, ruin the integrity of neo-core and potential of integrating with other systems and frameworks. You don't want to be that one blockchain that can only run one way, right? ... oh wait that's what it is now. Stability is important too, and that's another thing that is lacking. Let's not go backwards here.

@shargon
Copy link
Member

shargon commented Jan 8, 2024

The issue for me is that seems that is not working well

private static Assembly CurrentDomain_AssemblyResolve(object sender, ResolveEventArgs args)

@cschuchardt88
Copy link
Member Author

cschuchardt88 commented Jan 8, 2024

That would be a Plugin Issue, the developer of the Plugin and their responsibility. That's like saying for example my RestServer that supports Swagger shouldn't have its dependencies and they should be added to core.

@shargon
Copy link
Member

shargon commented Jan 8, 2024

That would be a Plugin Issue, the developer of the Plugin and their responsibility. That's like saying for example my RestServer that supports Swagger shouldn't have its dependencies and they should be added to core.

You mean copy manually the files to the parent directory?

@shargon
Copy link
Member

shargon commented Jan 8, 2024

I think that we need to put the asp net core libraries of the plugin in root\Plugins

if (!File.Exists(path)) path = Combine(PluginsDirectory, filename);
and it should work

@cschuchardt88
Copy link
Member Author

cschuchardt88 commented Jan 8, 2024

In the Plugins directory. By default dotnet checks the system32 and windows directory 1st. Than the current working directory. But that method you posted checks the assembly file location, where plugin is loaded. Which is fine. The point is, it's unnecessary to add deps to neo.dll that are not needed.

@shargon
Copy link
Member

shargon commented Jan 8, 2024

In the Plugins directory. By default dotnet checks the system32 and windows directory 1st. Than the current working directory. But that method you posted checks the assembly file location, where plugin is loaded. Which is fine. The point is, it's unnecessary to add deps to neo.dll that are not needed.

If it's solved copying the library there, we can close both and fix the plugin release

@cschuchardt88
Copy link
Member Author

cschuchardt88 commented Jan 8, 2024

#3067 is easier and safer. You wont know all the required dlls for asp.net core. It's a lot. Its just @vncoelho is using a mini version of asp.net core and not the Runtime. Works fine on my computer. But in docker... no. Also main host process neo-cli need to point to framework.

@vncoelho
Copy link
Member

vncoelho commented Jan 8, 2024

Which one is this runtime version?
@cschuchardt88 ,I tried other ones in the RestServer PR and no success. Can you preciselly send the link to the image?

@shargon ,
I think that removing it from NeoCore is good if it is not used nowadays.
Thus,adding on neo cli looks well.

As it now, perhaps that for RpcServer we just need the framework for compilation.

@shargon
Copy link
Member

shargon commented Jan 8, 2024

@cschuchardt88 vould you add it to neo-gui also?

@cschuchardt88
Copy link
Member Author

@shargon yes i can do that.

@cschuchardt88 cschuchardt88 dismissed stale reviews from shargon and vncoelho via d8715cc January 9, 2024 00:22
Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is still not clear to me the best direction to go.

In this sense, @cschuchardt88, I believe it will be better to postpone this.
I am not against it.
I am just in favor of keeping everything simpler and more stable for now. Merge this PR would keep this cycle of trying to fix something that is not still fully solved (at least in the explanations).

@cschuchardt88
Copy link
Member Author

Its RestServer plugin.... i will fix that.

@cschuchardt88
Copy link
Member Author

@vncoelho
It works fine, its user error on your part. I have tested everything with docker and without all works great see other Issues and PR I show results

@shargon
Copy link
Member

shargon commented Jan 9, 2024

We will fix modules, or exes (or its already solved)

@shargon shargon closed this Jan 9, 2024
@shargon shargon deleted the fix-neo-cli branch January 9, 2024 06:30
@shargon shargon restored the fix-neo-cli branch January 9, 2024 06:30
@shargon shargon reopened this Jan 9, 2024
@shargon
Copy link
Member

shargon commented Jan 9, 2024

Sorry I confuse the pr hahaha

@cschuchardt88
Copy link
Member Author

cschuchardt88 commented Jan 9, 2024

solved already with this pR

@vncoelho vncoelho dismissed their stale review January 9, 2024 12:16

If not reverted for neo(which is not a good direction) this is needed for cli and gui

@shargon
Copy link
Member

shargon commented Jan 10, 2024

Still needed?

@cschuchardt88
Copy link
Member Author

@shargon
Yes

@shargon shargon merged commit 27df5ce into neo-project:master Jan 10, 2024
2 checks passed
@shargon shargon deleted the fix-neo-cli branch January 10, 2024 08:12
Jim8y added a commit to Jim8y/neo that referenced this pull request Jan 10, 2024
* master:
  Fixed asp.net core project (neo-project#3067)
  Updated BLS12_381 (neo-project#3074)
  avoid nonsense exception messages. (neo-project#3063)
  Removed `MyGet` (neo-project#3071)
  Updated unit-test (neo-project#3073)
  add hash verification for OnImport (neo-project#3070)
  Make public ReadUserInput (neo-project#3068)
  Removed asp.net core (neo-project#3065)
  Enforce Line Endings in `.editorconfig` (neo-project#3060)
  Remove some warnings (neo-project#3057)
  Fixed workflow  timeout-minutes (neo-project#3048)
  Fix: specify the error message (neo-project#3030)
  Removes `WebSocket`s from the network layer (neo-project#3039)
  set timeout for tests (neo-project#3046)
  Fix: Editconfig (neo-project#3023)
  Set project as nullable (neo-project#3042)
  Fix: fix equal (neo-project#3028)

# Conflicts:
#	src/Neo.CLI/CLI/MainService.cs
#	src/Neo.CLI/Settings.cs
#	src/Neo/ProtocolSettings.cs
Jim8y added a commit to Jim8y/neo that referenced this pull request Jan 12, 2024
* master:
  Made `MemoryStore` the default whithout `config.json` for `neo-cli` (neo-project#3085)
  Adding Devcontainer and link to codespace (neo-project#3075)
  Update & Consolidate nugets (neo-project#3083)
  Adding NNS to `neo-cli` (neo-project#3032)
  Add: add pull request template (neo-project#3081)
  Move to monorepo: Neo.Cryptography.BLS12_381 (neo-project#3077)
  Add: add a new verify result status code (neo-project#3076)
  Convert to Neo.Json and Neo.ConsoleService to `dotnet` standard 2.1 (neo-project#3044)
  Avoid IsExternalInit  (neo-project#3079)
  Clean usings (neo-project#3078)
  Fixed asp.net core project (neo-project#3067)
  Updated BLS12_381 (neo-project#3074)
  avoid nonsense exception messages. (neo-project#3063)
  Removed `MyGet` (neo-project#3071)
  Updated unit-test (neo-project#3073)
  add hash verification for OnImport (neo-project#3070)
  Make public ReadUserInput (neo-project#3068)
  Removed asp.net core (neo-project#3065)
  Enforce Line Endings in `.editorconfig` (neo-project#3060)

# Conflicts:
#	src/Neo.CLI/CLI/MainService.cs
@roman-khimov roman-khimov added this to the v3.7.0 milestone Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Aspnet dependencies problems
4 participants