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

[chore] Separated near-primitives dependencies into features #11597

Merged
merged 11 commits into from
Jun 20, 2024

Conversation

akorchyn
Copy link
Contributor

This pull request decreases near-primitive dependencies count even more by separating part of functionality into features.

I have created three new features:

  • rand - extracts rand, rand-chacha crates from the compilation. This is common practice to split by this type.
  • solomon - extracts reed-solomon-erasure, libc, lru, and some outdated subdeps (e.g. hashbrowns)
  • clock - removes tokio from dependency list that is nice :)

Moreover, I have featured gated rand in near-crypto following the same logic :)

Final results: near-primitives have 107 dependencies (-23) using a feature-less version

@akorchyn akorchyn requested a review from a team as a code owner June 17, 2024 17:16
@akorchyn akorchyn requested a review from wacban June 17, 2024 17:16
@wacban
Copy link
Contributor

wacban commented Jun 18, 2024

@nagisa Can you have a look?

@wacban wacban requested a review from nagisa June 18, 2024 09:36
Copy link
Collaborator

@nagisa nagisa left a comment

Choose a reason for hiding this comment

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

In principle LGTM, some nits inline.

.gitignore Outdated Show resolved Hide resolved
Cargo.lock Outdated Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
core/crypto/Cargo.toml Outdated Show resolved Hide resolved
where
R: Rng + ?Sized,
{
// Shuffling shard ids to avoid a bias towards lower ids, see [`ShuffledShardIds`]. We
Copy link
Collaborator

Choose a reason for hiding this comment

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

FWIW this looks very much like business logic that has no business living in near-primitives in the first place. We should figure out an effective way to prevent engineers from adding such code to this crate in the long run, otherwise the eventual end state is going to be the same as it was just recently...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. Though not sure how to prevent that ... My only thought that code review + eventually we could make primitives quite thin and limit by amount of deps 👍🏻

core/time/Cargo.toml Outdated Show resolved Hide resolved
core/time/src/lib.rs Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jun 18, 2024

Codecov Report

Attention: Patch coverage is 94.92386% with 20 lines in your changes missing coverage. Please review.

Project coverage is 71.49%. Comparing base (776e86a) to head (5be36a2).
Report is 2 commits behind head on master.

Files Patch % Lines
core/time/src/serde.rs 87.30% 0 Missing and 16 partials ⚠️
core/time/src/clock.rs 97.20% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master   #11597   +/-   ##
=======================================
  Coverage   71.48%   71.49%           
=======================================
  Files         788      790    +2     
  Lines      160921   160950   +29     
  Branches   160921   160950   +29     
=======================================
+ Hits       115034   115070   +36     
+ Misses      40864    40858    -6     
+ Partials     5023     5022    -1     
Flag Coverage Δ
backward-compatibility 0.23% <0.00%> (+<0.01%) ⬆️
db-migration 0.23% <0.00%> (+<0.01%) ⬆️
genesis-check 1.36% <10.17%> (+<0.01%) ⬆️
integration-tests 37.70% <65.73%> (+0.06%) ⬆️
linux 68.91% <94.41%> (+<0.01%) ⬆️
linux-nightly 70.98% <94.92%> (+0.05%) ⬆️
macos 52.30% <83.50%> (+0.99%) ⬆️
pytests 1.59% <11.67%> (+<0.01%) ⬆️
sanity-checks 1.39% <10.77%> (+<0.01%) ⬆️
unittests 66.14% <93.65%> (-0.01%) ⬇️
upgradability 0.28% <0.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@akorchyn
Copy link
Contributor Author

akorchyn commented Jun 18, 2024

@nagisa, I don't really understand about lychee lints. Did I break it? It has a bit weird output log, so I don't really follow what's broken. Could you please point it out to me?

Passed :)

Copy link
Collaborator

@nagisa nagisa left a comment

Choose a reason for hiding this comment

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

needs rebase/merge for the conflict.

Cargo.toml Show resolved Hide resolved
@@ -264,7 +257,9 @@ near-state-viewer = { path = "tools/state-viewer", package = "state-viewer" }
near-store = { path = "core/store" }
near-telemetry = { path = "chain/telemetry" }
near-test-contracts = { path = "runtime/near-test-contracts" }
near-time = { path = "core/time" }
near-time = { path = "core/time", default-features = false, features = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note: enabling features here means that they cannot be disabled in any of the dependencies anymore, so e.g. if there is a published crate that depends on near-time but doesn't need the serde feature then it will be enabling that feature superfluously. Not a big deal as far as I am concerned, but thought you should be aware of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, but I would keep it as it is for now. I think it might be a good pr to feature-gate serde across repo, but not for this PR.

core/primitives/Cargo.toml Outdated Show resolved Hide resolved
core/primitives/tests/crate-limit-test.rs Outdated Show resolved Hide resolved
auto-merge was automatically disabled June 19, 2024 11:07

Head branch was pushed to by a user without write access

@nagisa nagisa enabled auto-merge June 19, 2024 12:48
@nagisa nagisa disabled auto-merge June 20, 2024 12:51
@nagisa nagisa enabled auto-merge June 20, 2024 12:51
@nagisa nagisa added this pull request to the merge queue Jun 20, 2024
Merged via the queue into near:master with commit 04d2b4f Jun 20, 2024
28 of 29 checks passed
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.

None yet

3 participants