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: Separate cache and unwrapped derivations #2 #240715

Merged
merged 1 commit into from
Jul 5, 2023

Conversation

FlafyDev
Copy link
Contributor

This PR is a continuation of 238177 (Sorry for the trouble)

flutter-unwrapped will now not come with engine artifacts in its cache directory.
To specify a different cache directory, set FLUTTER_CACHE_DIR

Before this PR, changing which artifacts Flutter will have required rebuilding Flutter. This is because Flutter's unwrapped derivation required symlinking the engine artifacts to the cache directory($out/bin/cache).

So this:

pkgs.flutter.override {
  supportsAndroid = false;
  supportsLinuxDesktop = true;
}

And this:

pkgs.flutter.override {
  supportsAndroid = true;
  supportsLinuxDesktop = true;
}

Would build Flutter twice, even though it's not necessary.

To fix this, this PR moved the cache directory to an environment variable(when specified). This allows changing which artifacts
Flutter sees at runtime, so no need to rebuild.

Additionally, the wrapper has been adjusted to this change and will now provide the engine artifacts through FLUTTER_CACHE_DIR.

Note

The sh file $out/bin/internal/shared.sh runs when launching Flutter and calls $FLUTTER_ROOT/bin/cache/ instead of our environment variable FLUTTER_CACHE_DIR.
I decided not to 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.

Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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.

flutter-unwrapped will now not come with engine artifacts in its cache directory(`$out/bin/cache`).

To specify a different cache directory, set FLUTTER_CACHE_DIR.

Flutter's wrapper now sets FLUTTER_CACHE_DIR to set engine artifacts.

The sh file `$out/bin/internal/shared.sh` runs when launching Flutter and calls `"$FLUTTER_ROOT/bin/cache/` instead of our environment variable `FLUTTER_CACHE_DIR`.
I decided not to 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.
@FlafyDev FlafyDev force-pushed the flutter-cache-drv-2 branch from d81c367 to 570f3ef Compare June 30, 2023 12:23
@FlafyDev
Copy link
Contributor Author

CC @hacker1024 @gilice @SuperSandro2000


Please add lndir to nativeBuildInputs

Done


Can you try to reduce the nested let ins here? Having more variables in a higher scope would be probably preferable.

Done. I reduced most of the let ins.


Can we move this out of the ?? otherwise changing one value here requires copying all of this.

I'm not sure where to move the default value to.
And like @hacker1024 said:

I don't think it's such a big deal. Someone who chooses to configure this will probably not need half of these defaults anyway and simply choose exactly what they need without complex conditionals.

you would usually edit supportsLinuxDesktop or supportsAndroid. If you need more control over the artifacts, you edit includedEngineArtifacts

@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 11-100 labels Jun 30, 2023
@FlafyDev
Copy link
Contributor Author

Result of nixpkgs-review pr 240715 run on x86_64-linux 1

16 packages built:
  • firmware-updater
  • firmware-updater.debug
  • fluffychat
  • fluffychat.debug
  • flutter
  • flutter-unwrapped (flutterPackages.stable)
  • flutter-unwrapped.cache (flutterPackages.stable.cache)
  • flutter2
  • flutter2-unwrapped (flutterPackages.v2)
  • flutter2-unwrapped.cache (flutterPackages.v2.cache)
  • flutter37
  • flutter37-unwrapped (flutterPackages.v37)
  • flutter37-unwrapped.cache (flutterPackages.v37.cache)
  • hover
  • yubioath-flutter
  • yubioath-flutter.debug

@figsoda figsoda added 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package labels Jul 1, 2023
@Kranzes Kranzes merged commit d625c36 into NixOS:master Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 11-100 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants