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

Support nalgebra 0.30 and 0.31 #91

Closed
wants to merge 2 commits into from

Conversation

AndrewJSchoen
Copy link
Contributor

@AndrewJSchoen AndrewJSchoen commented May 26, 2022

Updated the cargo.toml file to support both versions. Running cargo build with 0.31 seems to build without any issue on my machine.

@codecov
Copy link

codecov bot commented May 26, 2022

Codecov Report

Merging #91 (2023f12) into main (442f6cb) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #91   +/-   ##
=======================================
  Coverage   68.53%   68.53%           
=======================================
  Files          14       14           
  Lines        1570     1570           
=======================================
  Hits         1076     1076           
  Misses        494      494           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@AndrewJSchoen
Copy link
Contributor Author

Seems like there may be some issues with kiss3d (which would presumably have to have the same update) based on the tests.

@AndrewJSchoen
Copy link
Contributor Author

@OTL given that this would require a corresponding fix in Kiss3d, how would you prefer we proceed?

@taiki-e
Copy link
Contributor

taiki-e commented Jun 8, 2022

Thanks for the PR! The situation is almost the same as before.

There are three major options we can choose from:

The first (regular) way is to update ncollide to use the latest nalgebra (dimforge/ncollide#379), then update kiss3d to use the latest nalgebra and ncollide, and finally update this crate. This way may take longer as we need to wait for ncollide and kiss3d to be updated.

The second way is to replace the use of ncollide in kiss3d with another library (perhaps parry). I'm not the maintainer of kiss3d, so I don't know if this is possible or if this would be acceptable. If this way is accepted, then we need to wait only for kiss3d.

The third way is to replace the use of kiss3d and ncollide in all repositories under github.com/openrr with other libraries. We are considering moving from kiss3d to bevy, ncollide to parry, but don't have the bandwidth to do it right now (the maintainers would accept this approach if it works). In this way, there is no need to wait for other libraries updates. Of course, this way is the hardest.

For now, we are choosing the first way until we have the bandwidth to do the third.

@AndrewJSchoen
Copy link
Contributor Author

Just to clarify, I think this change should be inconsequential for kiss3d, since if kiss3d has a dependency on an earlier version of nalgebra, the earlier version would be selected in the dependencies.
nalgebra = ">= 0.30, >= 0.31" as opposed to simply nalgebra = "0.31" allows both, but if kiss3d is not being used, and a newer version is needed in another library, 0.31 would be selected.

@taiki-e
Copy link
Contributor

taiki-e commented Jan 10, 2023

I think that will work for private dependencies, but for public dependencies, I think one of the builds will break when both crate using k and nalgebra 0.31 and crate using k and nalgebra 0.30 appear in the dependency graph.

@AndrewJSchoen
Copy link
Contributor Author

I'm not sure I follow. Is that something we could test locally? I would be happy to try it out. For example, right now, I have another project that also uses/exports their own version of nalgebra (0.31) which means my build always breaks right now unless I use my own fork (which has the above dependency setting).

@taiki-e
Copy link
Contributor

taiki-e commented Jan 11, 2023

It does not seem to work as you said in #91 (comment). For example, the following will cause a "mismatched types" error due to version mismatch:

[package]
name = "a"
version = "0.1.0"
edition = "2021"

[workspace]

[dependencies]
urdf-viz = { version = "0.41", default-features = false }

[patch.crates-io]
k = { git = "https://github.com/AndrewJSchoen/k.git" }

cargo tree output for the above is:

$ cargo tree -i -p [email protected] -p [email protected]
nalgebra v0.30.1
├── kiss3d v0.35.0
│   └── urdf-viz v0.41.0
│       └── c v0.1.0 (/Users/taiki/projects/tmp/a/c)
└── ncollide3d v0.33.0
    └── kiss3d v0.35.0 (*)

nalgebra v0.31.4
└── k v0.29.1 (https://github.com/AndrewJSchoen/k.git#2023f12c)
    └── urdf-viz v0.41.0 (*)

(My original concern in #91 (comment) was the case where crate like the above example and nalgebra 0.31 are both included in the dependency tree.)

For example, right now, I have another project that also uses/exports their own version of nalgebra (0.31) which means my build always breaks right now unless I use my own fork (which has the above dependency setting).

In this case, you are using a version of nalgebra that is incompatible with the version of nalgebra that k uses, so this is expected behavior. And patching until upstream is updated is also the correct workaround.

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.

2 participants