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

Jump to bottom

diskonaut: init at 0.3.0 #91460

Merged
merged 1 commit into from Jun 26, 2020
Merged

diskonaut: init at 0.3.0 #91460

merged 1 commit into from Jun 26, 2020

Conversation

evanjs
Copy link
Member

@evanjs evanjs commented Jun 25, 2020 •
edited

Motivation for this change

Diskonaut — A disk usage explorer / terminal disk space navigator
Featured as crate of the week in This Week In Rust 344 (23 Jun 2020)

Tests currently failing
builder for '/nix/store/7i727mk7dif5jmvh06xrj77162yl8s3k-diskonaut-1.15.1.drv' failed with exit code 101; last 10 log lines:
  thread 'tests::cases::ui::small_files_with_x_as_zero' panicked at 'snapshot assertion for 'small_files_with_x_as_zero' failed in line 2108', /build/diskonaut-1.15.1-vendor.tar.gz/insta/src/runtime.rs:995:9
  
  
  failures:
      tests::cases::ui::small_files_with_x_as_zero
      tests::cases::ui::small_files_with_y_as_zero
  
  test result: FAILED. 35 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out
  
  error: test failed, to rerun pass '--bin diskonaut'
[0 built (1 failed)]
error: build of '/nix/store/7i727mk7dif5jmvh06xrj77162yl8s3k-diskonaut-1.15.1.drv' failed

iblech
iblech approved these changes Jun 25, 2020
View reviewed changes
Copy link
Contributor

@iblech iblech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool, thanks for packaging diskonaut! I justed tested it here, works beautifully. Wouldn't know how to improve the build file, looks perfect. Thank you!

@iblech
Copy link
Contributor

iblech commented Jun 25, 2020

Also I should say that no tests are failing on my computer:

     Running target/x86_64-unknown-linux-gnu/release/deps/diskonaut-40e8cbd4e97b6b1c

running 37 tests
test tests::cases::ui::cant_delete_file_with_term_too_small ... ok
test tests::cases::ui::clear_selection_when_moving_off_screen_edges ... ok
test tests::cases::ui::delete_file ... ok
test tests::cases::ui::delete_file_press_n ... ok
test tests::cases::ui::delete_folder ... ok
test tests::cases::ui::delete_folder_small_window ... ok
test tests::cases::ui::cannot_move_into_small_files ... ok
test tests::cases::ui::delete_folder_with_multiple_children ... ok
test tests::cases::ui::empty_folder ... ok
test tests::cases::ui::eleven_files ... ok
test tests::cases::ui::enter_folder ... ok
test tests::cases::ui::enter_folder_medium_width ... ok
test tests::cases::ui::enter_folder_small_width ... ok
test tests::cases::ui::files_with_size_zero ... ok
test tests::cases::ui::medium_width ... ok
test tests::cases::ui::esc_to_go_up ... ok
test tests::cases::ui::move_down_and_enter_folder ... ok
test tests::cases::ui::move_left_and_enter_folder ... ok
test tests::cases::ui::minimum_tile_sides ... ok
test tests::cases::ui::move_right_and_enter_folder ... ok
test tests::cases::ui::move_up_and_enter_folder ... ok
test tests::cases::ui::noop_when_entering_file ... ok
test tests::cases::ui::permission_denied_when_deleting ... ok
test tests::cases::ui::pressing_delete_with_no_selected_tile ... ok
test tests::cases::ui::noop_when_pressing_esc_at_base_folder ... ok
test tests::cases::ui::small_files_with_x_as_zero ... ok
test tests::cases::ui::small_files ... ok
test tests::cases::ui::small_width ... ok
test tests::cases::ui::small_width_long_folder_name ... ok
test tests::cases::ui::too_small_height ... ok
test tests::cases::ui::too_small_width_five ... ok
test tests::cases::ui::too_small_width_four ... ok
test tests::cases::ui::too_small_width_one ... ok
test tests::cases::ui::too_small_width_three ... ok
test tests::cases::ui::too_small_width_two ... ok
test tests::cases::ui::two_large_files_one_small_file ... ok
test tests::cases::ui::small_files_with_y_as_zero ... ok

test result: ok. 37 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

@evanjs
Copy link
Member Author

evanjs commented Jun 25, 2020

Also I should say that no tests are failing on my computer:

     Running target/x86_64-unknown-linux-gnu/release/deps/diskonaut-40e8cbd4e97b6b1c



running 37 tests

test tests::cases::ui::cant_delete_file_with_term_too_small ... ok

test tests::cases::ui::clear_selection_when_moving_off_screen_edges ... ok

test tests::cases::ui::delete_file ... ok

test tests::cases::ui::delete_file_press_n ... ok

test tests::cases::ui::delete_folder ... ok

test tests::cases::ui::delete_folder_small_window ... ok

test tests::cases::ui::cannot_move_into_small_files ... ok

test tests::cases::ui::delete_folder_with_multiple_children ... ok

test tests::cases::ui::empty_folder ... ok

test tests::cases::ui::eleven_files ... ok

test tests::cases::ui::enter_folder ... ok

test tests::cases::ui::enter_folder_medium_width ... ok

test tests::cases::ui::enter_folder_small_width ... ok

test tests::cases::ui::files_with_size_zero ... ok

test tests::cases::ui::medium_width ... ok

test tests::cases::ui::esc_to_go_up ... ok

test tests::cases::ui::move_down_and_enter_folder ... ok

test tests::cases::ui::move_left_and_enter_folder ... ok

