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

resolver 1 fallback does not work in edition 2021 #665

Closed
clux opened this issue Oct 22, 2021 · 28 comments · Fixed by #667
Closed

resolver 1 fallback does not work in edition 2021 #665

clux opened this issue Oct 22, 2021 · 28 comments · Fixed by #667
Labels
invalid rejected as a valid issue

Comments

@clux
Copy link
Member

clux commented Oct 22, 2021

Trying to figure out why this is. Have tried setting resolver = "1" to get the old behaviour, but getting the k8s-openapi compile time failure when publishing.

might open a bug upstream 🤔

@kazk
Copy link
Member

kazk commented Oct 22, 2021

Not directly related, but should we have a policy for MSRV?

@clux
Copy link
Member Author

clux commented Oct 22, 2021

Do you mean setting the new flag in cargo.toml or something else?

I don't think we're realistically going to be targeting anything below the latest stable version. As soon as there's a new dependency out, we basically have to move with the community.

@clux clux mentioned this issue Oct 22, 2021
@nightkr
Copy link
Member

nightkr commented Oct 22, 2021

What error message do you get? I can't seem to reproduce this locally...

@clux
Copy link
Member Author

clux commented Oct 22, 2021

It's one of those fun ones that only triggers on cargo publish

logs

⋯  repos  kube-rs  kube-core ⎈ default∈k3d-main  master  ↑1 +3 ⚑  cargo publish --dry-run
    Updating crates.io index
   Packaging kube-core v0.61.0 (/home/clux/repos/kube-rs/kube-core)
   Verifying kube-core v0.61.0 (/home/clux/repos/kube-rs/kube-core)
   Compiling proc-macro2 v1.0.30
   Compiling unicode-xid v0.2.2
   Compiling syn v1.0.80
   Compiling autocfg v1.0.1
   Compiling serde_derive v1.0.130
   Compiling serde v1.0.130
   Compiling libc v0.2.104
   Compiling ryu v1.0.5
   Compiling serde_json v1.0.68
   Compiling itoa v0.4.8
   Compiling bytes v1.1.0
   Compiling k8s-openapi v0.13.1
   Compiling base64 v0.13.0
   Compiling fnv v1.0.7
   Compiling percent-encoding v2.1.0
   Compiling matches v0.1.9
   Compiling once_cell v1.8.0
   Compiling num-traits v0.2.14
   Compiling num-integer v0.1.44
   Compiling form_urlencoded v1.0.1
   Compiling http v0.2.5
error: failed to run custom build command for `k8s-openapi v0.13.1`

Caused by:
  process didn't exit successfully: `/home/clux/repos/kube-rs/target/package/kube-core-0.61.0/target/debug/build/k8s-openapi-e490db9789443aa3/build-script-build` (exit status: 101)
  --- stderr
  thread 'main' panicked at '
  None of the v1_* features are enabled on the k8s-openapi crate.

  The k8s-openapi crate requires a feature to be enabled to indicate which version of Kubernetes it should support.

  If you're using k8s-openapi in a binary crate, enable the feature corresponding to the minimum version of API server that you want to support. It may be possible that your binary crate does not directly depend on k8s-openapi. In this case, add a dependency on k8s-openapi, then enable the corresponding feature.

  If you're using k8s-openapi in a library crate, add a dev-dependency on k8s-openapi and enable one of the features there. This way the feature will be enabled when buildings tests and examples of your library, but not when building the library itself. It may be possible that your library crate does not directly depend on k8s-openapi. In this case, add a dev-dependency on k8s-openapi, then enable the corresponding feature.

  Library crates *must not* enable any features in their direct dependency on k8s-openapi, only in their dev-dependency. The choice of Kubernetes version to support should be left to the final binary crate, so only the binary crate should enable a specific feature. If library crates also enable features, it can cause multiple features to be enabled simultaneously, which k8s-openapi does not support.

  If you want to restrict your library crate to support only a single specific version or range of versions of Kubernetes, please use the k8s_* version-specific macros to emit different code based on which feature gets enabled in the end.', /home/clux/.cargo/registry/src/github.com-1ecc6299db9ec823/k8s-openapi-0.13.1/build.rs:9:42
  stack backtrace:
     0: rust_begin_unwind
               at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa/library/std/src/panicking.rs:517:5
     1: core::panicking::panic_fmt
               at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa/library/core/src/panicking.rs:101:14
     2: core::option::expect_failed
               at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa/library/core/src/option.rs:1615:5
     3: core::option::Option<T>::expect
               at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa/library/core/src/option.rs:698:21
     4: build_script_build::main
               at ./build.rs:9:18
     5: core::ops::function::FnOnce::call_once
               at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa/library/core/src/ops/function.rs:227:5
  note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
