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

toml fails to compile due to IndexMap #539

Closed
andrewbaxter opened this issue Apr 3, 2023 · 4 comments
Closed

toml fails to compile due to IndexMap #539

andrewbaxter opened this issue Apr 3, 2023 · 4 comments

Comments

@andrewbaxter
Copy link

andrewbaxter commented Apr 3, 2023

re #524

I think this issue also occurs on toml per: andrewbaxter/genemichaels#72

I wasn't able to reproduce it unfortunately, toml is a transitive dep. After patching in a fork of the parent dep though I saw both indexmap feature "default" and "std" appearing in the cargo tree separately:

│       ├── toml feature "default"
│       │   ├── toml v0.7.3
│       │   │   ├── serde feature "default" (*)
│       │   │   ├── indexmap feature "default"
│       │   │   │   └── indexmap v1.9.3
│       │   │   │       └── hashbrown feature "raw"
│       │   │   │           └── hashbrown v0.12.3
│       │   │   │       [build-dependencies]
│       │   │   │       └── autocfg feature "default"
│       │   │   │           └── autocfg v1.1.0
│       │   │   ├── serde_spanned feature "default"
│       │   │   │   └── serde_spanned v0.6.1
│       │   │   │       └── serde feature "default" (*)
│       │   │   ├── serde_spanned feature "serde"
│       │   │   │   └── serde_spanned v0.6.1 (*)
│       │   │   ├── toml_datetime feature "default"
│       │   │   │   └── toml_datetime v0.6.1
│       │   │   │       └── serde feature "default" (*)
│       │   │   ├── toml_datetime feature "serde"
│       │   │   │   └── toml_datetime v0.6.1 (*)
│       │   │   ├── toml_edit feature "default"
│       │   │   │   └── toml_edit v0.19.8
│       │   │   │       ├── serde feature "default" (*)
│       │   │   │       ├── indexmap feature "default" (*)
│       │   │   │       ├── indexmap feature "std"
│       │   │   │       │   └── indexmap v1.9.3 (*)
│       │   │   │       ├── serde_spanned feature "default" (*)
│       │   │   │       ├── serde_spanned feature "serde" (*)
│       │   │   │       ├── toml_datetime feature "default" (*)
│       │   │   │       └── winnow feature "default"
│       │   │   │           ├── winnow v0.4.1
│       │   │   │           └── winnow feature "std"
│       │   │   │               ├── winnow v0.4.1
│       │   │   │               └── winnow feature "alloc"
│       │   │   │                   └── winnow v0.4.1
│       │   │   └── toml_edit feature "serde"
│       │   │       ├── toml_edit v0.19.8 (*)
│       │   │       └── toml_datetime feature "serde" (*)
│       │   ├── toml feature "display"
│       │   │   └── toml v0.7.3 (*)
│       │   └── toml feature "parse"
│       │       └── toml v0.7.3 (*)

(Sorry if this is off base though, I'm not even sure how autocfg is included by indexmap - AFAICT it doesn't even appear in the Cargo.toml so I may be out of my depth)

@epage
Copy link
Member

epage commented Apr 3, 2023

Is that cargo tree output for a run that failed or succeeded?

While toml should also set it, you should be getting it through toml_edit. Note that all of this is dependent on the Cargo.lock so when helping someone through this, their cargo tree is important. Patching dependencies should not be needed. They can use cargo update to control specific dependencies, like cargo update -p toml_edit --precise 0.19.8.

@epage
Copy link
Member

epage commented Apr 3, 2023

Note that for toml, this is only a problem if

  • The preserve_order feature is enabled
  • (The parse and display features are disabled) or using an older toml_edit

@andrewbaxter
Copy link
Author

andrewbaxter commented Apr 3, 2023

That was a tree output from the successful build I ran locally. Arrg, okay, maybe I was reading the tree output wrong - I thought there were two indexmap deps included, one with the feature and one without. Please feel free to close since it seems like the issue isn't here, I do appreciate the help though. Not sure it's useful, but here's the reverse tree from the indexmap dep:

$ cargo tree -i indexmap --edges features
indexmap v1.9.3
├── indexmap feature "default"
│   ├── toml v0.7.3
│   │   ├── toml feature "default"
│   │   │   └── cargo-manifest v0.7.1
│   │   │       └── cargo-manifest feature "default" (command-line)
│   │   ├── toml feature "display"
│   │   │   └── toml feature "default" (*)
│   │   ├── toml feature "indexmap"
│   │   │   └── toml feature "preserve_order"
│   │   │       └── cargo-manifest v0.7.1 (*)
│   │   ├── toml feature "parse"
│   │   │   └── toml feature "default" (*)
│   │   └── toml feature "preserve_order" (*)
│   └── toml_edit v0.19.8
│       ├── toml_edit feature "default"
│       │   └── toml v0.7.3 (*)
│       └── toml_edit feature "serde"
│           └── toml v0.7.3 (*)
└── indexmap feature "std"
    └── toml_edit v0.19.8 (*)

I'll ask about the cargo tree, but I think they're only installing it (no project dir) so that may not be easy to get.

The cargo update -p toml_edit thing only works if that's a direct dependency, right? toml is a dep of cargo-manifest which is the direct dependency of genemichaels, so I forked cargo-manifest to update the toml dep to 0.7.3, which included the toml_edit package on it's own. cargo-manifest (patched from 0.5.6) has:

toml = { version = "0.7.3", features = ["preserve_order"] }

So I think the parse and display features should be included (not disabling default features).

@andrewbaxter
Copy link
Author

andrewbaxter commented Apr 3, 2023

Okay, so to summarize (for my own sake)

  1. The original build was with toml-0.5.10. This pulled in indexmap-1.9.3. toml-0.5.10 doesn't touch the indexmap features... indexmap-1.9.3 seems to have std on by default but maybe that's wrong.
  2. If I update to toml-0.7.3, that includes toml_edit which should set the std flag on indexmap. toml itself also includes indexmap, with only the default features, but I guess cargo merges deps keeping the union of features even if their features are different (based on this assumption I guess cargo build = spurious errors, prevent cargo from merging the same dependency with different features rust-lang/cargo#4323 ).

So updating to 0.7.3 should be sufficient...

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

No branches or pull requests

2 participants