search

paritytech / substrate

Substrate: The platform for blockchain innovators
Apache License 2.0
8.39k stars 2.65k forks source link

[tracking] Remove cyclic dependencies #4338

Open gnunicorn opened 4 years ago

gnunicorn commented 4 years ago

cargo cyclist currently shows 15 internal dependency cycles in our Cargo.lock, which prevents us from publishing on crates.io. These need to be fixed:

debris commented 4 years ago

I just run cargo-cyclist several times to see how my changes are affecting the dependency list and I've noticed that this tools output is not deterministic. i.e. It 5 subsequent runs on master branch can reported 18, 16, 16, 15 and 16 cyclic dependencies

gnunicorn commented 4 years ago

Yeah, I noticed that, too. I think it is because a) it builds an hashmap of the keys and deps, which it reads from - so that order is non-deterministic and then b) it stops at the first circle it finds per dependency. Which also means it currently doesn't detect multiple cycles... but if run in a different order, it might ...

On Tue, 10 Dec 2019, 17:38 Marek Kotewicz, notifications@github.com wrote:

I just run cargo-cyclist several times to see how my changes are affecting the dependency list and I've noticed that this tools output is not deterministic. i.e. It 5 subsequent runs on master branch can reported 18, 16, 16, 15 and 16 cyclic dependencies

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/paritytech/substrate/issues/4338?email_source=notifications&email_token=AAAJ4MCBYGRJ2KKWLCZAI7LQX7AZXA5CNFSM4JYR2FQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGP4UHA#issuecomment-564120092, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAJ4MBNWM44T5OHESK63MDQX7AZXANCNFSM4JYR2FQQ .

--

Wichtige Mitteilung

Diese Mitteilung wurde von Parity Technologies Deutschland GmbH kommuniziert, eine im Handelsregister des Amtsgerichtes Charlottenburg unter HRB 190583 B registrierte Gesellschaft mit beschränkter Haftung (GmbH). Die Geschäftsführerin der GmbH ist Frau Dr. Jutta Steiner. Der registrierte Geschäftssitz ist Glogauer Straße 6 in 10999 Berlin, Deutschland. 

Diese Mitteilung enthält Informationen welche vertraulich sind und welche eventuell die Vertraulichkeit der Rechtsberatung ("Anwaltsgeheimnis") berühren. Sie ist ausschließlich für den/die vorgesehenen Empfänger bestimmt. Wenn Sie nicht der/die beabsichtigte(n) Empfänger sind, benachrichtigen Sie bitte admin@parity.io mailto:admin@parity.io und löschen Sie diese Nachricht sofort.

Unsere Datenschutzrichtlinie, einschließlich die Art und den Umfang von personenbezogenen Daten, die wir erfassen, wie wir diese Daten erfassen und verarbeiten, an wen wir sie in Bezug auf die von uns angebotenen Dienste weitergeben dürfen, sowie bestimmte Rechte und Optionen, die Sie in dieser Hinsicht haben, finden Sie unter: https://www.parity.io/privacy/ https://www.parity.io/privacy/

-- This communication and any attachments are confidential.

gnunicorn commented 4 years ago

@bkchr any idea, how to tackles these ones?

  • [ ] sp-core (2.0.0): Cycle through sp-core(2.0.0) -> sp-runtime-interface(2.0.0) -> sp-runtime-interface-test-wasm(2.0.0) -> sp-core(2.0.0)
  • [ ] sp-trie (2.0.0): Cycle through sp-trie(2.0.0) -> sp-core(2.0.0) -> sp-runtime-interface(2.0.0) -> sp-runtime-interface-test-wasm(2.0.0) -> sp-io(2.0.0) -> sp-state-machine(2.0.0) -> sp-trie(2.0.0)
bkchr commented 4 years ago

The first one could be done by splitting the stuff that is used in sp-runtime-interface-test-wasm into a sub-crate of core. However, as sp-runtime-interface-test-wasm will never be published on crates.io and it is only a dev-dependency, why do we not just remove the dev-dependencies before we run cargo publish?

gnunicorn commented 4 years ago

I am against running cargo publish against local changes. In particular, I am not a fan of removing dev-dependencies as a hack, just to be able to publish, because it almost always means you publish something that then the one pulling from crates.io can't run the tests with ... this clearly is wrong, as running the tests is kinda the first thing you should be (able to) doing if the local version of your lib doesn't behave as expected.

On a higher/metalevel, this means we publish something to crates.io that actually isn't the code we show in our repository... which feels obviously wrong.

When putting time into this, why not do it right, instead?

bkchr commented 4 years ago

Who pulls stuff from crates.io to run tests with? I'm not even sure how to do this with cargo?

As always we are speaking here about a Cargo bug that should be fixed upstream and even the official "hack" for now is to prune all dev-dependencies: https://github.com/rust-lang/cargo/issues/4242#issuecomment-413250159

I'm totally against releasing stuff like sp-runtime-interface-test-wasm that will never be used by anyone, it will just clutter crates.io.

gnunicorn commented 4 years ago

Fair enough. From the regular lib world, I am used for that being a thing, but apparently this is a non-expectation within the rust-ecosystem (sadly, though). I am still not happy with publishing on a dirty checkout, but if this is part of the automatic release, procedure, well ... whatever...

Unfortunately, right now, the Cargo.lock also clumps them together, so finding only cycles that aren't in dev-dependencies (yes, we have those, too) is a bit tricky. Will investigate.

bkchr commented 4 years ago

Why not rewrite your cargo cylic to use the actual Cargo.toml files? Use cargo metadata to get all the files and build the dependency graph while ignoring dev-dependencies.

gnunicorn commented 4 years ago

That's one option... I am investigating.

gnunicorn commented 4 years ago

cargo metadata doesn't distinguish between dependencies and dev-dependencies... would have to read the Cargo.toml-files myself and build all the aliases and trees :( ...

bkchr commented 4 years ago

Yeah, that is what I meant :D But cargo metadata gives you all Cargo.toml files. If a dependency contains a package = "bla" you take this or you take the name given for the dependency. So, it should not be that hard to parse all of this.

bkchr commented 4 years ago

https://github.com/bkchr/proc-macro-crate/blob/721d5a9dbdedbfbb6d160a3e4a482226d8edb178/src/lib.rs#L125 probably not perfect, but a starting point

gnunicorn commented 4 years ago

hoping that the current version of cycle doesn't hide them from us, the only remaining non-dev-dependency-cycle appears to be:

  • [ ] sp-core (2.0.0): Cycle through sp-core(2.0.0) -> sp-runtime-interface(2.0.0) -> sp-core(2.0.0)

any takers? It looks like it might not be that easy to resolve...

edit: sp-core only uses runtime_interface::pass_by:: ... maybe we can move that somewhere more common for both, @bkchr ?

bkchr commented 4 years ago

This must be some dev-dependency, otherwise we could not compile this, because cargo would complain about cyclic dependencies. Everything we fixed in this issue here, were only stuff that was cyclic because of some dev-dependencies. Otherwise it would not have compiled before. And it is strictly required that sp-core includes runtime-interface to use the derive macros.

gnunicorn commented 4 years ago

ah, you are right, it is a sp-core is a dev-dependency in runtime_interface.

bkchr commented 4 years ago

If you would have moved the tests folder to the test crate of runtime interface, you would have removed this as well.