warning: build failed, waiting for other jobs to finish...
error: failed to verify package tarball

Caused by:
  build failed

@clux
Copy link
Member Author

clux commented Oct 22, 2021

because kube-core depends on k8s-openapi but without features basically and relying on one specific feature set for testing:

[dev-dependencies.k8s-openapi]
version = "0.13.1"
default-features = false
features = ["v1_22"]

@nightkr
Copy link
Member

nightkr commented Oct 22, 2021

Ah right, because it essentially builds (the equivalent of) cargo +nightly build -Z avoid-dev-deps...

@nightkr
Copy link
Member

nightkr commented Oct 22, 2021

For what it's worth, a local repro command would be cargo package -p kube-client

@clux
Copy link
Member Author

clux commented Oct 22, 2021

Nice. Sounds like it's worth raising this upstream then.

@nightkr
Copy link
Member

nightkr commented Oct 22, 2021

Okay, looks like this can be worked around locally, by running cargo publish -p kube-client --features k8s-openapi/v1_22. I wonder if we can hook this into cargo-release?

@nightkr
Copy link
Member

nightkr commented Oct 22, 2021

Yes, looks like we just need to set enable-features in release.toml: https://github.com/crate-ci/cargo-release/blob/master/docs/reference.md

@clux
Copy link
Member Author

clux commented Oct 22, 2021

hm. interesting. didn't know publish would be configurable for these flags. that's great!

now. i just merged the backout. thinking i'll just do the rust 2018 release with for 0.62 for now, then make sure 2021 is ok for the next version?

@nightkr
Copy link
Member

nightkr commented Oct 22, 2021

Sure. I'll prepare a PR for backing out the backout.

@clux
Copy link
Member Author

clux commented Oct 22, 2021

This did finally publish (manually)... but it might need a follow-up (suspect the docs build for kube-runtime will fail because of a bootstrapping problem - it dev depends on kube with runtime feature, which did not exist at time of publishing)

@kazk
Copy link
Member

kazk commented Oct 22, 2021

Do you mean setting the new flag in cargo.toml or something else?

rust-version can be added to Cargo.toml to show a better message when users are using an older compiler in the future (the field is ignored by Cargo older than the one just released). https://doc.rust-lang.org/cargo/reference/manifest.html#the-rust-version-field

I don't think we're realistically going to be targeting anything below the latest stable version. As soon as there's a new dependency out, we basically have to move with the community.

New compilers will continue to work. Maybe I'm misunderstanding, but setting edition = "2021" requires users to use 1.56+, right?

Tokio has MSRV policy of 6 months.

@clux
Copy link
Member Author

clux commented Oct 22, 2021

Yeah, i think it's definitely a good idea for us to automatically set the rust-version field when we make upgrades that requires newer rust versions (and indeed edition 2021 would require 1.56).

I am not sure how tokio can do sensibly have 6 months policy on this. If all their dependencies (say) bump to edition 2021, then how can they get security updates? Hope library authors backport?

@olix0r
Copy link
Contributor

olix0r commented Oct 22, 2021

@clux Tokio doesn't really have external dependencies that aren't also maintained by Tokio maintainers...

For a higher level project like this, it's going to be dependent on the upstream compatibility policies. Six months, though, does seem like a good target to me, if possible.

@clux
Copy link
Member Author

clux commented Oct 22, 2021

Hm, we would definitely be doing upgrades on a more sparse basis then. But I suppose it's workable if we get CI to run against the MSRV in addition to stable.

