Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

flutter: Construct SDK from Git repository + flutter precache #262789

Merged
merged 34 commits into from
Dec 21, 2023

Conversation

hacker1024
Copy link
Member

@hacker1024 hacker1024 commented Oct 22, 2023

Description of changes

This PR rethinks the approach to building Flutter and packaging artifacts.

Existing issues

  1. A different Flutter SDK tarball is required for every platform, even though the differences between them are minimal. This is a waste of resources, especially for maintainers and the official binary cache.
  2. SDK tarballs contain components we do not need, such as the Dart SDK and pre-cached Dart packages. This is a significant waste of space.
  3. Keeping track of all the engine artifact behaviour is challenging. Our current system needs to re-implement much of the logic from the Flutter tool in Nix, often with ugly hacks as it changes - Dart can be refactored a lot more easily than Nix can in this case.
  4. We have not yet packaged non-engine artifacts, which have even more flexible installation processes. If we ever want more control over them, working them in to the existing system will be very hard.

Changes in this PR

  • The Flutter SDK is set up directly from the Git repository - no tarballs required.
  • The Flutter CLI is built in a standard way, with buildDartApplication.
  • All artifacts are fetched with flutter precache and packed into fixed-output derivations (one per platform).

With these changes, we save a lot of resources, reduce the build complexity, and remove the need to keep up with the artifact logic in the Flutter tool.

Drawbacks

  • There is less granularity over artifact inclusion. In practice, this shouldn't change much - you need most of the platform artifacts to develop for a platform anyway.
  • The Flutter SDK no longer knows its own version, as there is no way to reproducibly keep .git directory. This leads to output from flutter doctor as shown below.
[!] Flutter (Channel [user-branch], 0.0.0-unknown, on NixOS 23.11 (Tapir) 6.5.7-zen1, locale en_AU.UTF-8)
    ! Flutter version 0.0.0-unknown on channel [user-branch] at /nix/store/h7bbwwy1hmmv2l2d7y1pf6h0165l04z4-flutter-wrapped-3.13.8-sdk-links
      Currently on an unknown channel. Run `flutter channel` to switch to an official channel.
      If that doesn't fix the issue, reinstall Flutter by following instructions at https://flutter.dev/docs/get-started/install.
      Cannot resolve current version, possibly due to local changes.
      Reinstall Flutter by following instructions at https://flutter.dev/docs/get-started/install.
    ! Unknown upstream repository.
      Reinstall Flutter by following instructions at https://flutter.dev/docs/get-started/install.
    • Framework revision ab75bef3f9 (54 years ago), 1970-01-01 00:00:00 +0000
    • Engine revision 767d8c75e8
    • Dart version 3.1.4
    • DevTools version 2.25.0
    • If those were intentional, you can disregard the above warnings; however it is recommended to use "git" directly to perform update checks and upgrades.

TODO

  • Web support is broken for now. WIP.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@NixOS/flutter

@hacker1024 hacker1024 marked this pull request as draft October 22, 2023 18:48
@hacker1024 hacker1024 mentioned this pull request Oct 22, 2023
12 tasks
@hacker1024 hacker1024 changed the title flutter: Construct SDK from Git directory flutter: Construct SDK from Git repository + flutter precache Oct 22, 2023
@mkg20001
Copy link
Member

can the flutter build code be patched to get the version from an environment variable given at build time?

@hacker1024
Copy link
Member Author

hacker1024 commented Oct 23, 2023

can the flutter build code be patched to get the version from an environment variable given at build time?

I looked into this, but the Flutter version code is actually quite complicated and involves large classes with numerous fields. A patch would be quite intrusive, and doesn't seem worth maintaining. I'm happy to be proven wrong, though.

@hacker1024
Copy link
Member Author

I've fixed the Web support. Turns out that some of flutter_tools's dependencies include static assets that the application tries to find by reading its own package_config.json, so that has to be made available along with the Pub package cache used during compilation.

@hacker1024 hacker1024 marked this pull request as ready for review October 23, 2023 04:51
Copy link
Contributor

