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

Update nalgebra and the rest of dependencies to have the crates compile #33

Merged
merged 28 commits into from Jul 12, 2021

Conversation

mpizenberg
Copy link
Member

WIP: this is a work in progress

I've updated nalgebra, nshare, ndarray and started making all the appropriate changes in the cv monorepo.
Right now I'm stuck due to a trait bound issue in cv-optimize, coming from argmin which cannot yet be updated to ndarray 0.15 because it itself depends on ndarray-linalg that hasn't been update yet. See argmin-rs/argmin#123

@mpizenberg
Copy link
Member Author

mpizenberg commented Jun 28, 2021

Current state of crates that compile and pass the tests:

  • akaze
  • cv
  • cv-core
  • cv-geom
  • cv-pinhole
  • eight-point
  • imgshow
  • kpdraw
  • lambda-twist
  • nister-stewenius
  • tutorial-code/chapter2-first-program
  • tutorial-code/chapter3-akaze-feature-extraction
  • cv-reconstruction
  • cv-optimize
  • vslam-sandbox

@mpizenberg
Copy link
Member Author

It seems that the last 3 remaining crates are also facing another problem when compiling in stable which is that they require nalgebra/alloc and that feature seems to pull in code only compiling on nightly. I don't know if that was also the case before. I'll let you handle that situation.

@mpizenberg
Copy link
Member Author

Also, just a remark, I've used the following in the dependencies while waiting for your merging of lm

levenberg-marquardt = { version = "0.8.0", path = "../../levenberg-marquardt" }

@astraw
Copy link
Contributor

astraw commented Jun 28, 2021

It seems that the last 3 remaining crates are also facing another problem when compiling in stable which is that they require nalgebra/alloc and that feature seems to pull in code only compiling on nightly. I don't know if that was also the case before. I'll let you handle that situation.

For what it's worth, I don't have any compilation errors with rust stable (rustc 1.52.1) using the alloc feature of nalgebra. For example I just checked the cam-geom crate with cargo test --features nalgebra/alloc and this compiled and passed all tests.

@mpizenberg
Copy link
Member Author

mpizenberg commented Jun 28, 2021

Thanks Andrew, I'm running with rustc 1.53 so I don't think that's a compiler version issue. The problem is the following compiler error:

cv-optimize $> cargo check
    Checking nalgebra v0.27.1
error[E0554]: `#![feature]` may not be used on the stable release channel
  --> /home/matthieu/.cargo/registry/src/github.com-1ecc6299db9ec823/nalgebra-0.27.1/src/lib.rs:89:59
   |
89 | #![cfg_attr(all(feature = "alloc", not(feature = "std")), feature(alloc))]
   |                                                           ^^^^^^^^^^^^^^

error: aborting due to previous error

For more information about this error, try `rustc --explain E0554`.
error: could not compile `nalgebra`

@mpizenberg
Copy link
Member Author

By the way @astraw I've just tried and cam-geom has the same issue. If you cargo clean and then:

cargo check --no-default-features --features nalgebra/alloc

I get the same compiler error. Don't forget the --no-default-features otherwise you pull in std and do not trigger that error.

@vadixidav
Copy link
Member

@astraw Can you confirm if this is working for you on stable, even with the clean and check? If so, we might need to do some more digging. I did ask sebcrozet about it on the Dimforge Discord server, but I haven't gotten a response back yet.

@astraw
Copy link
Contributor

astraw commented Jun 29, 2021

@vadixidav yes I get an error in that constellation with stable rust. But, since the unstable feature alloc is being requested I don't think this is a bug: (there is no trouble on nightly)

cargo check --no-default-features --features nalgebra/alloc
    Checking nalgebra v0.27.1
error[E0554]: `#![feature]` may not be used on the stable release channel
  --> C:\Users\drand\.cargo\registry\src\github.com-1ecc6299db9ec823\nalgebra-0.27.1\src/lib.rs:89:59
   |
89 | #![cfg_attr(all(feature = "alloc", not(feature = "std")), feature(alloc))]
   |                                                           ^^^^^^^^^^^^^^

