Fixing CI errors #1226
Fixing CI errors #1226
Conversation
skewballfox
changed the title
tests/zst.rs
Outdated
| @@ -18,6 +18,6 @@ fn test_swap() { | |||
| #[test] | |||
| fn test() { | |||
| let c = ArcArray::<(), _>::default((3, 4)); | |||
| let mut d = c.clone(); | |||
| let mut d = c; | |||
There was a problem hiding this comment.
Cloning an Arc changes the ref count. It has to be assumed to be part of the test. Without this clone, the test doesn't use copy on write anymore.
We have to realize that often the linter is wrong, not the code. 🙂
There was a problem hiding this comment.
I can revert the changes and then add some stuff to ignore lints on specific lines(I think)
There was a problem hiding this comment.
reverted and suppressed lints for function
src/zip/mod.rs
Outdated
| @@ -92,7 +92,7 @@ where | |||
| { | |||
| type Output = ArrayView<'a, A, E::Dim>; | |||
| fn broadcast_unwrap(self, shape: E) -> Self::Output { | |||
| let res: ArrayView<'_, A, E::Dim> = (&self).broadcast_unwrap(shape.into_dimension()); | |||
| let res: ArrayView<'_, A, E::Dim> = self.broadcast_unwrap(shape.into_dimension()); | |||
There was a problem hiding this comment.
Linter is wrong and it shouldn't be changed, it looks like. local suppression possible?
There was a problem hiding this comment.
yeah, I believe so, at least it seems that way from this SO post
There was a problem hiding this comment.
I suppressed this lint for the specified function
tests/array.rs
Outdated
| [4, 5, 6], | ||
| [7, 8, 9], | ||
| [10,11,12]]); | ||
| let mut a = arr2(&[[1, 2, 3], [4, 5, 6], [7, 8, 9], [10, 11, 12]]); |
There was a problem hiding this comment.
If we can avoid it, we won't commit changes like this where nice visual formatting is removed
There was a problem hiding this comment.
I reverted the changes to the original and reapplied the fixes. I turned off formatting so now the formatting is as it was
|
hey just letting you know, haven't abandoned this, just hit a rough patch of deadlines, likely won't circle back round until after December 7th. If someone wants to perform a PR on this branch to fix issues before then (rather than opening a duplicate PR), I'm happy to merge them. |
|
I am sorry to close this as you obviously put a lot of effort into these changes, but I want to merge #1286 instead which makes a lot of similar modifications but avoids bumping the MSRV and any formatting changes. |
I noticed that CI was failing, I figured I'd do a PR to fix issues I'm noticing
the first CI failure is due to use of a deprecated function
itertools::zipintest/array.rs.switching to
std::iter::ziprequires updating the MSRV inci.ymlfrom 1.51 to 1.59, I went ahead and changed my copy to that rust version, but I don't know if there is any specific reason why 1.51 was set as the minimum. If there is, what would be the best alternative to using either version of zip?the second error seems to be due to a duplicate mount point for the i686 cross-test, still working on that one.
the third error is for the clippy lints, I fixed two by running
cargo clippy --fix --no-deps, but the third is less straight forward.EDIT:
the second error I believe is caused by the layout of the project, with ndarray and ndarray-rand both being dependencies with relative imports. because ndarray is in the parent directory, and ndarray-rand is a child directory, this creates duplicate mountpoints. I could fix this by moving ndarray into it's own directory and keeping the top level parent for project specs but I should probably confirm that is the direction you guys would like to go first. There may be a way to fix it by providing some info to cross via an env variable or CLI flag, still looking through the repo for cross to figure out if that's the case
EDIT2: I fixed this by adding xtest-numeric to the list of workspace members, which makes all the CI test pass, but has the downside of making the CI test run for around 20 minutes. Before the test from
all-test.shwere finishing pretty quickly. Is there anything else I need to change when addingxtest-numericto the list of workspace-members?