Nadrieril / dhall-rust

Maintainable configuration files, for Rust users
Other
303 stars 27 forks source link

Propagate reqwest feature to serde_dhall #182

Closed basile-henry closed 3 years ago

rushsteve1 commented 3 years ago

From my testing this doesn't appear to have worked. Changing the dependency to...

serde_dhall = {
            git = "https://github.com/basile-henry/dhall-rust/",
            branch = "optional-reqwest",
            default-features = false
            }

Doesn't change the number of built dependencies, the size of the release binary, or the output of cargo tree.

basile-henry commented 3 years ago

Thanks for testing! I was building it from within serde_dhall directory and I couldn't see reqwest in the dependencies. I don't know if default-features works when building from the root of the project. At least cargo CLI doesn't allow the use of --no-default-features from there.

Nadrieril commented 3 years ago

Oh I thought this would work :( My reading of the cargo docs says it should. @rushsteve1 do you by any chance have another dependency that itself depends on dhall?

basile-henry commented 3 years ago

I replicated your way of getting serde_dhall @rushsteve1 and I believe it works:

test-bin via 🦀 v1.43.1
➜ cargo tree | grep reqwest
└── serde_dhall v0.7.1 (git+https://github.com/basile-henry/dhall-rust/?branch=optional-reqwest#b1f0ccaa03beb5dc7048fe8db7be258f9b4a24d8)
    ├── dhall v0.7.0 (git+https://github.com/basile-henry/dhall-rust/?branch=optional-reqwest#b1f0ccaa03beb5dc7048fe8db7be258f9b4a24d8)
    │   ├── abnf_to_pest v0.5.0 (git+https://github.com/basile-henry/dhall-rust/?branch=optional-reqwest#b1f0ccaa03beb5dc7048fe8db7be258f9b4a24d8)
    ├── dhall_proc_macros v0.5.0 (git+https://github.com/basile-henry/dhall-rust/?branch=optional-reqwest#b1f0ccaa03beb5dc7048fe8db7be258f9b4a24d8)

No mentions of the reqwest package in the cargo-tree output. And I have checked it is there when using the default features 😄

rushsteve1 commented 3 years ago

Here are my files and outputs

Cargo.toml

[dependencies]
serde = { version = "^1.0", features = ["derive"] }
rust-crypto = "^0.2"
# serde_dhall = "^0.7"
serde_dhall = {
            git = "https://github.com/basile-henry/dhall-rust/",
            branch = "optional-reqwest",
            default-features = false
            }
serde_json = "^1.0"
structopt = "^0.3"
dirs = "^3.0"

cargo tree | grep reqwest

├── serde_dhall v0.7.1 (https://github.com/basile-henry/dhall-rust/?branch=optional-reqwest#b1f0ccaa)
│   ├── dhall v0.7.0 (https://github.com/basile-henry/dhall-rust/?branch=optional-reqwest#b1f0ccaa)
│   │   ├── reqwest v0.10.8
│   │   ├── abnf_to_pest v0.5.0 (https://github.com/basile-henry/dhall-rust/?branch=optional-reqwest#b1f0ccaa)
│   ├── dhall_proc_macros v0.5.0 (https://github.com/basile-henry/dhall-rust/?branch=optional-reqwest#b1f0ccaa)
rushsteve1 commented 3 years ago

I figured it out, I had a mistake in my Cargo.toml. The current version by @basile-henry does work!

Nadrieril commented 3 years ago

Oh wait, I know why it failed in the first place: I had not released a version of dhall since @basile-henry's PR making the feature work, I had only released a version of serde_dhall ><

Nadrieril commented 3 years ago

I just released 0.7.2, properly this time. Should be fixed