Fix du parameters on macOS #74

friederbluemle · 2020-04-05T03:43:06Z

Fixes #70 by not using the unsupported -b flag on macOS. Instead, we use -k which is supported on both Linux and macOS. Since the reported value is already in kilobytes, we no longer have to convert it in npkill.

Also fixes #67 (same issue).

marcins · 2020-04-05T04:15:38Z

src/services/linux-files.service.ts


 export class LinuxFilesService extends FileService {
  constructor(private streamService: StreamService) {
    super();
  }

  getFolderSize(path: string): Observable<{}> {
-    const du = spawn('du', ['-s', '-b', path]);
+    const du = isOSMacOS() ? spawn('du', ['-s', path]) : spawn('du', ['-s', '-b', path]);


Note the default is to return blocks. So this won’t give the right number on macOS (the reason for the original change to -b). AFAIK the default block size is 512b.

My suggestion is to use -k (kilobytes), and skip the bytes to kB call for macOS.

Thanks for your comment @marcins! You mean use -k instead of -s on macOS?

Oh wait, use -sk I guess, since we're just interested in the summary

@marcins, you were absolutely right, it did not give the correct number on macOS.

Actually, the solution was much easier than what I initially tried with isOSMacOS: We can use du -sk on both Linux and macOS. :)

tonivj5 · 2020-04-05T23:14:37Z

Thanks for the PR @friederbluemle, I appreciate your contribution 👏

However, it resolves #70 and #67, it produces other drawbacks related with the reported size saved (as I commented here), because it's a non "real size". If you check the sizes reported using -b and -k, they could differ up to 50% bigger. I'm okey with revert the use of -b, running in macOS is more important, but we should look for how to resolve the source problem.

With nothing more to add, LGTM 👍

tonivj5 · 2020-04-05T23:16:53Z

I think @zaldih (and @NyaGarcia?) should comment here about this because he was who found the bug 👀

friederbluemle · 2020-04-06T08:59:08Z

Thanks for your comment @tonivj5 - Definitely agree with you that this might not be the final fix. If you have a suggestion on how to address it in this PR please let me know. Otherwise we can merge it and you can submit a follow-up PR?

This PR is just the bare minimum to fix the current issue on macOS (i.e. npkill not working at all).

I did check your comment about the sizing problem, and tried to reproduce it locally. Could you explain a bit more how exactly the reported size deviates from the "real size"? Thank you!

tonivj5 · 2020-04-06T22:41:15Z

Otherwise we can merge it and you can submit a follow-up PR?

Totally I agree with you

I did check your comment about the sizing problem, and tried to reproduce it locally. Could you explain a bit more how exactly the reported size deviates from the "real size"?

I think this article is going to explain it better than me. However, both sizes are "real", we have noticed that -b gets us a huge difference calculating the freed space of node_modules. And seeing the size reported from other file managers (Dolphin from my side), they report the space as du -b does.

I'm not completely sure, but I remember @zaldih had or saw an issue for that reason.

Maybe, we was doing the wrong way here, but I'm not very confident now 🙊

friederbluemle · 2020-04-07T02:07:30Z

@tonivj5 - Okay, that makes sense.

So it sounds like -b will give the exact (or apparent) size of the data on disk, while -k will provide the occupied disk space in kilobytes?

It sounds to me the "exact" byte size (-b) is less interesting here, and we really should be using -k anyway (even if it may be slightly off the "real" occupied space). That's still much better than using -b which could be orders of magnitude off.

For example: Consider a directory with 1,000 files in it. Each one only contains a single character (1 Byte). What should a tool report if asked for the size of the directory? 1,000 Bytes? Not really... The disk space the directory occupies is much higher: With a disk block size of 4096 Bytes it is around 4 Megabytes!

Since node_modules/ typically contains many small files, the occupied size is higher than the "real" size. And since npkill is about telling someone how much space will be saved, it will be much more accurate to use the "occupied" size instead of "real" size.

friederbluemle · 2020-04-07T02:26:42Z

Btw, I just rebased my PR head branch on top of develop - I've noticed this repo is using two "main" branches (develop and master), and it appears PRs should be submitted not against the default branch (master). This is slightly confusing and maybe should be pointed out explicitly in the README.md.
Or even better - Remove one branch (ideally the non-default one, develop) and move to a single branch model (master). Since the repo is already using tags for releases there is no benefit in using more than one main branch. It creates unnecessary complexity and increases branch "spaghetti" ;)

