refactor(neard): decouple into individual crates #4292

miraclx · 2021-05-10T01:09:32Z

Dissociates neard into its binary and library constituents, with the binary depending on the library. Fixes #4233.

The intent is to have the binary named neard and the library named nearcore. However, because the workspace is itself nearcore, we can either move the library into the root or use a different name for either the workspace or the library itself.

neard/bin/Cargo.toml

matklad · 2021-05-11T10:28:27Z

However, because the workspace is itself nearcore

I feel that that's unfortunate. The root of our workspace is a crate, but this crate only contains integration tests, it isn't a binary or a library. That's surprising (most big projects are set-up differently) and obscures the actual dependencies between the things.

The typical setup is to make the root of the workspace a "virtual manifest", without the [package] section. See examples: rust-analyzer, rustc, substrate, firecracker. That way, you don't have "primary" crate, which often makes sense for large projects, as they have several entry points. cargo test in the workspace by default test everything. And you get somewhat more sane --feature flag behavior (with the second version of feature resolver, we should be able to even get rid of foo = [bar/foo, baz/foo] feature propagations).

Large project with non-virtual manifest typically have a top-level crate of substance, with src top-level directory which contains stuff, like cargo.

So, if we want to introduce a genuine nearcore library, I would suggest to get rid of tests-only top-level package first. Between "let's make top-level nearcore a real library" and "let's have a virtual manifest", I think "virtual manifest" has more benefits, as we do have several entry points (neard, params estimator, tools like genesis-populate).

I understand that this derails immediate work of extracing a library out of neard, but "fix workspace, then split neard" seems to be less work overall.

matklad · 2021-05-11T10:41:20Z

Here's a my suggested plan for virtual manifest:

check which tooling depends on the project layout and might break. Quick look at pipeline.yml shows that it should work with virtual manifest as is, but we might have some other automation.a
move tests from /test folder somewhere -- either to some of the existing packages (neard perhaps?) or into a new integration-tests package
re-read the features chapter of the cargo book, especially this section: https://doc.rust-lang.org/cargo/reference/features.html#resolver-version-2-command-line-flags
check that features in top-level toml will work when we switch to virtual manifest (with resolevr = "2" that should be the case)
add [workspace] resolver = "2" to the top-level toml
remove [package], [dependencies], [features] section from top-level toml ([profile] should stay)
check that everything works as expected.

Is this reasonable? Let's do a checklist to make sure we decide one way or another:

Do you agree that we should move to virtual manifest setup before introducing proper nearcore library? Check the box if you do!

(tagging a couple of folks who I know have opinions about code organization)

frol · 2021-05-11T15:45:31Z

I totally agree with the proposals from @matklad.

I don't expect any breakage from dropping [package] from the workspace Cargo.toml, and I even consider it is a great thing to do since it was quite confusing when you would call cargo test or cargo build which did nothing useful.

I suggest we move the current tests/ into nearcore (lib crate) if possible, otherwise fall back to neard (bin).

bowenwang1996 · 2021-05-11T17:20:36Z

Yeah agree with what @frol said. Don't expect anything to break with virtual manifest

frol

We are on the right track! Once CI is green, we can merge it

neard/Cargo.toml

…rror with Cargo.lock is missing (lib-mode)

chefsale

LGTM, just please take a look into the tests why are they failing. Seems after second restart they failed again, so maybe one third time? And ask @bowenwang1996 to maybe take a look as well.

ailisp

Looks good!

.buildkite/pipeline.yml

chain/client/Cargo.toml

neard/Cargo.toml

khorolets

LGTM!

olonho · 2021-05-27T06:41:01Z

Cargo.toml

-protocol_feature_cap_max_gas_price = ["near-primitives/protocol_feature_cap_max_gas_price", "neard/protocol_feature_cap_max_gas_price", "near-chain/protocol_feature_cap_max_gas_price"]
-
-# enable this to build neard with wasmer 1.0 runner
-# now if none of wasmer0_default, wasmer1_default or wasmtime_default is enabled, wasmer0 would be default


Those features are needed, why they are removed?

Good catch, I didn't notice that! I think it'd be good indeed to move this over to neard/Cargo.toml instead of just deleting.

That being said, I wouldn't block on that, as the following now works without feature forwarding:

cargo build -p neard --features node-runtime/wasmer1_default

@olonho Great catch!

It seems it should be in both nearcore (defined as node-runtime/wasmer1_default) and in neard (defined as nearcore/wasmer1_default).

stays consistent with previous behaviour should be better cleaned out after near#4325 is resolved

@ailisp

