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

refactor: removed duplicate and some useless optional dependencies #2520

Merged
merged 3 commits into from
Apr 27, 2020

Conversation

frol
Copy link
Collaborator

@frol frol commented Apr 24, 2020

  • tempdir is deprecated since merge into tempfile (I would not touch it would not it introduce the third version of rand crate into nearcore... we are stuck using two versions due to the epoch manager being rand-sensitive)
  • I integrated cargo-deny (see deny.toml config for the details about the whitelisted duplicates) to check for duplicate dependencies on CI (updated Buildkite pipeline); we may also use it to check licenses and advisories
  • I downgraded ansi_term to 0.11 since it is used in clap, and there are no major fixes in 0.12, so it makes sense to avoid duplication
  • I did not update base64 crate to 0.12 as it would cause duplication since actix-http uses 0.11 version
  • I did not update rocksdb to 0.14 since it requires some migration changes and I don't want to deal with it in this PR

@gitpod-io
Copy link

gitpod-io bot commented Apr 24, 2020

@frol frol force-pushed the refactor/cleanup-dependencies branch 5 times, most recently from 5318097 to 361bef8 Compare April 24, 2020 05:15
@frol frol force-pushed the refactor/cleanup-dependencies branch from 361bef8 to 9f2a532 Compare April 24, 2020 05:22
@frol frol force-pushed the refactor/cleanup-dependencies branch from 9f2a532 to 012de6a Compare April 24, 2020 10:27
@frol
Copy link
Collaborator Author

frol commented Apr 24, 2020

@ailisp I had added the following command to the "sanity checks" step on CI:

      if [ -e deny.toml ]; then
        wget "https://github.com/EmbarkStudios/cargo-deny/releases/download/$${CARGO_DENY_VERSION}/$${CARGO_DENY_PKG}.tar.gz"
        tar xzf "$${CARGO_DENY_PKG}.tar.gz"
        "$${CARGO_DENY_PKG}/cargo-deny" check bans
      fi

NOTE: I had to escape $ using $$ to force Buildkite to resolve environment variables in runtime.

UPD: Can we include cargo-deny installation step to the base image?

UPD2: Can we run pytest suite against PR?

@bowenwang1996
Copy link
Collaborator

Can we run pytest suite against PR?

Do you want to run all of them?

@ailisp
Copy link
Member

ailisp commented Apr 24, 2020

UPD: Can we include cargo-deny installation step to the base image?

Yes but not very urgent as this step is usually faster than the other steps so speed up this step but does not increase the user wait time

UPD2: Can we run pytest suite against PR?

Plan to do that soon. currently how nightly run is not very ideal. My plan:

@ailisp ailisp removed their assignment Apr 24, 2020
Copy link
Member

@ailisp ailisp left a comment

Choose a reason for hiding this comment

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

Nice cleanup! I was wondering how would different version of a crate coexist and the behave in the single neard binary

@frol
Copy link
Collaborator Author

frol commented Apr 24, 2020

@ailisp Different versions of the same crate just have different symbol names like rand__0_6_5__new (it is more packed than that, but they don't collide). The problem is when you try to pass a structure actix__0_8__Message to a function that expects actix__0_9__Message, and those messages in Rust were a little bit cryptic (they got improved AFAIK).

Some of the crates still are duplicated, but they are now explicitly whitelisted and documented in deny.toml and checked on CI, so we won't introduce new duplicates without noticing that.

@frol
Copy link
Collaborator Author

frol commented Apr 24, 2020

Do you want to run all of them?

Well, this is a massive dependencies update, so I would rather catch problems in PR, than on master.

Also, "nearlib test" was flaky on CI, so I just want to test it thoroughly, if possible.

@ailisp Do we have a facility to trigger pytest manually? (I mean, I can start it running on my laptop, but I guess, that might take ages to finish.

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.

This is great! Thank you!

@frol frol added the automerge label Apr 27, 2020
@nearprotocol-bulldozer nearprotocol-bulldozer bot merged commit 0796e59 into master Apr 27, 2020
@nearprotocol-bulldozer nearprotocol-bulldozer bot deleted the refactor/cleanup-dependencies branch April 27, 2020 18:34
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