@FlafyDev FlafyDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! That's a way cleaner way of fetching the artifacts. didn't know flutter had that command.
I left a few comments, but I might add more later since this pr is quite big and I still haven't had the time to see all of it

Also, thanks to this PR, we can remove the FLUTTER_CACHE_DIR variable.

  1. remove its section from move-cache.patch
  2. remove the line to set it in the immutableFlutter wrapper
  3. in fetch-artifacts.nix, do this instead:
  lndir -silent ${flutter} .
  HOME="$NIX_BUILD_TOP" FLUTTER_ROOT="." flutter precache -v '--${platform}' ${builtins.concatStringsSep " " (map (p: "'--no-${p}'") (lib.remove platform platforms))}
  find ./bin/cache -type l -exec rm {} \;
  cp -r ./bin/cache $out

pkgs/development/compilers/flutter/flutter.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/flutter/wrapper.nix Outdated Show resolved Hide resolved
@@ -120,15 +79,15 @@ let
# We do not patch it since the script doesn't require engine artifacts(which are the only thing not added by the unwrapped derivation), so it shouldn't fail, and patching it will just be harder to maintain.
immutableFlutter = writeShellScript "flutter_immutable" ''
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we are already refactoring this much, let's drop this writeShellScript and move the 2 environment variables it sets to the makeWrapper below.

        --set-default PUB_CACHE "\$HOME/.pub-cache" \
        --set-default FLUTTER_CACHE_DIR "${cacheDir}" \

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know, the C implementation of makeWrapper (which is being used currently) does not support using environment variables such as HOME.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I didn't know that. Also, where in the code do we specify to use the C implementation? is it by default now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure. It may have something to do with wrapGAppsHook.

@hacker1024
Copy link
Member Author

Nice! That's a way cleaner way of fetching the artifacts. didn't know flutter had that command. I left a few comments, but I might add more later since this pr is quite big and I still haven't had the time to see all of it

Also, thanks to this PR, we can remove the FLUTTER_CACHE_DIR variable.

1. remove its section from move-cache.patch

2. remove the line to set it in the immutableFlutter wrapper

3. in `fetch-artifacts.nix`, do this instead:
  lndir -silent ${flutter} .
  HOME="$NIX_BUILD_TOP" FLUTTER_ROOT="." flutter precache -v '--${platform}' ${builtins.concatStringsSep " " (map (p: "'--no-${p}'") (lib.remove platform platforms))}
  find ./bin/cache -type l -exec rm {} \;
  cp -r ./bin/cache $out

Nice, I was looking for a way to do this without accidentally including SDK files in the FOD.

My only concern with this method is that it may break if precache makes any links of its own, which it could technically do. This can be improved to only remove links going to the base flutter derivation, though.

@hacker1024
Copy link
Member Author

I have implemented the proposed idea to remove FLUTTER_CACHE_DIR.

@mkg20001
Copy link
Member

re version: another solution to fix the version wouldbe to create a mock git binary that just echos a preset output based on the arguments

or is the .git needed after build aswell?

@hacker1024
Copy link
Member Author

re version: another solution to fix the version wouldbe to create a mock git binary that just echos a preset output based on the arguments

or is the .git needed after build aswell?

It is needed at runtime, as the Flutter CLI uses it for version info all the time. We could add it to the wrapper, though.

That would be quite an effort though. A wide range of Git commands would need to be mocked.

@hacker1024
Copy link
Member Author

hacker1024 commented Oct 24, 2023

I've removed support for aarch64-darwin for now, as there are no host artifacts for the platform. In theory, the Flutter tool could be built natively, but the artifacts will always have to be used through Rosetta 2.

It's likely that the artifact hashes in this case will match x86_64-darwin, in which case support can be re-introduced - but I have no hardware to test on.

@ofborg ofborg bot requested a review from FlafyDev October 24, 2023 10:06
@hacker1024 hacker1024 mentioned this pull request Oct 25, 2023
13 tasks
Copy link
Contributor

@FlafyDev FlafyDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this PR is ready.
I left a few small comments.
Also, how do we get the hashes for the artifacts? It seems to depend on the host's platform to fetch them

