Skip to content

Flutter fixup#160942

Merged
mkg20001 merged 10 commits intoNixOS:masterfrom
mkg20001:flutter-fixup
Feb 23, 2022
Merged

Flutter fixup#160942
mkg20001 merged 10 commits intoNixOS:masterfrom
mkg20001:flutter-fixup

Conversation

@mkg20001
Copy link
Copy Markdown
Member

@mkg20001 mkg20001 commented Feb 19, 2022

Motivation for this change

Packaging some more things for flutter to check and smooth out the raw edges of mkFlutterApp

If yov've got any comments regarding #147107 this is the place to comment

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/)
  • 22.05 Release Notes (or backporting 21.11 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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Feb 19, 2022
@mkg20001 mkg20001 force-pushed the flutter-fixup branch 2 times, most recently from dec03b6 to 631bc0f Compare February 20, 2022 08:59
@mkg20001 mkg20001 marked this pull request as ready for review February 20, 2022 09:04
Comment on lines 232 to 247
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is this? Why not just use buildPhase with preHooks? Looks to me a lot easier overhaul.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

How do you mean with prehooks? The idea is the user is able to overwrite buildPhase but can optionally call the original one somewhere in the middle of it. What would be the best way to accomplish this?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you want to run something before you would put it into preBuild and for after into postBuild. Basically this would already achieve this:

Suggested change
preConfigurePhases = [ "flutterConfigurePhase" ];
# avoid cmake phase
configurePhase = "true";
flutterBuildPhase = ''
# for some reason fluffychat build breaks without this - seems file gets overriden by some tool
mv pubspec-backup pubspec.yaml
mkdir -p build/flutter_assets/fonts
flutter packages get --offline -v
flutter build linux --release -v
'';
buildPhase = ''
runHook preBuild
runHook flutterBuildPhase
runHook postBuild
'';
preConfigurePhases = [ "flutterConfigurePhase" ];
# avoid cmake phase
configurePhase = "true";
buildPhase = ''
runHook preBuild
# for some reason fluffychat build breaks without this - seems file gets overriden by some tool
mv pubspec-backup pubspec.yaml
mkdir -p build/flutter_assets/fonts
flutter packages get --offline -v
flutter build linux --release -v
runHook postBuild
'';

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hrm I'll leave the phases as-is for now. It was initially added to package appflowy, but that is already packaged as bin derivation.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
configurePhase = "true";
dontConfigure = true;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The user might want to overwrite configurePhase and then it would be skipped. Also using preConfigurePhases not sure if this will work.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What happens if we just leave configurePhase and don't do anything to it? Maybe we just want to disable cmake and ninja?

@mkg20001 mkg20001 force-pushed the flutter-fixup branch 2 times, most recently from f469250 to c3fce32 Compare February 20, 2022 17:37
mkg20001 and others added 10 commits February 23, 2022 15:02
Otherwise it tries to run some other commands, this prevents that

Co-authored-by: Sandro <sandro.jaeckel@gmail.com>
Co-authored-by: Sandro <sandro.jaeckel@gmail.com>
Co-authored-by: Sandro <sandro.jaeckel@gmail.com>
Previously this was pulling from $built, which got moved to $out/app,
so the glob didn't do anything. Now uses find on $out/app
@mkg20001 mkg20001 merged commit f724c46 into NixOS:master Feb 23, 2022
@mkg20001 mkg20001 deleted the flutter-fixup branch February 23, 2022 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants