-
Notifications
You must be signed in to change notification settings - Fork 1k
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
NeoVM integration #2970
NeoVM integration #2970
Conversation
@roman-khimov may you please take a look, i think your extraordinary experience in maintaining the monorepo neogo would be very helpful here. |
@lock9 we need to modify the workflow in order to test Neo.VM at the same time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C# packages/modules/projects/whatever are very different from Go packages, so it's hard for me to say anything here. IIUC it'll still be a separate dll (project?), but that should be OK if it has the appropriate code from the latest VM, builds, passes tests and all other components are using it instead of referencing the old repo.
@lock9 please look at https://github.com/neo-project/neo/blob/master/src/Directory.Build.props and remove those properties from the Edit: |
9f8f061
to
d315686
Compare
@Liaojinghui @shargon @roman-khimov Make a note that github actions has to add |
|
is |
Agreed. The previous name wasn't good |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have verified that the code has been ported without being modified except for the references, compiles, passes the unit tests and for my part it can be merged.
I think it is good to go, @shargon. But i also suggest to move submoules out of src to the root folder, if we agree on this, can be done after we complete the monorepo. And can you please archive the neo-vm repo, unpin it from the project and pin neo-express instead. |
Out src why? |
Not very serious reason, just make the the structure more clear. Just mention it, can ignore this if not agree. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
And another question from my side: starting from this PR NeoVM tests are being run by GitHub action's Test
job the same as the rest of the Neo tests on every PR, right?
Oh, I see the #2970 (comment), so it's OK.
<Project Sdk="Microsoft.NET.Sdk"> | ||
|
||
<PropertyGroup> | ||
<TargetFrameworks>netstandard2.1;net7.0</TargetFrameworks> | ||
<AllowUnsafeBlocks>true</AllowUnsafeBlocks> | ||
<Nullable>enable</Nullable> | ||
</PropertyGroup> | ||
|
||
</Project> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a question: why the <Copyright>
, <Description>
, <LangVersion>
and other description tags were removed? It is still a separate project, it would be nice to have some description metadata for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the previous content. I don't think there is a lot to reuse. The older description was just "Neo virtual machine":
<PropertyGroup>
<Copyright>2016-2023 The Neo Project</Copyright>
<Description>Neo virtual machine</Description>
<VersionPrefix>3.6.2</VersionPrefix>
<Authors>The Neo Project</Authors>
<TargetFrameworks>netstandard2.1;net7.0</TargetFrameworks>
<LangVersion>9.0</LangVersion>
<PackageTags>NEO;AntShares;Blockchain;Smart Contract;VM</PackageTags>
<PackageProjectUrl>https://github.com/neo-project/neo-vm</PackageProjectUrl>
<PackageLicenseExpression>MIT</PackageLicenseExpression>
<RepositoryType>git</RepositoryType>
<RepositoryUrl>https://github.com/neo-project/neo-vm.git</RepositoryUrl>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<Nullable>enable</Nullable>
<GenerateDocumentationFile>true</GenerateDocumentationFile>
</PropertyGroup>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The older description was just
At least there was some description, so it would be nice to port it. As far as Copyright
, Authors
and package-related tags (we can change the PackageProjectUrl
to https://github.com/neo-project/neo
. VM is still a separate package, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other packages don't have this information. I'm not against adding it, but I could argue that this may be done separately and to all the packages. None of the other projects have their own description or custom details.
Example:
Neo JSON:
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<ImplicitUsings>enable</ImplicitUsings>
<Nullable>enable</Nullable>
<PackageTags>NEO;JSON</PackageTags>
</PropertyGroup>
</Project>
RPC Server:
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<PackageId>Neo.Plugins.RpcServer</PackageId>
<RootNamespace>Neo.Plugins</RootNamespace>
</PropertyGroup>
</Project>
Neo Modules projects inherit the configuration from this file:
<?xml version="1.0" encoding="utf-8"?>
<Project ToolsVersion="15.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<PropertyGroup>
<VersionPrefix>3.6.2</VersionPrefix>
<TargetFramework>net7.0</TargetFramework>
<RootNamespace>Neo.Plugins</RootNamespace>
<Authors>The Neo Project</Authors>
<PackageTags>NEO;Blockchain</PackageTags>
<PackageProjectUrl>https://github.com/neo-project/neo-modules</PackageProjectUrl>
<PackageLicenseExpression>MIT</PackageLicenseExpression>
<RepositoryType>git</RepositoryType>
<RepositoryUrl>https://github.com/neo-project/neo-modules.git</RepositoryUrl>
</PropertyGroup>
<ItemGroup>
<None Include="config.json" Condition="Exists('config.json') And '$(IncludeSettingsFileOutput)' != 'False'">
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
<CopyToPublishDirectory>PreserveNewest</CopyToPublishDirectory>
</None>
</ItemGroup>
<ItemGroup>
<PackageReference Include="Neo" Version="3.6.2" />
</ItemGroup>
</Project>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good, let's just leave it as is, resolved.
Could we discuss this in another issue? I like the way it is today |
@AnnaShaleva lets focus on functionalities. minor issues can be skipped for now and fixed later in another pr. |
@shargon may you please check, review, and merge? |
No more commits from my last review, we can merge if @roman-khimov approve |
@roman-khimov please check and review |
I will transfer the issues from neo-vm and archive the neo-vm repository after merge this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lock9 “That’s one small step for man, one giant leap for mankind”. |
@lock9 take a look to the workflow please
|
Because of this? <TargetFrameworks>netstandard2.1;net7.0</TargetFrameworks> |
Afaik, .NET Standard is just a 'common interface' in .NET. I'll check what is causing the build to break. I don't think it's even possible to make a DLL from .NET Standard Libraries (I'm not sure). I'll check & update the workflow file to fix it. |
There were two problems. The dll wasn't being generated but that was a bug caused by conflicting properties. Fixed by #2989 |
* master: (30 commits) Set project as nullable (neo-project#3042) Fix: fix equal (neo-project#3028) Added README to packages (neo-project#3026) Nuget MyGet Fix (neo-project#3031) Add: print out the stack (neo-project#3033) fixed myget (neo-project#3029) Fixed MyGet Workflow (neo-project#3027) Package icons - hotfix (neo-project#3022) Nuget Package Icon & Symbols (neo-project#3020) Fix warning (neo-project#3021) Neo-node Migration (neo-project#2990) Remove unnecessary default seedlist (neo-project#2980) Fix Neo VM target frameworks (neo-project#2989) Update Neo.VM location in README.md (neo-project#2988) Migrating Neo VM (neo-project#2970) 3.6.2 (neo-project#2962) fix ut (neo-project#2959) Validate serialization during Contract deploy and Update (neo-project#2948) code optimization (neo-project#2958) check null scriptcontainer (neo-project#2953) ...
I am migrating the NeoVM project into the Neo repository.
Note: I've removed .NET Core 3.1 support from Neo VM. I don't see any reason to keep it, and it was breaking the build.
Related to #2972