The problem with this though, would be users smuggling in a rust-version bumping upgrade in a patch release (perhaps through a dependency minor upgrade that they don't expect to be revving rust). I would assume we would need to maintain semver ranges for all of that, and cause a corresponding increase in maintenance burden.

@kazk
Copy link
Member

kazk commented Oct 22, 2021

Sorry, I shouldn't have started MSRV discussion here. Let's open a new issue/discussion.

it might need a follow-up (suspect the docs build for kube-runtime will fail because of a bootstrapping problem - it dev depends on kube with runtime feature, which did not exist at time of publishing)

I don't see any of the docs.

https://crates.io/crates/kube doesn't have the link to docs.rs.

I don't see any errors like #366 either.

@clux
Copy link
Member Author

clux commented Oct 22, 2021

Yeah, let's can open a new issue on MSRV.

Yeah, it's been taking longer and longer between docs.rs publishes, so was hoping this is just a long queue on their end. I was going to tweet, but have to wait for docs. I don't think it's related to the bootstrapping problem though because that should only affect kube-runtime (it was the only crate i had to hack during the publish cycle).

@kazk
Copy link
Member

kazk commented Oct 22, 2021

I just found this page: https://docs.rs/releases/queue

kube-core is 134th.

Everyone is releasing today
image

@clux
Copy link
Member Author

clux commented Oct 22, 2021

Ah, that's a good status page.
Guess we get to enjoy 6? hour iteration times between publishing and seeing if docs.rs builds work 😅

@clux clux closed this as completed in #667 Oct 24, 2021
@clux
Copy link
Member Author

clux commented Oct 24, 2021

Everything is working fine in the backout AFAIKT. Not going to open up any bugs against resolver 1 in cargo on 2021 because we can actually use resolver 2 by passing --features to cargo package and cargo publish (and via cargo release).

Will open a separate issue on MSRV and summarise the points.

@clux clux added the invalid rejected as a valid issue label Oct 24, 2021
@webern
Copy link

webern commented Jan 25, 2022

I may be hitting this with a local cargo install command:

cargo install --path "${MY_WORKSPACE}/mything" --force

I don't understand why I didn't see this before, it seems like it just popped up and I'm not sure if it's the same problem. Changing my command to specify features does not fix the problem:

cargo install --path "${MY_WORKSPACE}/mything" --features k8s-openapi/v1_20 --force

I do have this in the Cargo.toml for this binary

[dependencies]
# ...
k8s-openapi = { version = "0", features = ["v1_20", "api"], default-features = false }

@clux
Copy link
Member Author

clux commented Jan 25, 2022

this is likely because there's a new version of k8s-openapi and the kube release picking up on it has not been released yet. hang on, will release the new kube version.

@clux
Copy link
Member Author

clux commented Jan 25, 2022

kube 0.67 is out now, I expect that should resolve the issue

@webern
Copy link

webern commented Jan 25, 2022

I don't think that was the issue, I'm using an older version right now anyway, 0.64 / 0.13.1 (I will update soon but that's in another PR waiting to let some PRs settle before merging).

I tried changing my Cargo.toml to say (though this is what's already in my workspace's Cargo.lock):

k8s-openapi = { version = "=0.13.1", features = ["v1_20", "api"], default-features = false }
kube = { version = "=0.64.0", default-features = true, features = ["config", "derive", "ws"] }

I did a cargo clean and then cargo install --path "${MY_WORKSPACE}/mything" --force

This time I got the type of error that indicates there are two versions of a struct or trait:

error[E0308]: mismatched types
   --> model/src/system/controller.rs:185:32
    |
185 |                   metadata: Some(ObjectMeta {
    |  ________________________________^
186 | |                     labels: Some(btreemap! {
187 | |                         LABEL_COMPONENT.to_string() => "controller".to_string(),
188 | |                     }),
189 | |                     namespace: Some(NAMESPACE.to_string()),
190 | |                     ..Default::default()
191 | |                 }),
    | |_________________^ expected struct `k8s_openapi::apimachinery::pkg::apis::meta::v1::ObjectMeta`, found struct `kube::kube_core::ObjectMeta`
    |

I definitely don't understand what's going on. I'll continue trying to figure it out tomorrow.

@clux
Copy link
Member Author

clux commented Jan 25, 2022

Well, that still looks like a multiple versions issue because k8s_openapi::apimachinery::pkg::apis::meta::v1::ObjectMeta is what we re-export as kube::kube_core::ObjectMeta.

Maybe try cargo tree | grep k8s-openapi to verify that you only have one version of it present.

@webern
Copy link

webern commented Jan 25, 2022

Thank you for your help with is. It was ultimately a version mismatch stemming from my belief that version = "0" would choose the correct "major" version of k8s-openapi. That doesn't seem to be the case, so I'll make sure to specify 0.X.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid rejected as a valid issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants