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

diener doesn't find all crates in a repository #15

Closed
joao-paulo-parity opened this issue May 19, 2022 · 5 comments
Closed

diener doesn't find all crates in a repository #15

joao-paulo-parity opened this issue May 19, 2022 · 5 comments

Comments

@joao-paulo-parity
Copy link

joao-paulo-parity commented May 19, 2022

As corroborated by https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/1577990#L521 and reproducible by the steps of paritytech/pipeline-scripts#45 (comment), diener doesn't patch all crates in a repository.

As an example polkadot-node-core-pfv (https://github.com/paritytech/polkadot/blob/f8b668b7540eaac106765a978c17af533d261f8e/node/core/pvf/Cargo.toml) for which there was no Adding patch for polkadot-node-core-pfv log line in the GitLab job and it also doesn't show up in the patch entries as demonstrated by the diff of paritytech/pipeline-scripts#45 (comment). Outdated, see #15 (comment).

From cursory look at the code I think this is where the crates are detected, but I haven't dug deeper into that to know the cause of why some crates are missed.

https://github.com/bkchr/diener/blob/69ab0f76fb851968af8e493061cca84a2f3b1c5b/src/patch.rs#L236-L255

joao-paulo-parity added a commit to joao-paulo-parity/diener that referenced this issue May 19, 2022
cargo_metadata wasn't finding all the crates in a repository

paritytech#15
joao-paulo-parity added a commit to joao-paulo-parity/diener that referenced this issue May 19, 2022
cargo_metadata wasn't finding all the crates in a repository

paritytech#15
joao-paulo-parity added a commit to joao-paulo-parity/diener that referenced this issue May 19, 2022
cargo_metadata wasn't finding all the crates in a repository

paritytech#15
joao-paulo-parity added a commit to joao-paulo-parity/scripts that referenced this issue May 19, 2022
@bkchr
Copy link
Member

bkchr commented May 19, 2022

This is not a bug in diener. There is the cumulus-test-relay-validation-worker-provider crate that doesn't respect the patches. Your provided pr also doesn't solve this.

@bkchr
Copy link
Member

bkchr commented May 19, 2022

This crate creates itself a crate inside the OUT_DIR. It needs to respect the workspace patches. This crate was written by me, without all the magic. This should be fixed.

@joao-paulo-parity joao-paulo-parity changed the title diener doesn't patch all crates in a repository diener doesn't find all crates in a repository May 23, 2022
@joao-paulo-parity
Copy link
Author

Thanks for the feedback. I now realize something is amiss.

Seems like it was possible to overcome the problem of not all dependencies being patched (from https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/1577990#L2211) with #16 (https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/1579787#L2003), but it's not due to the reason I pointed out in the ticket's description; the case in point polkadot-node-core-pfv is found in both https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/1577990#L573 and https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/1579787#L951, so the problem is related to something else. I'll reevaluate it.

@joao-paulo-parity
Copy link
Author

I've compared the "Patching" sections of https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/1579869 (#16) vs https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/1577990 (diener v0.4.2). For most parts I didn't find noticeable differences except for "Patching polkadot into cumulus", so I'll provide a comparison below of https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/1579869#L889 (#16) vs https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/1577990#L519 (diener v0.4.2). Lines starting with - are packages which the latter did not detect.

I think the lines related to polkadot-overseer-gen and polkadot-overseer-gen-proc-macro can be ignored because they got renamed to orchestra and orchestra-proc-macro respectively (paritytech/polkadot@64f0948). Putting those changes aside there are lots of crates which diener v0.4.2 did not patch compared to #16. I didn't dig into the orchestra code yet but hope to do it soon.

index a8b864c..9ba5e87 100644
--- a/pr-16.txt
+++ b/diener-v0.4.2.txt
@@ -1,34 +1,18 @@
 bp-header-chain
-bp-kusama
 bp-message-dispatch
 bp-messages
-bp-millau
-bp-polkadot
 bp-polkadot-core
-bp-rialto
-bp-rialto-parachain
 bp-rococo
 bp-runtime
 bp-test-utils
-bp-token-swap
-bp-westend
 bp-wococo
 bridge-runtime-common
-erasure_coding_fuzzer
-finality-relay
 kusama-runtime
 kusama-runtime-constants
-messages-relay
 metered-channel
-millau-bridge-node
-millau-runtime
-orchestra
-orchestra-proc-macro
 pallet-bridge-dispatch
 pallet-bridge-grandpa
 pallet-bridge-messages
-pallet-bridge-token-swap
-pallet-shift-session-manager
 pallet-xcm
 pallet-xcm-benchmarks
 polkadot
@@ -67,6 +51,8 @@ polkadot-node-subsystem-test-helpers
 polkadot-node-subsystem-types
 polkadot-node-subsystem-util
 polkadot-overseer
+polkadot-overseer-gen
+polkadot-overseer-gen-proc-macro
 polkadot-parachain
 polkadot-performance-test
 polkadot-primitives
@@ -85,28 +71,11 @@ polkadot-test-malus
 polkadot-test-runtime
 polkadot-test-service
 polkadot-voter-bags
-relay-kusama-client
-relay-millau-client
-relay-polkadot-client
-relay-rialto-client
-relay-rialto-parachain-client
-relay-rococo-client
-relay-substrate-client
-relay-utils
-relay-westend-client
-relay-wococo-client
 remote-ext-tests-bags-list
-rialto-bridge-node
-rialto-parachain-collator
-rialto-parachain-runtime
-rialto-runtime
 rococo-runtime
 rococo-runtime-constants
 slot-range-helper
 staking-miner
-storage-proof-fuzzer
-substrate-relay
-substrate-relay-helper
 test-parachain-adder
 test-parachain-adder-collator
 test-parachain-halt

@bkchr
Copy link
Member

bkchr commented Jun 10, 2022

The proper solution is here: paritytech/cumulus#1344

@bkchr bkchr closed this as completed Jun 10, 2022
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 a pull request may close this issue.

2 participants