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

ignore path dev deps in circular deps check #2471

Closed
wants to merge 5 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 20 additions & 42 deletions ci/order-crates-for-publishing.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,55 +21,33 @@ def load_metadata():
return json.loads(subprocess.Popen(
cmd, shell=True, stdout=subprocess.PIPE).communicate()[0])

# Consider a situation where a crate now wants to use already existing
# developing-oriented library code for their integration tests and benchmarks,
# like creating malformed data or omitting signature verifications. Ideally,
# the code should have been guarded under the special feature
# `dev-context-only-utils` to avoid accidental misuse for production code path.
#
# In this case, the feature needs to be defined then activated for the crate
# itself. To that end, the crate actually needs to depend on itself as a
# dev-dependency with `dev-context-only-utils` activated, so that the feature
# is conditionally activated only for integration tests and benchmarks. In this
# way, other crates won't see the feature activated even if they normal-depend
# on the crate.
#
# This self-referencing dev-dependency can be thought of a variant of
# dev-dependency cycles and it's well supported by cargo. The only exception is
# when publishing. In general, cyclic dev-dependency doesn't work nicely with
# publishing: https://github.com/rust-lang/cargo/issues/4242 .
#
# However, there's a work around supported by cargo. Namely, it will ignore and
# strip these cyclic dev-dependencies when publishing, if explicit version
# isn't specified: https://github.com/rust-lang/cargo/pull/7333 (Released in
# rust 1.40.0: https://releases.rs/docs/1.40.0/#cargo )
#
# This script follows the same safe discarding logic to exclude these
# special-cased dev dependencies from its `dependency_graph` and further
# processing.
def is_self_dev_dep_with_dev_context_only_utils(package, dependency, wrong_self_dev_dependencies):
# Cargo publish is fine with circular dev-dependencies if
# they are path deps.
# However, cargo still fails if deps are path deps with versions
# (this when you use `workspace = true`): https://github.com/rust-lang/cargo/issues/4242
# This function checks for these good and bad uses of dev dependencies.
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
Copy link
Member

@ryoqun ryoqun Aug 13, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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...

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

'dev-context-only-utils' in dependency['features'] and
'path' in dependency):
is_special_cased = True
if dependency['req'] != no_explicit_version:
# it's likely `{ workspace = true, ... }` is used, which implicitly pulls the
# version in...
wrong_self_dev_dependencies.append(dependency)
wrong_path_dev_dependencies.append(dependency)

return is_special_cased

def should_add(package, dependency, wrong_self_dev_dependencies):
related_to_solana = dependency['name'].startswith('solana')
self_dev_dep_with_dev_context_only_utils = is_self_dev_dep_with_dev_context_only_utils(
package, dependency, wrong_self_dev_dependencies
def should_add(package, dependency, wrong_path_dev_dependencies):
name = dependency['name']
related_to_solana = name.startswith('solana') or name.startswith('agave')
path_dev_dep = is_path_dev_dep(
package, dependency, wrong_path_dev_dependencies
)

return related_to_solana and not self_dev_dep_with_dev_context_only_utils
return related_to_solana and not path_dev_dep

def get_packages():
metadata = load_metadata()
Expand All @@ -78,12 +56,12 @@ def get_packages():

# Build dictionary of packages and their immediate solana-only dependencies
dependency_graph = dict()
wrong_self_dev_dependencies = list()
wrong_path_dev_dependencies = list()

for pkg in metadata['packages']:
manifest_path[pkg['name']] = pkg['manifest_path'];
dependency_graph[pkg['name']] = [
x['name'] for x in pkg['dependencies'] if should_add(pkg, x, wrong_self_dev_dependencies)
x['name'] for x in pkg['dependencies'] if should_add(pkg, x, wrong_path_dev_dependencies)
];

# Check for direct circular dependencies
Expand All @@ -95,13 +73,13 @@ def get_packages():

for dependency in circular_dependencies:
sys.stderr.write('Error: Circular dependency: {}\n'.format(dependency))
for dependency in wrong_self_dev_dependencies:
sys.stderr.write('Error: wrong dev-context-only-utils circular dependency. try: ' +
'{} = {{ path = ".", features = {} }}\n'
.format(dependency['name'], json.dumps(dependency['features']))
for dependency in wrong_path_dev_dependencies:
sys.stderr.write('Error: wrong circular dev dependency. try using a path dependency: ' +
'{} = {{ path = "$path_to_crate" }}\n'
.format(dependency['name'])
)

if len(circular_dependencies) != 0 or len(wrong_self_dev_dependencies) != 0:
if len(circular_dependencies) != 0 or len(wrong_path_dev_dependencies) != 0:
sys.exit(1)

# Order dependencies
Expand Down