We're sort of in a weird spot as it is, we have features within `nearcore` that are required but can't be enabled if they depend on crates listed under `dev-dependencies` (bug in cargo: rust-lang/cargo#6915). Temporarily listing them under `dependencies`, fixed the CI testing failures we had while working on #4292 (same error @ailisp highlighted: #4333 (comment)), but as pointed out in #4331 (comment), this method should be out of the question. If we remove it, however, we can't work with the tests that depend on those features. This PR moves the _previously top-level_ tests into a new crate to have better dependency and feature handling as @matklad suggested > * [move tests from `/test` folder somewhere -- either to some of the existing packages (neard perhaps?) or into a new `integration-tests` package](#4292 (comment)) This would ensure we have dependencies like `testlib`, `runtime-params-estimator` and `restored-receipts-verifier` that are only needed for testing and depend on nearcore without cyclic dependency issues while having all features relevant to testing in order. The features within `neard` have been reduced to proxies to `nearcore` features, which partially resolves #4325 (though the intent for that has morphed a bit). This also makes for a cleaner dependency graph from `neard` perspective, removing extraneous dependencies that were previously required. I also noticed removed the `old_tests` feature that should've been removed along with the rest of it in #928. Updated some docs and code comments referencing the old `neard` path too.

miraclx requested review from frol and bowenwang1996 May 10, 2021 01:09

first pass: neard(lib), neard-bin(bin)

f42daf2

miraclx force-pushed the near(d)ecouple branch from 40886b9 to f42daf2 Compare May 10, 2021 02:08

bowenwang1996 reviewed May 10, 2021

View reviewed changes

neard/bin/Cargo.toml Outdated Show resolved Hide resolved

second pass: libneard(lib), neard(bin)

e649a2e

miraclx force-pushed the near(d)ecouple branch from 8b2b6a9 to e649a2e Compare May 11, 2021 11:45

miraclx added 6 commits May 17, 2021 07:08

reformat: use plain versioning syntax in manifest

e6b6687

third pass: nearcore(lib), neard(bin)

f82f0ef

move workspace tests into neard package

3837557

update CODEOWNERS

83c475b

Merge branch 'master' into near(d)ecouple

83b16e9

revert toml condensing of top-level pkg opt-levels

28c7b5a

frol approved these changes May 22, 2021

View reviewed changes

neard/Cargo.toml Outdated Show resolved Hide resolved

miraclx and others added 7 commits May 24, 2021 14:56

pin actix to beta version

a13e7a9

add final touches: skip denied dup deps, unpin funty

2f5c842

Merge branch 'master' into near(d)ecouple

ca75640

move stowaway resources to nearcore

184d7dc

fixed 'cargo release check' task on CI to be useful

c35c803

move neard/tests to nearcore/tests

0cbeb53

pinned actix_derive to 0.6.1-beta.1 in order to resolve compilation e…

a427a81

…rror with Cargo.lock is missing (lib-mode)

frol mentioned this pull request May 25, 2021

[BUG] Update the references to mainnet genesis file after a recent refactoring to nearcore near/docs#719

Closed

hotfix Cargo.lock

b13b9f0

frol marked this pull request as ready for review May 25, 2021 12:30

frol requested review from ailisp and chefsale as code owners May 25, 2021 12:30

frol requested review from matklad, mfornet, mhalambek, MaksymZavershynskyi, olonho, pmnoxx and SkidanovAlex as code owners May 25, 2021 12:30

chefsale approved these changes May 25, 2021

View reviewed changes

ailisp approved these changes May 25, 2021

View reviewed changes

matklad approved these changes May 26, 2021

View reviewed changes

.buildkite/pipeline.yml Show resolved Hide resolved

chain/client/Cargo.toml Show resolved Hide resolved

neard/Cargo.toml Show resolved Hide resolved

neard/Cargo.toml Show resolved Hide resolved

Looogarithm and others added 2 commits May 26, 2021 18:04

add testlib to feature list

4f6cda7

Merge branch 'master' into near(d)ecouple

a80bf97

miraclx requested review from bowenwang1996 and matklad May 26, 2021 15:39

khorolets approved these changes May 26, 2021

View reviewed changes

This was referenced May 26, 2021

Remove legacy near binary #4324

Closed

Remove nearcore features from neard Cargo.toml #4325

Closed

bowenwang1996 approved these changes May 27, 2021

View reviewed changes

olonho reviewed May 27, 2021

View reviewed changes

miraclx added 2 commits May 27, 2021 11:44

restore wasm feature flags in lib and bin

a7334ae

stays consistent with previous behaviour should be better cleaned out after near#4325 is resolved

Merge branch 'master' into near(d)ecouple

f0e48a2

miraclx force-pushed the near(d)ecouple branch from 4944e50 to f0e48a2 Compare May 27, 2021 10:45

olonho approved these changes May 27, 2021

View reviewed changes

frol added the automerge label May 27, 2021

near-bulldozer bot merged commit 7d16cf5 into near:master May 27, 2021

miraclx deleted the near(d)ecouple branch May 27, 2021 11:53

miraclx mentioned this pull request May 29, 2021

fix(neard): fixed failed CI tests due to features mismatches #4339

Merged

Longarithm mentioned this pull request Jun 4, 2021

Compilation error while running near-client test on nightly #4351

Closed

frol mentioned this pull request Aug 9, 2021

[Epic] Restructure nearcore project #1881

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(neard): decouple into individual crates #4292

refactor(neard): decouple into individual crates #4292

miraclx commented May 10, 2021 •

edited by frol

Loading

matklad commented May 11, 2021

matklad commented May 11, 2021 •

edited by bowenwang1996

Loading

frol commented May 11, 2021

bowenwang1996 commented May 11, 2021

frol left a comment

chefsale left a comment •

edited

Loading

ailisp left a comment

khorolets left a comment

olonho May 27, 2021

matklad May 27, 2021

frol May 27, 2021

refactor(neard): decouple into individual crates #4292

refactor(neard): decouple into individual crates #4292

Conversation

miraclx commented May 10, 2021 • edited by frol Loading

matklad commented May 11, 2021

matklad commented May 11, 2021 • edited by bowenwang1996 Loading

frol commented May 11, 2021

bowenwang1996 commented May 11, 2021

frol left a comment

Choose a reason for hiding this comment

chefsale left a comment • edited Loading

Choose a reason for hiding this comment

ailisp left a comment

Choose a reason for hiding this comment

khorolets left a comment

Choose a reason for hiding this comment

olonho May 27, 2021

Choose a reason for hiding this comment

matklad May 27, 2021

Choose a reason for hiding this comment

frol May 27, 2021

Choose a reason for hiding this comment

miraclx commented May 10, 2021 •

edited by frol

Loading

matklad commented May 11, 2021 •

edited by bowenwang1996

Loading

chefsale left a comment •

edited

Loading