@@ -0,0 +1,54 @@
{ lib
, runCommand
, xorg
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can be replaced with lndir directly

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is flutter still using shared.sh? If not I think its section in this patch can be removed

@maximoffua
Copy link
Contributor

I have managed to get Flutter version working without .git: just fill out file bin/cache/flutter.version.json

diff --git a/pkgs/flutter/flutter.nix b/pkgs/flutter/flutter.nix
index 40aa090..69efe67 100644
--- a/pkgs/flutter/flutter.nix
+++ b/pkgs/flutter/flutter.nix
@@ -13,7 +13,6 @@
 , darwin
 , git
 , which
+, runCommand
 }:
 
 let
@@ -23,19 +22,6 @@ let
     inherit patches;
     inherit pubspecLockFile vendorHash depsListFile;
   };
+  formatTimestamp = format: (builtins.readFile (runCommand "timestamp" {} "echo -n `date '+${format}'` > $out"));
+  devToolsVersion = (builtins.fromJSON (builtins.readFile "${dart}/bin/resources/devtools/version.json")).version;
+  flutterVersion = {
+    inherit devToolsVersion;
+    flutterVersion = "${version}";
+    frameworkVersion = "${version}";
+    channel = "stable";
+    repositoryUrl = "https://github.com/flutter/flutter.git";
+    frameworkRevision = "nixpkgs000000000000000000000000000000000";
+    frameworkCommitDate = formatTimestamp "%Y-%m-%d %H:%M:%S";
+    engineRevision = "${engineVersion}";
+    dartSdkVersion = "${dart.version}";
+  };
 
   unwrapped =
     stdenv.mkDerivation {
@@ -65,10 +51,9 @@ let
         git init -b nixpkgs
         GIT_AUTHOR_NAME=Nixpkgs GIT_COMMITTER_NAME=Nixpkgs \
         GIT_AUTHOR_EMAIL= GIT_COMMITTER_EMAIL= \
+        GIT_AUTHOR_DATE='${formatTimestamp "%m/%d/%Y %H:%M:%S %z"}' \
+        GIT_COMMITTER_DATE='${formatTimestamp "%m/%d/%Y %H:%M:%S %z"}' \
-        GIT_AUTHOR_DATE='1/1/1970 00:00:00 +0000' GIT_COMMITTER_DATE='1/1/1970 00:00:00 +0000' \
           git commit --allow-empty -m "Initial commit"
         (. '${../../../build-support/fetchgit/deterministic-git}'; make_deterministic_repo .)
 
         mkdir -p bin/cache
 
@@ -84,7 +69,6 @@ let
         ln -s '${tools.dartDeps.packageConfig}' packages/flutter_tools/.dart_tool/package_config.json
 
         echo -n "${version}" > version
+        echo -n '${builtins.toJSON flutterVersion}' | tee bin/cache/flutter.version.json
       '';
 
       installPhase = ''

This is necessary, for any flutter project which specifies minimal SDK version.

@maximoffua
Copy link
Contributor

maximoffua commented Dec 4, 2023

@hacker1024 could you please assist me with updating Flutter version in your flutter-from-source branch?

I have created Nix flake based on your work (see here). But I cannot understand how to update Flutter version, specifically these hash sums of artifacts. Can you give me a hint of how to obtain a download URL and a proper hash for every component from artifacts/hashes.nix and files in lockfiles/stable/?

@FlafyDev
Copy link
Contributor

FlafyDev commented Dec 13, 2023

@hacker1024 could you please assist me with updating Flutter version in your flutter-from-source branch?

I have created Nix flake based on your work (see here). But I cannot understand how to update Flutter version, specifically these hash sums of artifacts. Can you give me a hint of how to obtain a download URL and a proper hash for every component from artifacts/hashes.nix and files in lockfiles/stable/?

@maximoffua
I'm not sure myself as well. But I did manage to make a nix script to print the hashes after some modifications to the PR.

diff --git a/pkgs/development/compilers/flutter/artifacts/fetch-artifacts.nix b/pkgs/development/compilers/flutter/artifacts/fetch-artifacts.nix
index d8b237cb1e16..9100f0939658 100644
--- a/pkgs/development/compilers/flutter/artifacts/fetch-artifacts.nix
+++ b/pkgs/development/compilers/flutter/artifacts/fetch-artifacts.nix
@@ -5,6 +5,7 @@
 , unzip
 
 , platform
+, archPlatform
 , flutter
 , hash
 }:
@@ -25,9 +26,13 @@ let
     # Use a version of Flutter with just enough capabilities to download
     # artifacts.
     supportedTargetPlatforms = [ ];
+
+    flutter = flutter.unwrapped.override {
+      inherit archPlatform;
+    };
   };
 in
-runCommand "flutter-artifacts-${platform}"
+runCommand "flutter-artifacts-${platform}-${archPlatform}"
 {
   nativeBuildInputs = [ xorg.lndir flutter' unzip ];
 
diff --git a/pkgs/development/compilers/flutter/flutter-tools.nix b/pkgs/development/compilers/flutter/flutter-tools.nix
index 4435c8fb6c76..cde029dba8a5 100644
--- a/pkgs/development/compilers/flutter/flutter-tools.nix
+++ b/pkgs/development/compilers/flutter/flutter-tools.nix
@@ -1,4 +1,4 @@
-{ hostPlatform
+{ archPlatform
 , buildDartApplication
 , git
 , which
@@ -35,7 +35,7 @@ buildDartApplication.override { inherit dart; } rec {
   '';
 
   dartEntryPoints."flutter_tools.snapshot" = "bin/flutter_tools.dart";
-  dartCompileFlags = [ "--define=NIX_FLUTTER_HOST_PLATFORM=${hostPlatform.system}" ];
+  dartCompileFlags = [ "--define=NIX_FLUTTER_HOST_PLATFORM=${archPlatform}" ];
 
   # The Dart wrapper launchers are useless for the Flutter tool - it is designed
   # to be launched from a snapshot by the SDK.
diff --git a/pkgs/development/compilers/flutter/flutter.nix b/pkgs/development/compilers/flutter/flutter.nix
index 1e68a3b02a9e..0d21b6a00839 100644
--- a/pkgs/development/compilers/flutter/flutter.nix
+++ b/pkgs/development/compilers/flutter/flutter.nix
@@ -11,6 +11,7 @@
 , darwin
 , git
 , which
+, archPlatform ? stdenv.hostPlatform.system
 }:
 
 let
@@ -19,6 +20,7 @@ let
     flutterSrc = src;
     inherit patches;
     inherit pubspecLock;
+    inherit archPlatform;
   };
 
   unwrapped =
diff --git a/pkgs/development/compilers/flutter/wrapper.nix b/pkgs/development/compilers/flutter/wrapper.nix
index 3dc67f4b6d7d..53aa489d74f6 100644
--- a/pkgs/development/compilers/flutter/wrapper.nix
+++ b/pkgs/development/compilers/flutter/wrapper.nix
@@ -51,6 +51,7 @@ let
       src = callPackage ./artifacts/fetch-artifacts.nix {
         inherit platform;
         flutter = callPackage ./wrapper.nix { inherit flutter; };
+        archPlatform = stdenv.hostPlatform.system;
         hash = artifactHashes.${platform}.${stdenv.hostPlatform.system} or "";
       };
     }));

(Note that this patch was made against commit 35900eb in the PR #263345)
this patch adds an archPlatform argument so this script is possible:

{pkgs ? import ./. {}}:
pkgs.callPackage (
  {
    callPackage,
    flutter,
    lib,
    symlinkJoin,
  }: let
    flutterPlatforms = [
      # "android"
      # "ios"
      # "web"
      "linux"
      # "windows"
      # "macos"
      # "fuchsia"
      # "universal"
    ];
    archPlatforms = [
      "x86_64-linux"
      "aarch64-linux"
      "x86_64-darwin"
      # "aarch64-darwin"
    ];

    # After packaging a new flutter version, put it here.
    newFlutter = flutter;

    derivations =
      lib.foldl' (
        acc: flutterPlatform:
          acc
          ++ (map (archPlatform:
            callPackage ./pkgs/development/compilers/flutter/artifacts/fetch-artifacts.nix {
              platform = flutterPlatform;
              inherit archPlatform;
              hash = "";
            })
          archPlatforms)
      ) []
      flutterPlatforms;
  in
    # Only way I found to bulid multiple derivations...
    symlinkJoin {
      name = "evaluate-derivations";
      paths = derivations;
    }
) {}

