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

fix(core): remove actix dependency from near-primitives #2511

Merged
merged 20 commits into from
Apr 29, 2020

Conversation

lexfrl
Copy link
Contributor

@lexfrl lexfrl commented Apr 21, 2020

The PR follows the very specific goal - to remove actix dependency from near-primitives.

The general approach here is to move in small iterations to bring us closer to #2433

@gitpod-io
Copy link

gitpod-io bot commented Apr 21, 2020

@lexfrl lexfrl force-pushed the fckt/extract-actix-from-primitives branch from 3c916d3 to 130ebbc Compare April 21, 2020 18:12
@lexfrl lexfrl changed the title fix: extract Actix dependency from near-primitives fix: remove actix dependency from near-primitives Apr 21, 2020
@lexfrl lexfrl marked this pull request as ready for review April 21, 2020 21:29
Copy link
Contributor

@MaksymZavershynskyi MaksymZavershynskyi left a comment

Choose a reason for hiding this comment

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

See the comment

env/Cargo.toml Outdated
@@ -0,0 +1,9 @@
[package]
Copy link
Contributor

Choose a reason for hiding this comment

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

This crate is essentially a place where we dump stuff that we don't know where else to put. I don't think root of nearcore is a good place for it and I think it should have a more descriptive name.

In general, instead of extracting actix from the primitives crate we should be extracting stuff used by the runtime into finer grained crates, leaving everything else behind in the primitives crate. It requires revisiting every type from primitives used by the runtime and deciding which category of primitives it better represents. It is a much harder thing to do than just cutting out actix from dependencies, but it actually what we want to do -- we want to break monolithic primitives crate into smaller but meaningful crates.

Copy link
Collaborator

@frol frol left a comment

Choose a reason for hiding this comment

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

I agree with Max, this new crate works around the dependency issue, but it has a completely non-descriptive name. Given we cannot come up with a better name, I consider it is an indicator that we need to think more about the proper separation.

I also suggest we start working towards a better project structure and only create the new crates following some schema, e.g. there is one suggested here: #1881

@frol
Copy link
Collaborator

frol commented Apr 21, 2020

I believe that the best documentation is a self-descriptive source code. I feel that there is some room for improvement in nearcore in this direction, so whatever opportunity we have with refactoring, we should take it to improve things.

Copy link
Collaborator

@bowenwang1996 bowenwang1996 left a comment

Choose a reason for hiding this comment

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

I agree with Max and Frol. env is quite confusing.

@lexfrl
Copy link
Contributor Author

lexfrl commented Apr 22, 2020

Thank you for your feedback, folks.

I agree with Max, this new crate works around the dependency issue, but it has a completely non-descriptive name. Given we cannot come up with a better name, I consider it is an indicator that we need to think more about the proper separation.

You're right, this is hard to come up with a good name for this (but it's better the to extract this from near-primitives). The goal of this change is not bring a perfect solution.

What's makes up this crate unique:

  • by type: it's a library
  • by functionality: it accumulates functions which alters globals
  • by usage: used only by binaries (main + tests) and not libraries

Keeping in mind these properties I don't see a category which suits all properties listed (in #1881 (comment)):

Maybe the best approach would be just having in in a neard category, but expose it as lib with name neard-env which would clearly symbolize that it's just a collection of tools which alters env in which neard executes.
WDYT?

PS: Why just not expose these functions from neard? The problem is that neard depend on the entire project - so to use these functions you'll have to compile everything.

@frol
Copy link
Collaborator

frol commented Apr 22, 2020

We should never consider making library-crate dependent on neard.

env and neard-env does not help me to make any guess about the type of functionality this crate may expose. near-actix-helpers, near-tracing-helpers, near-testlib-helpers (though, if we use those helpers outside of tests, it is not a good name), ...

I think, libraries/helpers/ is a good candidate to score such crates

@MaksymZavershynskyi
Copy link
Contributor

