Re: [Buildroot] [PATCH v3 11/11] package/flutter-sdk-bin/Config.in.host: add pub-cache location option

["Yann E. MORIN" <yann.morin.1998@free.fr>]

Adam, All,

On 2024-01-02 16:59 -0700, Adam Duskett spake thusly:
> When running the command "flutter pub get," the plugins are stored in the
> pub-cache directory along with their sha256sum hashes. The default location of
> the pub-cache directory is current $(HOST_DIR)/share/flutter/sdk/.pub-cache,
> which is not an acceptable choice by default because every plugin is
> re-downloaded during every build of a flutter application either during a new
> build or when building with the per-package-directory option enabled.

This is a very good explanation of the problem, thanks! We indeed need
to find a better location; see below.

> Furthermore, keeping the pub-cache in its current location prevents users from
> committing the pub-cache directory to git for faster rebuilds of a
> Buildroot-based system, as a user cannot store the pub-cache for later use.

If the pub-cache is located in a sub-directory of BR2_DL_DIR, then they
will be able to keep that location between builds.

In case you did not know, you can export BR2_DL_DIR in the environment,
then Buildroot wil use that instead of $(TOPDIR)/dl/ and wil not remove
it when running "make clean" or "make distclean", effectively making the
directory pointed to by BR2_DL_DIR, an actual cache of downloaded files.

> To fix the above issue, add a new option to flutter-sdk-bin/Config.in.host,
> BR2_PACKAGE_HOST_FLUTTER_SDK_BIN_PUB_CACHE_LOCATION and set the default to
> $(DL_DIR)/br-flutter-pub-cache. While hard-coding the location is generally
> preferable, a developer may already have a working pub-cache at a different
> location. Setting this option to a string provides no downsides, and the
> default location is now set to a sane default, providing an optimal solution
> to the problem above.
> 
> Signed-off-by: Adam Duskett <adam.duskett@amarulasolutions.com>
> ---
>  package/flutter-sdk-bin/Config.in.host     | 11 +++++++++++
>  package/flutter-sdk-bin/flutter-sdk-bin.mk |  2 +-
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/package/flutter-sdk-bin/Config.in.host b/package/flutter-sdk-bin/Config.in.host
> index 181a2ee6e5..a1ac5835d9 100644
> --- a/package/flutter-sdk-bin/Config.in.host
> +++ b/package/flutter-sdk-bin/Config.in.host
> @@ -13,3 +13,14 @@ config BR2_PACKAGE_HOST_FLUTTER_SDK_BIN
>  	  free and open source.
>  
>  	  https://flutter.dev/
> +
> +if BR2_PACKAGE_HOST_FLUTTER_SDK_BIN_ARCH_SUPPORTS
> +
> +config BR2_PACKAGE_HOST_FLUTTER_SDK_BIN_PUB_CACHE_LOCATION
> +	string "pub-cache location"
> +	default "$(DL_DIR)/br-flutter-pub-cache"
> +	help
> +	  Directory to store cached packages used by Pub with
> +	  Dart/Flutter.
> +
> +endif
> diff --git a/package/flutter-sdk-bin/flutter-sdk-bin.mk b/package/flutter-sdk-bin/flutter-sdk-bin.mk
> index e7fd09cb56..5edd53df28 100644
> --- a/package/flutter-sdk-bin/flutter-sdk-bin.mk
> +++ b/package/flutter-sdk-bin/flutter-sdk-bin.mk
> @@ -105,4 +105,4 @@ HOST_FLUTTER_SDK_BIN_DART_BIN = \
>  $(eval $(host-generic-package))
>  
>  # For target packages to locate said pub-cache
> -FLUTTER_SDK_BIN_PUB_CACHE = $(HOST_FLUTTER_SDK_BIN_SDK)/.pub-cache
> +FLUTTER_SDK_BIN_PUB_CACHE = $(call qstrip,$(BR2_PACKAGE_HOST_FLUTTER_SDK_BIN_PUB_CACHE_LOCATION))

So, as Arnout and I already replied when we reviewed the first iteration
of this patch, this should not be a configurable item; rather, it should
just be hard-coded to point to: $(BR2_DL_DIR)/br-flutter-pub-cache
(like we already have $(BR2_DL_DIR)/br-cargo-home for example, which
serves a very similar purpose).

Furthermore, as Arnout noticed, the current code for the package
host-flutter-sdk-bin will just promptly remove the currently hard-coded
pub-cache directory:

    package/flutter-sdk-bin/flutter-sdk-bin.mk
    44 # Remove the cache, as we will run precache after setting up flutter and dart
    45 # with the new config options.
    46 define HOST_FLUTTER_SDK_BIN_BUILD_CMDS
    47 »       mkdir -p $(HOST_FLUTTER_SDK_BIN_SDK)
    48 »       rm -rf $(HOST_FLUTTER_SDK_BIN_SDK)/.pub-cache
    49 »       cd $(@D) && \
    50 »       »       $(HOST_FLUTTER_SDK_BIN_ENV) $(@D)/bin/flutter precache;
    51 endef

The code above then becomes wrong, because the pub-cache is no longer
located in that hard-coded location, so instead of the hard-coded value,
it should be changed to use $(FLUTTER_SDK_BIN_PUB_CACHE).

So, making the pub-cache location configurable will only ensure that a
pre-filled cache, potentially shared with non-Buildroot tools), would be
removed when using Buildroot, which goes directly against the stated
goal for that patch.

I am sorry, but I am going to reject this patch, and suggest that the
pub-cache be placed in a sub-directory of BR2_DL_DIR, as the previous
reviews suggested, and as described and explained above.

Regards,
Yann E. MORIN.

> -- 
> 2.43.0
> 
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot