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

Upgrade dotenv to 0.15 [blocked on change to test suite] #2223

Closed
wants to merge 2 commits into from

Conversation

notriddle
Copy link

Trying to cargo install diesel_cli gave me compiler failures for error_chain and dotenv. I'm using a current nightly compiler.

This can be fixed by using a newer version of dotenv.

@notriddle
Copy link
Author

notriddle commented Nov 18, 2019

Nightly-to-stable regression reported upstream. rust-lang/rust#66508

Copy link
Member

@weiznich weiznich left a comment

Choose a reason for hiding this comment

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

I think we do not need to pinpoint the version that much.

diesel/Cargo.toml Outdated Show resolved Hide resolved
diesel_derives/Cargo.toml Outdated Show resolved Hide resolved
diesel_migrations/Cargo.toml Outdated Show resolved Hide resolved
diesel_tests/Cargo.toml Outdated Show resolved Hide resolved
diesel_tests/Cargo.toml Outdated Show resolved Hide resolved
examples/postgres/getting_started_step_2/Cargo.toml Outdated Show resolved Hide resolved
examples/postgres/getting_started_step_3/Cargo.toml Outdated Show resolved Hide resolved
examples/sqlite/getting_started_step_1/Cargo.toml Outdated Show resolved Hide resolved
examples/sqlite/getting_started_step_2/Cargo.toml Outdated Show resolved Hide resolved
examples/sqlite/getting_started_step_3/Cargo.toml Outdated Show resolved Hide resolved
@notriddle
Copy link
Author

Yeah, we can probably afford a wider version range.

@weiznich weiznich requested a review from a team November 19, 2019 10:11

[dependencies]
assert_matches = "1.0.1"
chrono = { version = "0.4" }
diesel = { path = "../diesel", default-features = false, features = ["quickcheck", "chrono", "uuidv07", "serde_json", "network-address", "numeric", "with-deprecated"] }
diesel_migrations = { version = "1.4.0" }
diesel_infer_schema = { version = "1.4.0" }
dotenv = ">=0.8, <0.11"
dotenv = ">=0.8, <=0.15"
Copy link
Member

Choose a reason for hiding this comment

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

This breaks the tests even back on older compiler versions with the following error:

error[E0659]: `error_chain` is ambiguous (derive helper attribute vs any other name)
  --> /home/weiznich/.cargo/registry/src/github.com-1ecc6299db9ec823/dotenv-0.10.1/src/lib.rs:23:40
   |
23 | #[cfg_attr(not(feature = "backtrace"), error_chain(backtrace = "false"))]
   |                                        ^^^^^^^^^^^ ambiguous name
   |
note: `error_chain` could refer to the derive helper attribute defined here
  --> /home/weiznich/.cargo/registry/src/github.com-1ecc6299db9ec823/dotenv-0.10.1/src/lib.rs:22:17
   |
22 | #[derive(Debug, error_chain)]
   |                 ^^^^^^^^^^^
note: `error_chain` could also refer to the derive macro imported here
  --> /home/weiznich/.cargo/registry/src/github.com-1ecc6299db9ec823/dotenv-0.10.1/src/lib.rs:10:1
   |
10 | #[macro_use]
   | ^^^^^^^^^^^^

error: aborting due to previous error

Copy link
Author

Choose a reason for hiding this comment

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

Okay, so no more allowing 0.10

Copy link
Member

@weiznich weiznich Nov 22, 2019

Choose a reason for hiding this comment

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

Disallowing 0.10 here does not fix that, because diesel-infer-schema pulls in the0.10version.

Copy link
Author

Choose a reason for hiding this comment

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

Luckily, it seems dotenv support in diesel_infer_schema is optional. I'll try just turning it off.

Copy link
Member

Choose a reason for hiding this comment

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

That will cause CI to be fixed but break people's ability to run tests locally. We probably just need to get rid of diesel_infer_schema from our test suite

Copy link
Author

Choose a reason for hiding this comment

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

That's probably better. All of the deprecated stuff is removed in the next release, anyway.

Copy link
Author

Choose a reason for hiding this comment

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

That's also a pretty invasive change, though. Somebody who knows more about diesel should do it in a separate PR, and this one's blocked on it.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine to just leave the old dotenv version here, because that one continues to work. #2197 removes the dependency on diesel_infer_schema.

It's not needed, since tests don't actually use `.env`,
and it pulls in unnecessary (and, in at least one case, broken by nightly Rust) code.
@notriddle notriddle changed the title Upgrade dotenv to 0.15 Upgrade dotenv to 0.15 [blocked on change to test suite] Nov 22, 2019
@JohnTitor
Copy link
Member

I think we could close this in favor of #2410 (sorry for stealing your contribution! I didn't notice this until right now).

@weiznich weiznich closed this Jul 1, 2020
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

Successfully merging this pull request may close these issues.

4 participants