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

[Jon Turney <jon.turney@dronecode.org.uk>]

On 08/05/2021 21:03, Christian Franke wrote:
> This experimental patch allows to reduce the footprint of a Cygwin 
> installation on Windows 10.

Thanks.

> The I/O-control itself is in a new file compactos.cc because 
> WIN32_LEAN_AND_MEAN (win32.h) must not be used and some required 

Yeah, I think "Any include of <windows.h> should be through this file" 
is more a guideline than a rule, since we already break it in other 
places :)

> 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.

> 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.

> Date: Sat, 8 May 2021 21:25:07 +0200
> Subject: [PATCH] Add new option '--compact-os'.
[...]> --- /dev/null
> +++ b/compactos.cc
> @@ -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.

> diff --git a/compactos.h b/compactos.h
> new file mode 100644
> index 0000000..c1470f1
> --- /dev/null
> +++ b/compactos.h
> @@ -0,0 +1,26 @@
> +//
> +// compactos.h
> +//
> +// Copyright (C) 2021 Christian Franke
> +//
> +// SPDX-License-Identifier: MIT
> +//
> +
> +#ifndef COMPACTOS_H
> +#define COMPACTOS_H
> +
> +#ifndef _INC_WINDOWS

I hope windows.h already has it's own include guard?

> +#include <windows.h>
> +#endif
[...]
> +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?

> +
>  // Normalise a unix style path relative to 
>  // cwd.
>  std::string
> @@ -120,7 +125,22 @@ get_root_dir_now ()
>    read_mounts (std::string ());
>  }
>  
> -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?

> +{
> +  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?