Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

sp-utils => sc-utils #9677

Merged
4 commits merged into from
Sep 4, 2021
Merged

sp-utils => sc-utils #9677

4 commits merged into from
Sep 4, 2021

Conversation

gilescope
Copy link
Contributor

@gilescope gilescope commented Sep 2, 2021

It's not referenced by anything in primitives so shouldn't really live there. (Part of #9214 )

@gilescope gilescope added A0-please_review Pull request needs code review. B3-apinoteworthy C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Sep 2, 2021
client/consensus/common/src/metrics.rs Outdated Show resolved Hide resolved
client/transaction-pool/api/src/error.rs Outdated Show resolved Hide resolved
client/transaction-pool/api/src/lib.rs Outdated Show resolved Hide resolved
client/utils/src/lib.rs Outdated Show resolved Hide resolved
client/utils/src/metrics.rs Outdated Show resolved Hide resolved
client/utils/src/mpsc.rs Outdated Show resolved Hide resolved
client/utils/src/status_sinks.rs Outdated Show resolved Hide resolved
@bkchr
Copy link
Member

bkchr commented Sep 4, 2021

bot merge

@ghost
Copy link

ghost commented Sep 4, 2021

Waiting for commit status.

@ghost
Copy link

ghost commented Sep 4, 2021

Bot will approve on the behalf of @bkchr, since they are a team lead, in an attempt to reach the minimum approval count

@ghost ghost merged commit d73c37e into paritytech:master Sep 4, 2021
@athei
Copy link
Member

athei commented Sep 5, 2021

This PR broke the CI I think. It is not properly formatted and probably needs a polkadot companion. Curious why the CI in this PR did not catch this.

@joao-paulo-parity
Copy link
Contributor

@athei

It is not properly formatted

cargo-fmt is allowed to fail. According to git blame, it got changed some time ago (maybe it got relaxed due to formatting PRs and no one turned it back on after?). In any case it's very easy to change that, simply set allow_failure: false and it'll become mandatory again.

and probably needs a polkadot companion

packages are patched by name and that might have been the problem, since the dependency got renamed here. I could not intuit yet why it passed here, but not on master, since it should have patched the same way for both scenarios: here the PR does not have a companion, so it doesn't get patched; master is not a PR, thus it does not get patched, either.

@ordian
Copy link
Member

ordian commented Sep 5, 2021

It should've returned an error here:


considering it sets early return: .

@bkchr
Copy link
Member

bkchr commented Sep 5, 2021

It should've returned an error here:

This line is totally stupid and not even required for these checks.

The problem here is that we use patching and patched a non-existing crate, while the old sp-utils was still around (I still pointed to the old version in master).

The check needs to include checks for deleted crates.

This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants