aHash icon indicating copy to clipboard operation
aHash copied to clipboard

cyclic package dependency: package `ahash v0.7.4` depends on itself

Open bit-ranger opened this issue 2 years ago • 32 comments

Updating crates.io index error: cyclic package dependency: package ahash v0.7.4 depends on itself. Cycle: package ahash v0.7.4 ... which is depended on by hashbrown v0.11.2 ... which is depended on by indexmap v1.7.0 ... which is depended on by serde_json v1.0.64 ... which is depended on by wasm-bindgen v0.2.74 ... which is depended on by js-sys v0.3.51 ... which is depended on by getrandom v0.2.3 ... which is depended on by ahash v0.7.4 Error: Process completed with exit code 101.

more detail in github actions:

https://github.com/bit-ranger/chord/runs/2950729626?check_suite_focus=true

bit-ranger avatar Jun 30 '21 09:06 bit-ranger

I believe, though am not certain, that indexmap updating its version of hashmap to a more modern one that depends on ahash, is the cause

https://github.com/bluss/indexmap/commit/8e843a9c1d62cbc4892b71488c1f43685f994e2c

LLBlumire avatar Jun 30 '21 11:06 LLBlumire

A temporal fix for most people will be to force version 1.6 as a dependency by adding indexmap = "=1.6.2" to Cargo.toml's [dependencies] field, this should work even if IndexMap is only a subdependency (such as for example on h2 or serde_json).

vicky5124 avatar Jul 05 '21 14:07 vicky5124

A temporal fix for most people will be to force version 1.6 as a dependency by adding indexmap = "=1.6.2" to Cargo.toml's [dependencies] field, this should work even if IndexMap is only a subdependency (such as for example on h2 or serde_json).

Hi @vicky5124, thanks for your comment I just use this workaround and works as expected :)

pepoviola avatar Jul 08 '21 00:07 pepoviola

@bit-ranger I am confused as to why this is happening. getrandom's toml indicates it depends on js-sys only on wasm32 (which doesn't appear to be the arch in the linked build) additionally, it only depends on it if the js feature is enabled which aHash does not request.

Is the build above cross compiling and it's just not obvious? Is there some other package that is also depending on getrandom and Cargo is turning on the feature for both it and aHash in an effort to avoid duplication of the crate?

tkaitchuck avatar Jul 08 '21 06:07 tkaitchuck

@tkaitchuck Some other crate in the crate tree is enabling the js feature, and ahash is affected through feature unification.

Also crate resolving does not take into account the target arch. When building the crate tree, all crates for all targets are taken into account (not just the ones required for the active target arch).

lucacasonato avatar Jul 09 '21 13:07 lucacasonato

The chain works as follows:

  • aHash depends on getrandom for randomness.
  • getrandom depends on wasm-bindgen only if the js feature is enabled. (This is up to the user application, but it may be relevant)
  • wasm-bindgen has an optional dependency on serde_json which is disabled by getrandom. - So to enable this part of the cycle an app would have to also depend on wasm-bindgen and enable the serde feature. If an application does this it provides no value to getrandom other than shrinking the total binary size by unifying the crate with and without the feature.
  • serde_json has a dependency on indexmap only if the preserve_order feature is enabled (it is off by default). So to enable this part of the cycle the app would have to also depend on serde_json and enable preserve_order. Similar to the above this provides no value to wasm-bindgen.
  • indexmap depends on hashbrown but disables the ahash feature. So to complete the cycle an application would have to depend on hashbrown.

This cycle requires enabling 4 non-default features at different points. Of these js may be necessary, but the following 3 are an artifact of unification. Several of these are features are quite common, and are likely to be enabled in many applications. AHash could make getrandom optional, but it wouldn't actually break the cycle, it would just add one more feature required to complete it.

I think the most advantageous place to break the cycle would be as close to below getrandom as possible, because it should not be pulling in a bunch of extra dependencies, and serde support is not providing it any value.

One way to do this would be to refactor some of the wasm-bindgen code into a "core" package which could be depended on directly but which does not itself have the serde feature.

