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

Revert "Removed asp.net core" #3066

Closed
wants to merge 1 commit into from
Closed

Conversation

shargon
Copy link
Member

@shargon shargon commented Jan 7, 2024

@shargon shargon requested a review from vncoelho January 7, 2024 22:46
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.

I will approve in a minute, I am just rebuilding my setup to ensure I did not made mistake when testing the error.

@cschuchardt88
Copy link
Member

Just wait on this

@cschuchardt88
Copy link
Member

cschuchardt88 commented Jan 7, 2024

Ill be adding to neo-cli instead, that will fix it. No rushed need for revert. If no release if due to out come this minute. Lets fix the problem 1st.

There isnt enough information. given for revert

@vncoelho
Copy link
Member

vncoelho commented Jan 7, 2024

The fact is that without neo-project/neo-modules@e9e7a07 the module does not build.
But even with it is produces error when running, missing some dependencies. (it just occurs on nodes with RpcServer)

I forced added rpcserver published dlls into client (everything built with same framework and same environment), and still some errors.

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.

With this broken all others PR can not be tested properly.

@cschuchardt88
Copy link
Member

Plus in this introduces a new problem

@cschuchardt88
Copy link
Member

The fact is that without neo-project/neo-modules@e9e7a07 the module does not build. But even with it is produces error when running, missing some dependencies. (it just occurs on nodes with RpcServer)

I forced added rpcserver published dlls into client (everything built with same framework and same environment), and still some errors.

YES THIS HAS TO DUE WITH neo-cli. and it will be fixed in a minute. Add this to neo-cli not neo.dll

@cschuchardt88
Copy link
Member

btw if test are broken then github action test should be more thorough

@vncoelho
Copy link
Member

vncoelho commented Jan 7, 2024

@cschuchardt88
Copy link
Member

Has to do with the host not targeting right framework

@vncoelho
Copy link
Member

vncoelho commented Jan 7, 2024

The fact is that without neo-project/neo-modules@e9e7a07 the module does not build. But even with it is produces error when running, missing some dependencies. (it just occurs on nodes with RpcServer)
I forced added rpcserver published dlls into client (everything built with same framework and same environment), and still some errors.

YES THIS HAS TO DUE WITH neo-cli. and it will be fixed in a minute. Add this to neo-cli not neo.dll

Sure, if there is a good solution.

Just do not merge other PRs until we solve this.
I created another one on neo-modules in case this one is merged.
If not, let's just close it.

@vncoelho
Copy link
Member

vncoelho commented Jan 7, 2024

Has to do with the host not targeting right framework

In the case I have here, as we discussed recently, we are targeting net7.0.
Building everything (neo-cli and modules) and publishing with FROM mcr.microsoft.com/dotnet/sdk:7.0.404-jammy

Running with FROM mcr.microsoft.com/dotnet/aspnet:7.0.14-jammy

@vncoelho
Copy link
Member

vncoelho commented Jan 7, 2024

I can try with another commands. The setup here is simple to modify, everything is one line command and dockerized.

@cschuchardt88
Copy link
Member

cschuchardt88 commented Jan 7, 2024

as in host I mean the one running the process. In this case its neo-cli . Is it the same for you?

@cschuchardt88
Copy link
Member

see #3067, close this.

@cschuchardt88
Copy link
Member

Plus don't forget 12-hour waiting period before merge....

@vncoelho
Copy link
Member

vncoelho commented Jan 7, 2024

Plus don't forget 12-hour waiting period before merge....

Plus, don't forget to push PRs that do not break everything...aehuahuea

@vncoelho vncoelho closed this Jan 7, 2024
@vncoelho
Copy link
Member

vncoelho commented Jan 7, 2024

The other PR fixed, @shargon

@vncoelho vncoelho deleted the revert-3065-remove-aspnetcore branch January 7, 2024 23:33
@shargon shargon restored the revert-3065-remove-aspnetcore branch January 8, 2024 06:31
@shargon shargon reopened this Jan 8, 2024
@shargon
Copy link
Member Author

shargon commented Jan 8, 2024

@vncoelho this works with neo-node and neo-gui, and future clients, because plugins are used by neo, but you choose @vncoelho

@shargon
Copy link
Member Author

shargon commented Jan 8, 2024

Has to do with the host not targeting right framework

The plugin is installed by the host, but is used by the framework, In my opinion, until we solve the dependency problem, we should revert it and find the good solution (not move the problem to another place)

@cschuchardt88
Copy link
Member

why not update the exe projects. It's only 3 lines of code.

@cschuchardt88
Copy link
Member

cschuchardt88 commented Jan 8, 2024

I don't understand why we are moving backward here. You have no good reasons. It's not like an release is around the corner.
You want things in sequences and PR #3067 does just that.

@shargon
Copy link
Member Author

shargon commented Jan 8, 2024

You have no good reasons.

The other PR is not a fix, it moves the problem, the real problem is load the dependencies from the plugin folder, this only moves the problem to another place

@cschuchardt88
Copy link
Member

cschuchardt88 commented Jan 8, 2024

You want things in sequences and PR #3067 does just that.

Plus we dont build GUI anymore for release. Since 3.6.0 No GUI In Release

@vncoelho
Copy link
Member

vncoelho commented Jan 8, 2024

@shargon ,I really appreciate your attention.
But I am not expert in these libraries and frameworks to decide.

In fact,maybe revert is good decision.
As I understood, neo-cli needs its dll during execution. And plugins need aspnet framework for compilation.

What is not clear to me is the image @cschuchardt88 is mentioning.

@Jim8y
Copy link
Contributor

Jim8y commented Jan 8, 2024

Wait a sec, if this is reverted, isn't it mean we still using core instead of standard? @cschuchardt88

@shargon
Copy link
Member Author

shargon commented Jan 8, 2024

Wait a sec, if this is reverted, isn't it mean we still using core instead of standard? @cschuchardt88

We are trying to find the good solution

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.

@shargon, too much instability on that other change. I am still not feeling safe to go.

The changes are requesting too much for the basic environment to build and run a simple client.

@cschuchardt88
Copy link
Member

Its RestServer plugin its autoloading stuff it shouldnt

@cschuchardt88
Copy link
Member

cschuchardt88 commented Jan 9, 2024

AFTER TESTING EVERYTHING THERE IS NO PROBLEMS!!! IT'S JUST USER ERROR!!!

@cschuchardt88
Copy link
Member

cschuchardt88 commented Jan 9, 2024

@vncoelho
image
image

@shargon
Copy link
Member Author

shargon commented Jan 9, 2024

We will fix exes or modules instead of neo

@shargon shargon closed this Jan 9, 2024
@shargon shargon deleted the revert-3065-remove-aspnetcore branch January 9, 2024 06:31
@cschuchardt88
Copy link
Member

@shargon we need it in both

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