git log --graph --oneline

friederbluemle · 2020-04-07T02:35:26Z

Btw, if you want to check the block size of your disk on macOS:

diskutil info / | grep "Block Size"

And on Linux (replace <device> with the name of your device):

blockdev --getbsz /dev/<device>

tonivj5 · 2020-04-12T21:42:22Z

Since node_modules/ typically contains many small files, the occupied size is higher than the "real" size. And since npkill is about telling someone how much space will be saved, it will be much more accurate to use the "occupied" size instead of "real" size.

Yeah, I agree 👍

@zaldih has the last word about this, awaiting for his review 👀

NyaGarcia · 2020-05-20T19:36:00Z

Hi folks, sorry for the delay in answering 😅
So I read the discussion, and I can definitely agree that, even though this might not be the final fix, it's better to have a half working solution than none at all. So I'll accept this for the time being, and I'll try to find a better solution. Thank you all for helping out 😺

friederbluemle mentioned this pull request Apr 5, 2020

No directories found #70

Closed

marcins reviewed Apr 5, 2020

View reviewed changes

friederbluemle force-pushed the fix-du-macos branch from 2dc190b to a828828 Compare April 5, 2020 08:21

Caballerog changed the base branch from master to develop April 5, 2020 19:12

tonivj5 approved these changes Apr 5, 2020

View reviewed changes

Fix du parameters on macOS

2566e9a

friederbluemle force-pushed the fix-du-macos branch from a828828 to 2566e9a Compare April 7, 2020 01:37

Caballerog self-requested a review April 7, 2020 10:58

NyaGarcia merged commit d3b2005 into voidcosmos:develop May 20, 2020

friederbluemle deleted the fix-du-macos branch May 20, 2020 19:58

NyaGarcia mentioned this pull request Jul 26, 2020

Yarn cache caused impossible total size #13

Closed

tonivj5 mentioned this pull request Jul 28, 2020

Fix/du #80

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix du parameters on macOS #74

Fix du parameters on macOS #74

friederbluemle commented Apr 5, 2020 •

edited

marcins Apr 5, 2020

friederbluemle Apr 5, 2020

friederbluemle Apr 5, 2020

friederbluemle Apr 5, 2020 •

edited

tonivj5 commented Apr 5, 2020

tonivj5 commented Apr 5, 2020 •

edited

friederbluemle commented Apr 6, 2020

tonivj5 commented Apr 6, 2020

friederbluemle commented Apr 7, 2020

friederbluemle commented Apr 7, 2020

friederbluemle commented Apr 7, 2020

tonivj5 commented Apr 12, 2020

NyaGarcia commented May 20, 2020

Fix du parameters on macOS #74

Fix du parameters on macOS #74

Conversation

friederbluemle commented Apr 5, 2020 • edited

marcins Apr 5, 2020

Choose a reason for hiding this comment

friederbluemle Apr 5, 2020

Choose a reason for hiding this comment

friederbluemle Apr 5, 2020

Choose a reason for hiding this comment

friederbluemle Apr 5, 2020 • edited

Choose a reason for hiding this comment

tonivj5 commented Apr 5, 2020

tonivj5 commented Apr 5, 2020 • edited

friederbluemle commented Apr 6, 2020

tonivj5 commented Apr 6, 2020

friederbluemle commented Apr 7, 2020

friederbluemle commented Apr 7, 2020

friederbluemle commented Apr 7, 2020

tonivj5 commented Apr 12, 2020

NyaGarcia commented May 20, 2020

friederbluemle commented Apr 5, 2020 •

edited

friederbluemle Apr 5, 2020 •

edited

tonivj5 commented Apr 5, 2020 •

edited