Another option would be to modify getrandom to support the js features without depending on js-sys or wasm-bindgen. I don't see any clear path that looks easy. Perhaps @josephlr or @newpavlov have some thoughts on this.

tkaitchuck avatar Jul 16 '21 03:07 tkaitchuck

@tkaitchuck thanks for the detailed writeup. As one of the maintainers of getrandom, I agree that this is a problem that should be fixed. Exactly who should fix it is a harder question. Thankfully, this bug is hard to hit.

One way to do this would be to refactor some of the wasm-bindgen code into a "core" package which could be depended on directly but which does not itself have the serde feature.

I like this idea, we could have a wasm-bindgen-core crate that contains only:

js-sys would then only depend on wasm-bindgen-core, the full wasm-bindgen would just reexport wasm-bindgen-core, and getrandom would just depend on wasm-bindgen-core and js-sys.

@alexcrichton would this be feasible?

I think the most advantageous place to break the cycle would be as close to below getrandom as possible, because it should not be pulling in a bunch of extra dependencies, and serde support is not providing it any value.

I completely agree, getrandom aims to have as few dependencies as possible. Other than all the wasm-bindgen stuff, we only depend cfg-if and "System ABI" crates like libc/wasi which just contain extern declarations. All of these are already dependencies of the standard library itself. The end goal is to have the functionality of getrandom in the standard library, but that's a ways off.

Another option would be to modify getrandom to support the js features without depending on js-sys or wasm-bindgen. I don't see any clear path that looks easy.

If this was possible, we would do it. We currently depend on 5 distinct imports:

  • wasm_bindgen_macro::wasm_bindgen
    • Mandatory: required on wasm32-unknown-unknown to bind to JS APIs
    • Similar to extern "C" on other platforms
  • wasm_bindgen::JsValue
    • Mandatory: used to interface with the wasm-bindgen glue code and refer to JS objects
  • js_sys::global()
    • Needed to get the global context (so we can start calling methods)
    • Not Mandatory: Could just copy the relevant code from js_sys into getrandom.
  • wasm_bindgen::JsCast
    • Only needed because we use js_sys::global()
  • js_sys::Uint8Array
    • Similarly, we could just copy the relevant code from js_sys

Also crate resolving does not take into account the target arch. When building the crate tree, all crates for all targets are taken into account (not just the ones required for the active target arch).

@bit-ranger if you use resolver = "2" does this remedy your issue?

josephlr avatar Jul 16 '21 07:07 josephlr

Oh dear this is quite the quandry, and sorry that wasm-bindgen is the cause of this! (I agree that wasm-bindgen is proabably the best place to fix this)

That being said I don't have the energy nowadays to redesign wasm-bindgen and/or implement major changes like refactoring the crates. I would recommend to basically avoid the serde features in wasm-bindgen. The serde features in wasm-bindgen are actually quite small and are probably best done manually through js-sys anyway.

alexcrichton avatar Jul 16 '21 14:07 alexcrichton

"Thankfully, this bug is hard to hit."

I've got it in two projects, and only through second-level dependencies except for serde_json.

StephanSchmidt avatar Aug 02 '21 10:08 StephanSchmidt

Unfortunately reqwest uses those features, which makes it quite hard to avoid.

s-panferov avatar Aug 16 '21 16:08 s-panferov

I also got hit by this in a project, after adding oauth2 (at least I think that was the cause).

Setting resolver = "2" made no difference.

Using indexmap = "~1.6.2" works as a workaround.

sindreij avatar Aug 23 '21 08:08 sindreij

oauth2 and sqlx together cause this issue. Using indexmap = "~1.6.2" does solve this.

StripedMonkey avatar Sep 13 '21 16:09 StripedMonkey

Hi,

Managed to run into this error, by having these 4 dependencies (all together):

[dependencies]
load_image = "2.15.1"
mongodb = "2.0.0"
reqwest = "0.11.4"
bevy = "0.5"

If I comment out any of these dependencies, it breaks cycle.

However, I did not managed to understand how exactly these 4 manage to get me into cycle.

Error for reference:

error: cyclic package dependency: package `ahash v0.7.4` depends on itself. Cycle:
package `ahash v0.7.4`
    ... which is depended on by `hashbrown v0.11.2`
    ... which is depended on by `indexmap v1.7.0`
    ... which is depended on by `serde_json v1.0.68`
    ... which is depended on by `wasm-bindgen v0.2.69`
    ... which is depended on by `js-sys v0.3.46`
    ... which is depended on by `getrandom v0.2.3`
    ... which is depended on by `ahash v0.7.4`

FylmTM avatar Sep 23 '21 13:09 FylmTM

I also have this issue, anyway to get arround this issue please?

error: cyclic package dependency: package `ahash v0.7.4` depends on itself. Cycle:
package `ahash v0.7.4`
    ... which is depended on by `hashbrown v0.11.2`
    ... which is depended on by `indexmap v1.7.0`
    ... which is depended on by `serde_json v1.0.64`
    ... which is depended on by `wasm-bindgen v0.2.74`
    ... which is depended on by `js-sys v0.3.51`
    ... which is depended on by `getrandom v0.2.3`
    ... which is depended on by `ahash v0.7.4`

videni avatar Oct 01 '21 02:10 videni

For those having sqlx as one of the dependencies, downgrading to 0.5.7 worked for me. sqlx@^0.5.8 requires indexmap@^1.7.0, which makes it impossible to pin it to ~1.6.2 as suggested above.

ngryman avatar Oct 18 '21 15:10 ngryman

Could we please get one of these packages to try to break the chain here? I just accidentally reintroduced this issue in SQLx 0.5.10 while upgrading dependencies. Keeping a dependency pinned at a single version forever is not a solution.

abonander avatar Jan 12 '22 09:01 abonander

As far as I understand now that 2021 is a thing you can use alternative feature resolvers as seen here: https://doc.rust-lang.org/cargo/reference/features.html#feature-resolver-version-2

Which may fix some of the cyclic dependency issues caused by feature unification. I may be wrong in my understanding, and I'm sure you guys are smarter about exactly what needs to happen, but AFAIK that was one of the big issues preventing a fix without some kind of pinning?

I've not kept up with this issue and it's been a while so I ma be wrong here.

StripedMonkey avatar Jan 12 '22 21:01 StripedMonkey

As described in https://github.com/tkaitchuck/aHash/issues/95#issuecomment-881242012, the issue here is wasm-bindgen depending on serde_json via its serde-serialize feature. To fix this either:

  1. Make sure you (and your dependencies) don't enable the serde-serialize feature (this might potentially require using the new resolver)
  2. Have wasm-bindgen stop depending on serde_json
  3. Have the wasm-bindgen folks expose a "core" crate that doesn't have any extra dependencies. If this existed we (i.e. getrandom) would use it.

To get traction on (2) or (3) it would probably be best to open an issue against wasm-bindgen rather than discussing this here.

josephlr avatar Jan 13 '22 21:01 josephlr

The problem is that it appears there won't be any movement on wasm-bindgen: https://github.com/tkaitchuck/aHash/issues/95#issuecomment-881503355

So the onus in my eyes falls onto one of the other packages in the chain. I would suggest maybe getrandom could generate its own bindings for the WASM target since it needs, what, one syscall?

abonander avatar Jan 13 '22 21:01 abonander

@abonander I don't think it's a practical option. To my best knowledge, we can not generate "bindings" to the WASM target, since it's literally unknown (i.e. wasm32-unknown-unknown), so we don't have any information about environment. In a certain sense, wasm-bindgen creates it's own custom target which is closely tied to the JS glue which it generates. Yes, we probably could hack something together using intimate knowledge of wasm-bindgen internals, but it will be a very fragile solution.

newpavlov avatar Jan 13 '22 21:01 newpavlov

I would suggest maybe getrandom could generate its own bindings for the WASM target since it needs, what, one syscall?

@abonander in my above comment I describe exactly what we need from wasm_bindgen to make getrandom work. I don't know of a way to fully drop the dependency and still have the crate function on the Web and Node.js. I've looked into this multiple times and I don't think it's possible. If anyone does know a way, we would be happy to accept patches.

I think if we want to see movement from wasm-bindgen someone should open an issue in their repo, create a PR officially deprecating the serde features, or create a proposal to create a wasm-bindgen-core crate.

josephlr avatar Jan 13 '22 22:01 josephlr

I am impacted by this as well and pinning indexmap to 1.6.2 result in deno_core being stuck in a year old dependency 0.110.0 :(

rubenfiszel avatar Apr 30 '22 08:04 rubenfiszel

I ran into this via feature unification inside a workspace and a transitive dependency of the goose load testing tool. That transitively depends on nanorand, which unconditionally enabled the js feature of getrandom. That seems to be fixed in the main branch of nanorand, another workaround was excluding the load testing toold from the cargo workspace.

jhorstmann avatar Jun 29 '22 07:06 jhorstmann

Now that there is movement on the wasm-bindgen side of things (https://github.com/rustwasm/wasm-bindgen/pull/3031), it seems like any dependency cycles can be resolved by not having crates depend (even optionally) on wasm-bindgen's serde-serialize feature.

I don't know if this means this issue can be closed or not. Fundamentally, the error is not in ahash, getrandom, js-sys, wasm-bindgen, indexmap, or hashbrown, but in other crates which activate the serde-serialize feature.

EDIT: I guess you could argue the issue is technically in wasm-bindgen, but they can't completely remove the feature without making a backwards incompatible change, so deprecating is the best they can do.

josephlr avatar Aug 30 '22 07:08 josephlr

With all due respect I fail to see how the blame lies in a project enabling a perfectly legitimate and probably necessary feature in their project. In my case I was using an open authentication and SQL library. Why is it my fault for enabling serialization features in a SQL and web auth library? Assigning blame because you're using a library provided feature is asinine at best. If the feature is broken it's not the end users fault for trying to use it.

The only reason this isn't completely crippling the rust ecosystem is that the dependency cycle is locked behind a feature flag. That's just a happy accident more than anything.

Fundamentally the problem is that there is a cyclic dependency. Not that other crates are trying to use supported components of these crates.

StripedMonkey avatar Aug 30 '22 13:08 StripedMonkey

I am having this issue and pinning indexmap's version isnt solving it Issue popped up once i added rust-s3 crate to one of my workspace members

x1qqq avatar Feb 16 '23 03:02 x1qqq

For anyone that might find this helpful, and pinning indexmap is not a solution, you can run cargo metadata > metadata.json to generate a file with all the crates/dependencies on your project. Then find all usages of wasm-bindgen with the feature flag serde-serialize (make sure it is not a dev dependency, as that might be okay). After finding which crates depend on wasm-bindgen with the feature flag enabled, you will need to update those packages to the latest version that does not use the serde-serialize feature in their dependencies.

If the packages in question, are still using the feature flag serde-serialize on their latest version, you can fix the crate on a forked repository, and patch your crates in your project to use the custom version from your branch.

tomasro27 avatar Feb 24 '23 04:02 tomasro27

Hope this might be useful for anyone, who is not interested in building wasm targets - one of the easy ways to break this circular dependency is by disabling wasm dependencies of getrandom package. You can fork and update getrandom(example) or simply add the patch below to your Cargo.toml:

[patch.crates-io]
# workaround to break the circular dependency for ahash(https://github.com/tkaitchuck/aHash/issues/95)
getrandom = { git = 'https://github.com/vladimir-dd/getrandom.git'}

vladimir-dd avatar Mar 24 '23 18:03 vladimir-dd

Ran into this when tried to build latest amethyst. Forcing indexmap to =1.6.2 somehow increased dependency count to 300 (from ~180) and also raised a few warnings, but worked. Thanks for the tip!

elenakrittik avatar Mar 30 '23 09:03 elenakrittik

@vladimir-dd You do not need to replace getrandom outright. Simply do not enable the js feature and instead use a custom implementation.

newpavlov avatar Mar 30 '23 13:03 newpavlov