Overall, the goal is to be able to publish runtime and compile it to Wasm. We should not be extracting stuff from primitives until it becomes publishable. Our crates should be as semantic as possible, having a crate that represents "stuff that does not compile to Wasm" is not good. Instead we should be extracting crates from primitives into finer-grained semantic crates. E.g. the way we extracted crypto and runtime-configs. Runtime should not be linked against something it does not care about, like network.rs or sharding.rs.

However, that said, this is probably a huge effort which would require too much refactoring that would delay our progress with testing harness, so let's create an issue for it right now, and link runtime against the primitives crate (with near-actix-helpers, near-tracing-helpers, near-testlib-helpers extracted).

I think, libraries/helpers/ is a good candidate to score such crates

I suggest put it in utils folder, almost every crate in our infra is a library, so no reason to have libraries folder. We also have test-utils already.

@frol
Copy link
Collaborator

frol commented Apr 23, 2020

@nearmax In #1881, I analyzed the root folder of nearcore project. There are tons of files and folders, so I categorized the folders into:

  • "neard" (we need to have the main entry-point to the project visible)
  • "tools" (near-loadtester, near-state-viewer, near-genesis)
  • "libraries" (which has subcategories: "core", "chain", "runtime", "test-utils", and "helpers" [though "utils" is also fine])

We have a ton of other files in the root of nearcore, which we cannot move, so it makes sense to shrink the scope where we can, so I think "libraries" folder is really helpful.

However, that said, this is probably a huge effort which would require too much refactoring that would delay our progress with testing harness

Agree, it is the tradeoff we want to take now to reach the goal. I think even this straightforward refactoring of extracting obviously non-related parts into separate crates will create a precedent, and make the future development cleaner.

@lexfrl
Copy link
Contributor Author

lexfrl commented Apr 23, 2020

IMO, on the top level, it make sense to have the complete components categorized by their role in the system, but not by the type of their interface.

@lexfrl lexfrl force-pushed the fckt/extract-actix-from-primitives branch from 4611259 to 1a0c46a Compare April 23, 2020 14:36
@lexfrl lexfrl force-pushed the fckt/extract-actix-from-primitives branch from 1a0c46a to 0382fbd Compare April 23, 2020 14:38
@lexfrl
Copy link
Contributor Author

lexfrl commented Apr 23, 2020

Addressed concerns
fdf9633
@nearmax @frol @bowenwang1996

utils/actix/Cargo.toml Show resolved Hide resolved
Copy link
Collaborator

@evgenykuzyakov evgenykuzyakov left a comment

Choose a reason for hiding this comment

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

Please add comments to the methods you moved from the test_utils, so we don't use them in prod. I'd suggest rename new in the Account to something else, to avoid it being used in production.

core/primitives/src/account.rs Outdated Show resolved Hide resolved
core/primitives/src/transaction.rs Outdated Show resolved Hide resolved
test-utils/testlib/src/runtime_utils.rs Outdated Show resolved Hide resolved
@lexfrl lexfrl assigned lexfrl and unassigned MaksymZavershynskyi Apr 28, 2020
@lexfrl
Copy link
Contributor Author

lexfrl commented Apr 28, 2020

I realized that having test_utils module in near-* crates is a common practice for today. I restored the core/primitives/src/test_utils.rs.

@lexfrl lexfrl requested review from evgenykuzyakov and frol April 28, 2020 23:27
@lexfrl lexfrl force-pushed the fckt/extract-actix-from-primitives branch from 518f9eb to 9309fd7 Compare April 29, 2020 00:55
@frol frol added the automerge label Apr 29, 2020
@frol frol changed the title fix: remove actix dependency from near-primitives fix(core): remove actix dependency from near-primitives Apr 29, 2020
@nearprotocol-bulldozer nearprotocol-bulldozer bot merged commit 650ac70 into master Apr 29, 2020
@nearprotocol-bulldozer nearprotocol-bulldozer bot deleted the fckt/extract-actix-from-primitives branch April 29, 2020 11:32
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.

5 participants