Re: [PATCH setup] Add new option '--compact-os'

[Christian Franke <Christian.Franke@t-online.de>]

Jon Turney wrote:
> On 08/05/2021 21:03, Christian Franke wrote:
> ...
>
>> definitions are still missing in the current headers.
>
> Let me encourage you to submit those to MinGW-w64 so they end up in 
> the w32api package.

Done: https://sourceforge.net/p/mingw-w64/mailman/message/37280923/


>
>> Test results with 64bit Cygwin (Disk space used without / with 
>> --compact-os):
>>
>> Base installation: 135MiB / 66,1 MiB (-51%)
>> Installation with g++, Mingw, Perl, Python, Tex, ...:  2.19GiB / 
>> 854MiB (-62%)
>>
>> Base installation with NTFS compression: 78.7MiB (results in 
>> significant file fragmentation, Compact OS does not)
>
> Nice.
>
> A few minor comments.
>
>
>> ...
>> @@ -0,0 +1,62 @@
>> +//
>> +// compactos.cc
>> +//
>> +// Copyright (C) 2021 Christian Franke
>> +//
>> +// SPDX-License-Identifier: MIT
>> +//
>> +
>> +#include "compactos.h"
>> +
>> +#ifndef FSCTL_SET_EXTERNAL_BACKING
>
> There should be a comment here saying "not yet provided by w32api" or 
> similar.

... or we wait for a release of w32api headers with the patch mentioned 
above :-)


>
>> ...
>> +#ifndef COMPACTOS_H
>> +#define COMPACTOS_H
>> +
>> +#ifndef _INC_WINDOWS
>
> I hope windows.h already has it's own include guard?

Yes, _INC_WINDOWS should be removed.


>
> ...
>> +bool io_stream_cygfile::compact_os_is_available = (OSMajorVersion () 
>> >= 10);
>
> The documentation seems a bit vague, but are we really expecting this 
> to work on Windows 10 1507?

Not tested with 1507. With an old 1511 VBox VM, the command 'compact /C 
/EXE:LZX' works, so this I/O-control should work also.
(BTW: Caution: 'compact /C /EXE:...' does not preserve last write time - 
this is IMO a bug)


>
>> ...
>> -io_stream_cygfile::io_stream_cygfile (const std::string& name, const 
>> std::string& mode, mode_t perms) : fp(), lasterr (0), fname(), wname 
>> (NULL)
>> +static bool
>> +compactos_is_useless (const std::string& name)
>
> Something like 'compression_useful' might be a bit clearer?

I intentionally selected 'useless' because the negation is only 
'possibly_useful'. Compression might still "fail" with 
ERROR_COMPRESSION_NOT_BENEFICIAL.


>
>> +{
>> +  const char * const p = name.c_str();
>> +  if (!(!strncmp (p, "/bin/", 5) || !strncmp (p, "/sbin/", 6) || 
>> !strncmp (p, "/usr/", 5)))
>> +    return true; /* File is not in R/O tree. */
>> +  const size_t len = name.size(); /* >= 5 */
>> +  if (!strcmp (p + (len - 4), ".dll") || !strcmp (p + (len - 3), 
>> ".so"))
>> +    return true; /* Rebase will open file for writing which 
>> uncompresses the file. */
>> +  if (!strcmp (p + (len - 3), ".gz") || !strcmp (p + (len - 3), ".xz"))
>> +    return true; /* File is already compressed. */
>
> Is this an assertion that there are no .bz2, .lzma, .zst etc. files in 
> the install?
>

No, but there are only a few occurrences in packages (except src 
packages). Extension .bz2 occurs more often, so it should possibly be 
added. Adding all compression formats is IMO not worth the effort.

Even applying the compression to all files would be safe. Any too small 
or non-compressible file would result in 
ERROR_COMPRESSION_NOT_BENEFICIAL. Any later open for write access would 
silently uncompress the file.