test tests::cases::ui::minimum_tile_sides ... ok

test tests::cases::ui::move_right_and_enter_folder ... ok

test tests::cases::ui::move_up_and_enter_folder ... ok

test tests::cases::ui::noop_when_entering_file ... ok

test tests::cases::ui::permission_denied_when_deleting ... ok

test tests::cases::ui::pressing_delete_with_no_selected_tile ... ok

test tests::cases::ui::noop_when_pressing_esc_at_base_folder ... ok

test tests::cases::ui::small_files_with_x_as_zero ... ok

test tests::cases::ui::small_files ... ok

test tests::cases::ui::small_width ... ok

test tests::cases::ui::small_width_long_folder_name ... ok

test tests::cases::ui::too_small_height ... ok

test tests::cases::ui::too_small_width_five ... ok

test tests::cases::ui::too_small_width_four ... ok

test tests::cases::ui::too_small_width_one ... ok

test tests::cases::ui::too_small_width_three ... ok

test tests::cases::ui::too_small_width_two ... ok

test tests::cases::ui::two_large_files_one_small_file ... ok

test tests::cases::ui::small_files_with_y_as_zero ... ok



test result: ok. 37 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

Ah. Interesting 🤔 I guess I’ll try nixpkgs-review and see if that works.

danieldk
danieldk suggested changes Jun 25, 2020
View reviewed changes
Copy link
Contributor

@danieldk danieldk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some suggested changes. Tests fail here as well, but there was an upstream issue for that:

imsnif/diskonaut#42

Until the next release, you can fetchpatch this patch:

https://github.com/imsnif/diskonaut/commit/e94f9b942d8928b7993351a4109cfcef03055d2f.diff

It would also be good to add a comment above the patch with a reference to the PR saying that it can be removed after the next version bump.

pkgs/tools/misc/diskonaut/default.nix Outdated Show resolved Hide resolved
pkgs/tools/misc/diskonaut/default.nix Outdated Show resolved Hide resolved
pkgs/tools/misc/diskonaut/default.nix Outdated Show resolved Hide resolved
@evanjs
Copy link
Member Author

evanjs commented Jun 25, 2020

I added some suggested changes. Tests fail here as well, but there was an upstream issue for that:

imsnif/diskonaut#42

Until the next release, you can fetchpatch this patch:

https://github.com/imsnif/diskonaut/commit/e94f9b942d8928b7993351a4109cfcef03055d2f.diff

It would also be good to add a comment above the patch with a reference to the PR saying that it can be removed after the next version bump.

Sounds good, didn’t see check the issues first.

And also forgot/missed that version change (I swear I changed it already). Whoops.

Thanks!

@evanjs evanjs force-pushed the diskonaut-init branch 2 times, most recently from 4a15ab8 to 57bc45b Compare June 25, 2020 16:19
@evanjs
Copy link
Member Author

evanjs commented Jun 25, 2020

Hrm, tests still seem to fail.
@danieldk any thoughts on what I might have missed?

Also: do you think that using the <PR>.patch url might be enough to imply "remove this after it has been released"?

@danieldk
Copy link
Contributor

Hrm, tests still seem to fail.
@danieldk any thoughts on what I might have missed?

They still fail, because diskonaut reports incorrect file sizes on some filesystems. I have filed an upstream bug:

imsnif/diskonaut#50

@evanjs
Copy link
Member Author

evanjs commented Jun 25, 2020

Hrm, tests still seem to fail.

@danieldk any thoughts on what I might have missed?

They still fail, because diskonaut reports incorrect file sizes on some filesystems. I have filed an upstream bug:

imsnif/diskonaut#50

Thanks!

I’ll just skip those specific tests for now, then?

@evanjs evanjs mentioned this pull request Jun 25, 2020
Closed
@evanjs
Copy link
Member Author

evanjs commented Jun 26, 2020 •
edited

Disabled the failing tests and fixed the hash for the patch.
Let me know if there's anything else!

danieldk
danieldk suggested changes Jun 26, 2020
View reviewed changes
Copy link
Contributor

@danieldk danieldk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think at this point it makes more sense to disable tests altogether. However, since incorrect file sizes are reported on the majority of systems that we tested on (yours, mine, the ofborg builders), it probably makes most sense to wait until the next release is out, which hopefully fixes this error and then update this PR for that release.

pkgs/tools/misc/diskonaut/default.nix Outdated Show resolved Hide resolved
pkgs/tools/misc/diskonaut/default.nix Outdated Show resolved Hide resolved
@danieldk
Copy link
Contributor

danieldk commented Jun 26, 2020 •
edited

it probably makes most sense to wait until the next release is out

Since this is the intended reported usage (see upstream issue), let's just disable tests. Maybe you could do that (as in the suggested changes), make this PR a non-draft, and then this PR should be good to go.

@evanjs evanjs marked this pull request as ready for review June 26, 2020 15:40
danieldk
danieldk approved these changes Jun 26, 2020
View reviewed changes
Copy link
Contributor

@danieldk danieldk left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I have moved cargoSha256 up to make the derivation more canonical.

Result of nixpkgs-review pr 91460 1

1 package built:
- diskonaut

Thanks for making all the requested changes! I'll merge this once ofborg is done.

@danieldk danieldk merged commit 5e65ede into NixOS:master Jun 26, 2020
@evanjs evanjs mentioned this pull request Jul 20, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danieldk danieldk danieldk approved these changes

@iblech iblech iblech approved these changes

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@evanjs @iblech @danieldk