ignore path dev deps in circular deps check

[kevinheavey]

Problem

cargo publish is fine with circular dev-dependencies if they are just path dependencies. One place where the need for such dependencies arises is doctests: if you pull a crate out of solana-program, its doc examples may still use other parts of solana-program

Summary of Changes

Update order-crates-for-publishing.py to special case all path dev-dependencies that don't specify a version. This replaces the previous check that allowed only deps where path = "." and feautres included dev-context-only-utils (ref: solana-labs#32432)

Fixes #2428

[ryoqun]

nit: update the local variable as well to path_dev_dep

[ryoqun]

for future reference, i think leaving the upstream bug issue like this could be helpful..:

# Cargo publishes fine with circular dev-dependencies if
# they are path deps without versions
# However, cargo still fails if deps are path deps with versions (this happens by the use of `workspace = true`): https://github.com/rust-lang/cargo/issues/4242
# This function checks for these good and bad uses of dev dependencies.

(reformat and/or reword the above. it's just a draft...)

[ryoqun]

is there actual doctest code which is blocked by this pr? Depending on the context, i wonder this self-dependency check could also be relaxed or the new comment should be adjusted to match with what the new code will be checking.

[ryoqun]

btw, relaxing this might involve to update the actual circular check as well, which might be bothersome. as the whole script itself is kind of some nice-to-have thing in ci and considering the unlikeliness of crate A -> path dev dep -> crate B -> path dev dep -> crate C -> path dev dep with version -> crate A, I think just commenting about incompleteness is enough...

[kevinheavey]

is there actual doctest code which is blocked by this pr?

Yes, for example in this PR I had to convert an example to text because of the circular dep check: e767309

[ryoqun]

thanks for sharing the failing example.

so, it looks like this check (dependency['name'] == package['name'] and) should be removed?

$ git reset --hard e7673099ca7d159e603f7288a351234d5634a1fa
git revert e7673099ca7d159e603f7288a351234d5634a1fa

$ ./ci/order-crates-for-publishing.py 
+ exec cargo +1.78.0 metadata --no-deps --format-version=1
Error: Circular dependency: solana-pubkey <--> solana-pubkey
Error: Circular dependency: solana-program <--> solana-pubkey

$ git checkout kevinheavey/allow-path-dev-deps ./ci/order-crates-for-publishing.py
Updated 1 path from 4560e6bb3ef

$ ./ci/order-crates-for-publishing.py
+ exec cargo +1.78.0 metadata --no-deps --format-version=1
Error: Circular dependency: solana-program <--> solana-pubkey

that makes my previous comment (https://github.com/anza-xyz/agave/pull/2471/files#r1714630153) valid and

... I think just commenting about incompleteness is enough...

this is my 2 cents. so, just add something like this below:

# Check for direct circular dependencies.
# Note that this check isn't exhaustive because path dependencies can be excluded.
circular_dependencies = set()
for package, dependencies in dependency_graph.items():

btw, this is the reason i made the original checks overly restricted, barely white-listing the specific dcou use.

[ryoqun]

well, i actually needed code changes like this, not just with removing the self-dep check...

$ git diff
diff --git a/ci/order-crates-for-publishing.py b/ci/order-crates-for-publishing.py
index fd763472934..a478a35200a 100755
--- a/ci/order-crates-for-publishing.py
+++ b/ci/order-crates-for-publishing.py
@@ -30,10 +30,9 @@ def is_path_dev_dep(package, dependency, wrong_path_dev_dependencies):
     no_explicit_version = '*'
     is_special_cased = False
     if (dependency['kind'] == 'dev' and
-        dependency['name'] == package['name'] and
         'path' in dependency):
         is_special_cased = True
-        if dependency['req'] != no_explicit_version:
+        # move the long dcou bla bla comment here
+        if dependency['req'] != no_explicit_version and dependency['name'] == package['name'] and 'dev-context-only-utils' in dependency['features']:
             # it's likely `{ workspace = true, ... }` is used, which implicitly pulls the
             # version in...
             wrong_path_dev_dependencies.append(dependency)

so, moving the removed comment and restoring the variable name wrong_self_dev_dependencies might be needed?

[ryoqun]

@kevinheavey sorry for delayed review... and, thanks for working on this. if you have some time, could you polish this pr?

[kevinheavey]

Ah I realise now that the is_path_dev_dep function doesn't do what I want - I want to whitelist all path dev deps from the circular dep check and that is not what this change achieves

[kevinheavey]

Closing in favour of #2578