If a new version of nalgebra requires allocation without that new requirement being intentional, they should be alerted to that. But I don't think that's the case here - by compiling with the alloc flag, one is saying that one wants the unstable alloc feature, so why should this be expected to work on stable?

By the way, you can compile something and ensure there is no requirement for std (or alloc) by compiling for a target without either of those like this cargo build --no-default-features --target thumbv7em-none-eabihf. I do this in the cam-geom continuous integration tests.

@vadixidav
Copy link
Member

@vadixidav yes I get an error in that constellation with stable rust. But, since the unstable feature alloc is being requested I don't think this is a bug: (there is no trouble on nightly)

cargo check --no-default-features --features nalgebra/alloc
    Checking nalgebra v0.27.1
error[E0554]: `#![feature]` may not be used on the stable release channel
  --> C:\Users\drand\.cargo\registry\src\github.com-1ecc6299db9ec823\nalgebra-0.27.1\src/lib.rs:89:59
   |
89 | #![cfg_attr(all(feature = "alloc", not(feature = "std")), feature(alloc))]
   |                                                           ^^^^^^^^^^^^^^

If a new version of nalgebra requires allocation without that new requirement being intentional, they should be alerted to that. But I don't think that's the case here - by compiling with the alloc flag, one is saying that one wants the unstable alloc feature, so why should this be expected to work on stable?

By the way, you can compile something and ensure there is no requirement for std (or alloc) by compiling for a target without either of those like this cargo build --no-default-features --target thumbv7em-none-eabihf. I do this in the cam-geom continuous integration tests.

Well, the alloc feature has been stable for a very long time. This is why I expected that it might work on stable. As @mpizenberg has discovered, it isn't the feature itself that is nightly-only, but rather asking for a feature is nightly-only. I will need to submit a PR to nalgebra to get it working on stable. Ideally, everything we do would work on stable Rust, and we are very close to that today.

@astraw
Copy link
Contributor

astraw commented Jun 30, 2021

I've checked nalgebra versions 0.24.1, 0.25.4, 0.26.2, and 0.27.1. The nalgebra alloc cargo feature does not compile with stable rust on any of those versions because it enables the unstable rust feature alloc. I do not think this is a regression in nalgebra and if, so, it is a regression from the ancient past.

Allocation can work on stable rust with std. If you need allocation but want to avoid std, as far as I can see nightly is currently the only possibility.

@vadixidav
Copy link
Member

@astraw nalgebra has never built with alloc on stable, but you can use alloc on stable. We just need to make a PR to nalgebra to remove the line that enables the alloc feature, which is outdated.

@vadixidav
Copy link
Member

Okay, I created a PR to solve the issues we encountered over here: dimforge/nalgebra#944.

In the meantime, we may need to depend on std. Once nalgebra fixes this issue, we can create a new release depending on nightly.

@vadixidav
Copy link
Member

I have almost got everything in working order. We are hitting a bug with Clippy that is reported here: rust-lang/rust-clippy#7423. I will merge this tomorrow after some final cleanup.

@vadixidav
Copy link
Member

We are also hitting some intermittent ICEs. This could be mitigated by switching CI to stable if these issues persist. If I am not mistaken, I believe we do not depend on any nightly features, so I can look into this tomorrow.

@vadixidav
Copy link
Member

Good news, sebcrozet merged the PR to nalgebra. Bad news, it didn't make it into version 0.28.0, which was cut right before the PR was accepted. So we will need to wait for version 0.28.1 of nalgebra for us to be able to use alloc without std again.

@vadixidav
Copy link
Member

So, everything is working except for nalgebra without std feature. We are still hitting CI failure due to the following Clippy bug:

thread 'rustc' panicked at '`LateContext::typeck_results` called outside of body', src/tools/clippy/clippy_lints/src/use_self.rs:213:20

Again, the Clippy issue is tracked here: rust-lang/rust-clippy#7423. I think I will go ahead and merge this for now. It is not ready for release, but it is all building and would otherwise pass Clippy if it weren't crashing.

@vadixidav vadixidav marked this pull request as ready for review July 12, 2021 18:32
@vadixidav vadixidav merged commit 65234dc into master Jul 12, 2021
@vadixidav vadixidav deleted the compilable branch July 12, 2021 18:33
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.

None yet

3 participants