aerospike / aerospike-client-rust

Rust client for the Aerospike database
https://www.aerospike.com/
Other
82 stars 29 forks source link

Update all dependencies and remove multi-versions #55

Closed dnaka91 closed 5 years ago

dnaka91 commented 5 years ago

I was just starting to use this library and noticed that it pulls in a lot of outdated dependencies. Also cargo-audit gave me 2 vulnerability warnings for the dependencies used.

I simply updated most of the dependencies and replaced a few where it made sense. Only two things are there, where I'm not 💯 sure about:

dnaka91 commented 5 years ago

Hm, build is failing but that doesn't seem to be my fault. According to the logs CI couldn't start up the Aerospike server 🤔

jhecking commented 5 years ago

Thanks for the pull request! Let me take a look at what’s wrong with the Aerospiker server on the CI env.

dnaka91 commented 5 years ago

Hey @jhecking I saw you updated the CI config to get the build working again. Two tests seem to fail (same for me locally), so I just wait until everything is working again and then I rebase on master.

I saw the .travis.yml config has Rust 1.26.2 in the build matrix, but trying to build with that version didn't work out for me. After some testing it seems 1.32.0 is the MSRV.

jhecking commented 5 years ago

Hey @jhecking I saw you updated the CI config to get the build working again. Two tests seem to fail (same for me locally), so I just wait until everything is working again and then I rebase on master.

Yes, saw that as well. Just haven't had time to look into that yet.

I saw the .travis.yml config has Rust 1.26.2 in the build matrix, but trying to build with that version didn't work out for me. After some testing it seems 1.32.0 is the MSRV.

1.26.2 used to be the MSRV -- and I assume still is on master. But could be that some of the dependencies you updated require a new version. I'm ok to update the Travis test matrix to replace 1.26.2 with 1.32.0.

jhecking commented 5 years ago

Saw that there's cargo-msrv now to help with determining the MSRV. I'll give that a try.

dnaka91 commented 5 years ago

1.26.2 used to be the MSRV -- and I assume still is on master.

I actually built on master, but same result. The MSRV seems to be 1.32.0 even there.

I tried cargo-msrv too, but it takes forever. I think all it does (so far) is taking the latest rust version, compile, download 1 older version and repeat until it hits a compile error. Well it's very early stage.

jhecking commented 5 years ago

Two tests seem to fail (same for me locally), so I just wait until everything is working again and then I rebase on master.

I have fixed the two errors in the CDT lists/maps tests. (#57)

1.26.2 used to be the MSRV -- and I assume still is on master.

I actually built on master, but same result. The MSRV seems to be 1.32.0 even there.

The aerospike crate itself still compiles with 1.26.2, but some of its dependencies require a newer Rust version. I've raised the minimum Rust version to test against on Travis to 1.32.0. (#60)

I'm not really sure what's the best practice for documenting and/or testing the MSRV for a library crate. Since it's recommended best practice to not commit the Cargo.lock file for libraries, if any of my dependencies releases a new version, which requires a newer Rust version, I have to raise the minimum Rust version to test against as well. :-(

In any case, you can rebase on master now.

khaf commented 5 years ago

@jhecking Could you please test it on the enterprise edition and see if user/pass authentication still works?

dnaka91 commented 5 years ago

@jhecking thanks for all the effort. I will rebase my branch tomorrow and also do a few tests regarding authentication just in case.

Regarding the dependency problem, how about Travis CI cron jobs? Setting for example a weekly build trigger you will get notifications whenever the build may fail due to dependencies or any other means.

jhecking commented 5 years ago

@jhecking Could you please test it on the enterprise edition and see if user/pass authentication still works?

It does not. I believe the static salt is required.

jhecking commented 5 years ago

Regarding the dependency problem, how about Travis CI cron jobs? Setting for example a weekly build trigger you will get notifications whenever the build may fail due to dependencies or any other means.

That's not really the problem I'm trying to address. I want to know what the MSRV of my own library is, and test against that, so that I can detect if there have been any changes in my library that require a newer Rust version.

There's a good discussion of this general topic in this users' forum as well as some additional info on cargo's -Z minimal-versions flag.

I haven't really decided on what to do about this problem yet. One option is to just test the library against Rust's stable and beta versions and not worry about MSRV at this point.

jhecking commented 5 years ago

base64@0.11.0 requires Rust 1.34, so in order to get the tests to pass, you need to update .travis.yml as well, to raise the minimum version tests against to 1.34.0.

dnaka91 commented 5 years ago

Now finally it compiles and passes all tests.

I also rolled back to use pwhash, maybe you can try the authentication again? Just in case there are any breaking changes from 0.1 to 0.3.

I want to know what the MSRV of my own library is, and test against that, so that I can detect if there have been any changes in my library that require a newer Rust version.

Ah I get it now. Well if I find any tool or technique for that in the future I'll let you know 👍 And maybe that's gonna be solved in the future, an RFC already exists: https://github.com/rust-lang/rfcs/blob/master/text/2495-min-rust-version.md.

jhecking commented 5 years ago

All tests pass against an enterprise server with access control enabled.

jhecking commented 5 years ago

Do you need a v0.4.0 release for this urgently? Or can you live with building the package from master for now?

dnaka91 commented 5 years ago

I have another rather big change waiting that fixes all the deprecation and clippy warnings. Will send you the PR tomorrow.

Maybe after that it would make sense to release a new version but for now I'm fine with just using the master branch.