put it in a nix file at the root of nixpkgs and run

nix build --keep-going --impure -L --expr 'import ./test.nix'

it should fail several times and print the hashes.

I'll try to make an update script for this.

@FlafyDev
Copy link
Contributor

FlafyDev commented Dec 13, 2023

I have managed to get Flutter version working without .git: just fill out file bin/cache/flutter.version.json

This is necessary, for any flutter project which specifies minimal SDK version.

@maximoffua Nice! I changed it a little so it doesn't cause IFD.

And I don't think the use of date is really necessary..

diff --git a/pkgs/development/compilers/flutter/flutter.nix b/pkgs/development/compilers/flutter/flutter.nix
index 1e68a3b02a9e..c78589997b43 100644
--- a/pkgs/development/compilers/flutter/flutter.nix
+++ b/pkgs/development/compilers/flutter/flutter.nix
@@ -11,6 +11,7 @@
 , darwin
 , git
 , which
+, jq
 }:
 
 let
@@ -27,7 +28,7 @@ let
       inherit src patches version;
 
       buildInputs = [ git ];
-      nativeBuildInputs = [ makeWrapper ]
+      nativeBuildInputs = [ makeWrapper jq ]
         ++ lib.optionals stdenv.hostPlatform.isDarwin [ darwin.DarwinTools ];
 
       preConfigure = ''
@@ -67,6 +68,19 @@ let
         ln -s '${tools.pubcache}/package_config.json' packages/flutter_tools/.dart_tool/package_config.json
 
         echo -n "${version}" > version
+        cat <<EOF > bin/cache/flutter.version.json
+        {
+          "devToolsVersion": "$(cat "${dart}/bin/resources/devtools/version.json" | jq -r .version)",
+          "flutterVersion": "${version}",
+          "frameworkVersion": "${version}",
+          "channel": "stable",
+          "repositoryUrl": "https://github.com/flutter/flutter.git",
+          "frameworkRevision": "nixpkgs000000000000000000000000000000000",
+          "frameworkCommitDate": "1970-01-01 00:00:00",
+          "engineRevision": "${engineVersion}",
+          "dartSdkVersion": "${dart.version}"
+        }
+        EOF
       '';
 
       installPhase = ''

(Note that this patch was made against commit 35900eb in the PR #263345)

@FlafyDev FlafyDev mentioned this pull request Dec 20, 2023
13 tasks
hacker1024 and others added 21 commits December 21, 2023 11:44
The Flutter CLI normally detects the host platform at runtime, but this results in incorrect behaviour in the Nix build environment.
There are no aarch64-darwin host artifacts.
Development should be done with Rosetta 2.

- flutter/flutter#60118
- flutter/flutter#69157
- flutter/flutter#101138
@mkg20001
Copy link
Member

I'm currently preparing this for merge (rebase, etc). Anything missing or can I go ahead with merge?

@FlafyDev
Copy link
Contributor

I'm currently preparing this for merge (rebase, etc). Anything missing or can I go ahead with merge?

I have a few things I want to add to the PR.

  • version in flutter doctor/--version
  • being able to update every hash for a potential update script

And a bit more stuff

Is there a way to add commits to this PR? Or should I make a new PR after this is merged?

@ofborg ofborg bot requested a review from FlafyDev December 21, 2023 13:14
@mkg20001
Copy link
Member

I suppose we can do it after merge. If you are committer you can push directly to this branch in hacker1024's repo (maybe maintainer can push too, not sure). But I prefer that we add new stuff afterwards, as this PR is already quite big.

@mkg20001 mkg20001 merged commit a2fc9f4 into NixOS:master Dec 21, 2023
21 of 22 checks passed
maximoffua added a commit to maximoffua/flutter.nix that referenced this pull request Dec 21, 2023
@linsui linsui mentioned this pull request Dec 24, 2023
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants