MVP for passing in custom config path #461

toastal · 2021-02-09T16:08:30Z

I have never used TypeScript and am not familiar with the tooling or coding styles of this project so I did the bare minimum that got my code running and your tests green (with no new tests). I believe this can address some of this issues in #457 in conjunction with process substitution in Bash, Fish, etc. to make a JSON "file" on the fly.

toastal · 2021-02-09T16:20:10Z

Considering I got here in fish

$ bubblewrap init --config .bubblewrap/config.json --manifest (dhall-to-json --file manifest.dhall | psub)                                                                                                 bosthlm 
,-----.        ,--.  ,--.  ,--.
|  |) /_,--.,--|  |-.|  |-.|  |,---.,--.   ,--,--.--.,--,--.,---.
|  .-.  |  ||  | .-. | .-. |  | .-. |  |.'.|  |  .--' ,-.  | .-. |
|  '--' '  ''  | `-' | `-' |  \   --|   .'.   |  |  \ '-'  | '-' '
`------' `----' `---' `---'`--'`----'--'   '--`--'   `--`--|  |-'
                                                           `--'    
Initializing application from Web Manifest:
	-  /run/user/1000/.psub.8n72sG4Hcf


cli ERROR Only absolute URLs are supported

I would say it's working so far for me. I'll work on how to generate my own config file shortly.

toastal · 2021-02-09T16:26:59Z

So far my shell.nix is for being able to install bubblewrap via yarn inside the shell (not globally). libuuid was the missing library I did not see in the various docs but prevented canvas from running.

{
  pkgs ? import (fetchTarball "https://github.com/NixOS/nixpkgs/archive/3f0e6e1daaa071b38121029c12de7c260286d719.tar.gz")
    { config = { android_sdk.accept_license = true; }; }
}:

with pkgs.lib;

let
  bubblewrapBuildToolsVersion = "29.0.2";

  androidComposition = pkgs.androidenv.composeAndroidPackages {
    toolsVersion = "26.1.1";
    platformToolsVersion = "30.0.5";
    buildToolsVersions = [ bubblewrapBuildToolsVersion ];
    platformVersions = [ "29" "30" ];
  };


in pkgs.mkShell {
  name = "xxxxxx";

  basePackages = with pkgs; [
    gawk
    jq
    ripgrep
  ];

  nativeBuildInputs = with pkgs.buildPackages; [
    # node
    nodejs-14_x
    yarn

    # dhall
    dhall
    dhall-bash
    dhall-json

    # android
    androidComposition.androidsdk
    cairo
    giflib
    gnome2.pango
    jdk8_headless
    libjpeg
    libuuid
    librsvg
    pkg-config
  ];

  ANDROID_SDK_ROOT = "${androidComposition.androidsdk}/libexec/android-sdk";

  GRADLE_OPTS = "-Dorg.gradle.project.android.aapt2FromMavenOverride=${ANDROID_SDK_ROOT}/build-tools/${bubblewrapBuildToolsVersion}/aapt2";

  LD_LIBRARY_PATH = "${makeLibraryPath (with pkgs; [
    giflib
    libjpeg
    librsvg
    libuuid
  ])}";
}

and wrapping bubblewrap with a shell script

#!/usr/bin/env bash
set -f

project_root_dir="$(cd "$(dirname 0)/.."; pwd -P)"
node_bin="$(cd "$project_root_dir"; yarn bin)"

# Just a wrapper around `bubblewrap` to inject Nix store directories
# into a config file.

if [ -z "$IN_NIX_SHELL" ]; then
  $node_bin/bubblewrap "$@"
else
  dhall_expr="$(cat <<EOF
    { jdkPath = "$(nix-build --no-out-link '<nixpkgs>' -A jdk8_headless)" : Text
    , androidSdkPath = "$(nix-store --query --referrers "$(which adb)")/libexec/android-sdk" : Text
    }
EOF
)"
  $node_bin/bubblewrap "$@" --config <(dhall-to-json --compact <<< $dhall_expr)
fi

andreban · 2021-02-09T16:59:40Z

packages/cli/src/lib/config.ts

-    prompt: Prompt = new InquirerPrompt(), path = DEFAULT_CONFIG_FILE_PATH): Promise<Config> {
-  await renameConfigIfNeeded(log);
-  const existingConfig = await Config.loadConfig(path);
+export async function loadOrCreateConfig(


I'm not sure I understand this change - it seems the end result is the same as the previous version.

In the section below, path is only assigned DEFAULT_CONFIG_FILE_PATH if the value passed is different from null and different from undefined:

export async function loadOrCreateConfig( prompt: Prompt = new InquirerPrompt(), path = DEFAULT_CONFIG_FILE_PATH): Promise<Config> {

so, generally analog to:

if (path === undefined) { await renameConfigIfNeeded(log); configPath = DEFAULT_CONFIG_FILE_PATH; } else { configPath = path; }

the only difference being the call to await renameConfigIfNeeded(log). But that call should be no-op in most cases (it's just about handling old config paths to the current one).

Well it is calling out to filesystem which is a slow and unnecessary operation if we are already sure we don't need to do it, right?

Not to mention you could be renaming a users input file which would be unexpected when rerunning a command.

Hitting the filesystem shouldn't be such a large overhead in this case and renaming will only happen for a very specific filename, so I'm generally happy with the solution. Nevertheless, those are good points.

It sounds to me loadOrCreateConfig() is doing too much. Maybe a better alternative is to rename this function to loadOrCreateDefaultConfig() and when parsedArgs.config is available just call Config.loadConfig() instead of this function.

PS: It's also possible to call Config.deserialize() to parse a JSON or build a config with a new Config(jdkPath, androidSdkPath). Would passing those strings from the command line make it a better solution for your use-case?

I don't want to be in charge of function naming.

I think this is a very fast solution that can work for a lot of odd use case as anyone can make a config on the fly but they can also check in and use a local config file (mine is at project/workspace/.bubblewrap/config.json). This mean a user could say put the config in package.json and use jq to read and pass in through process substitution.

I plan to use Dhall everywhere and dhall-to-json for my configuration because I want types for my configuration (makes me want my twa-manifest.json to be typed too which I'm working on). Also with this solution I can to call nix-build --no-out-link '<nixpkgs>' -A jdk8_headless to get the current version of JDK (same for Android SDK) on the fly which currently is at /nix/store/q839v7q4k8s958zs2smmwm6id72xjsva-openjdk-headless-8u272-b10. I need reproducible builds ... including all building tools and libraries, so everything needs to be local to nix-shell which includes bubblewrap and all its dependencies. I may make a derivation if time allows that contains all the bits that make this work on Nix.

andreban · 2021-02-09T17:04:34Z

packages/cli/src/lib/Cli.ts

    const parsedArgs = minimist(args);

+    const config = await loadOrCreateConfig(


If we revert loadOrCreateConfig(), we should be able to call loadOrCreateConfig(undefined, undefined, parsedArgs.config). So, the 1st and 2nd parameters get the default value and the last uses parsedArgs.config. If parsedArgs.config is null, it will use the default value.

Well, since there aren't named arguments, and they were cases where some were used and some not, I figured a dictionary would be cleaner than loadOrCreateConfig(undefined, undefined, parsedArgs.config), but I don't really care one way or another on the style.

andreban · 2021-02-09T17:05:27Z

@toastal thank you very much for this! The contribution is much appreciated! I dropped a couple of comments.

andreban · 2021-02-09T17:11:29Z

Considering I got here in fish

$ bubblewrap init --config .bubblewrap/config.json --manifest (dhall-to-json --file manifest.dhall | psub)                                                                                                 bosthlm 
,-----.        ,--.  ,--.  ,--.
|  |) /_,--.,--|  |-.|  |-.|  |,---.,--.   ,--,--.--.,--,--.,---.
|  .-.  |  ||  | .-. | .-. |  | .-. |  |.'.|  |  .--' ,-.  | .-. |
|  '--' '  ''  | `-' | `-' |  \   --|   .'.   |  |  \ '-'  | '-' '
`------' `----' `---' `---'`--'`----'--'   '--`--'   `--`--|  |-'
                                                           `--'    
Initializing application from Web Manifest:
	-  /run/user/1000/.psub.8n72sG4Hcf


cli ERROR Only absolute URLs are supported

I would say it's working so far for me. I'll work on how to generate my own config file shortly.

It seems to me you are bumping into #401 - we require an URL as the --manifest parameter.

toastal · 2021-02-10T02:20:00Z

I was going to make a separate twa-manifest.dhall file deriving from the manifest.dhall file I already have where I could include some of these new attributes. I REALLY wish the differences in these manifests were documented somewhere else other than in the config TS code. I tried searching for documentation about the twa-manifest.json and its additional properties, but got nothing.

Found it in the code though.

andreban · 2021-02-10T09:48:27Z

The best place to check for the twa-manifest.json values is this bit of code.

andreban

LGTM, Thank you! Shall I merge this then?

toastal · 2021-02-11T05:18:05Z

Yeah, that be great.

toastal · 2021-02-11T15:03:45Z

How often do you cut releases?

andreban · 2021-02-11T15:10:22Z

We don't have a fixed schedule, due to development not moving at a constant rate. I want to get #463 merged and update the docs with the --config option for the next release.

toastal force-pushed the mvp-custom-config branch from 636177e to 3ff800c Compare February 9, 2021 16:13

custom config path

45144b3

andreban reviewed Feb 9, 2021

View reviewed changes

optional argument pattern

54d8444

andreban approved these changes Feb 10, 2021

View reviewed changes

Merge branch 'master' into mvp-custom-config

c038e97

andreban merged commit d628122 into GoogleChromeLabs:master Feb 11, 2021

andreban added the enhancement New feature or request label Feb 11, 2021

andreban mentioned this pull request Feb 15, 2021

Allow local config / download + read JDK path from the environment #457

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MVP for passing in custom config path #461

MVP for passing in custom config path #461

toastal commented Feb 9, 2021

toastal commented Feb 9, 2021 •

edited

Loading

toastal commented Feb 9, 2021 •

edited

Loading

andreban Feb 9, 2021

toastal Feb 10, 2021

toastal Feb 10, 2021

andreban Feb 10, 2021

toastal Feb 10, 2021

andreban Feb 9, 2021 •

edited

Loading

toastal Feb 10, 2021

andreban commented Feb 9, 2021

andreban commented Feb 9, 2021

toastal commented Feb 10, 2021 •

edited

Loading

andreban commented Feb 10, 2021

andreban left a comment

toastal commented Feb 11, 2021

toastal commented Feb 11, 2021

andreban commented Feb 11, 2021

		const parsedArgs = minimist(args);

		const config = await loadOrCreateConfig(

MVP for passing in custom config path #461

MVP for passing in custom config path #461

Conversation

toastal commented Feb 9, 2021

toastal commented Feb 9, 2021 • edited Loading

toastal commented Feb 9, 2021 • edited Loading

andreban Feb 9, 2021

Choose a reason for hiding this comment

toastal Feb 10, 2021

Choose a reason for hiding this comment

toastal Feb 10, 2021

Choose a reason for hiding this comment

andreban Feb 10, 2021

Choose a reason for hiding this comment

toastal Feb 10, 2021

Choose a reason for hiding this comment

andreban Feb 9, 2021 • edited Loading

Choose a reason for hiding this comment

toastal Feb 10, 2021

Choose a reason for hiding this comment

andreban commented Feb 9, 2021

andreban commented Feb 9, 2021

toastal commented Feb 10, 2021 • edited Loading

andreban commented Feb 10, 2021

andreban left a comment

Choose a reason for hiding this comment

toastal commented Feb 11, 2021

toastal commented Feb 11, 2021

andreban commented Feb 11, 2021

toastal commented Feb 9, 2021 •

edited

Loading

toastal commented Feb 9, 2021 •

edited

Loading

andreban Feb 9, 2021 •

edited

Loading

toastal commented Feb 10, 2021 •

edited

Loading