Teach Linux to use local engine by jonahwilliams · Pull Request #31631 · flutter/flutter

jonahwilliams · 2019-04-25T23:30:44Z

Description

Copy the linux artifacts!

Related Issues

stuartmorgan-g

I'm fine with landing this approach (modulo the comment below about the timestamp logic, which I'm not sure will ever actually be used as written), but it might be easier to just skip ahead to absorbing FDE's copy script into flutter_tool and having the copy done on this side, since that's the goal anyway.

stuartmorgan-g · 2019-04-26T19:39:34Z

packages/flutter_tools/lib/src/linux/build_linux.dart

+    // Otherwise we can use the engine stamp.
+    stampContents = Cache.instance.getStampFileFor('linux-sdk').readAsStringSync();
+  }
+  linuxProject.editableHostAppDirectory.childFile('.generated_engine_stamp')


Per comments on the other PR, I'm not sure it's useful to add this logic until flutter_tool is also doing the copy. At that point, it'll be easy to read the file and do logic on it (the way the current FDE copy script does).

I've moved the copy the logic to the flutter tool, per my understanding of what the Makefile is doing...

…_asset_sync

jonahwilliams · 2019-04-30T01:26:50Z

Sorry for the delay on this. I got everything mostly working locally and then noticed buttons weren't working on linux, so I had to take a trip to revert-land.

…utter into land_desktop_asset_sync

stuartmorgan-g · 2019-04-30T22:56:10Z

packages/flutter_tools/lib/src/linux/build_linux.dart

-
  final String buildFlag = buildInfo?.isDebug == true ? 'debug' : 'release';
+  final Directory cacheDirectory = fs.directory(artifacts.getEngineArtifactsPath(TargetPlatform.linux_x64));
+  final Directory buildDirectory = fs.directory(fs.path.join(getLinuxBuildDirectory(), 'cache', 'flutter_library'));


I think we should take this opportunity to standardize how/where we're pushing files. On macOS we have a hybrid of what FDE happened to be doing (putting the framework in the the build output directly) and what iOS does (putting everything in a Flutter/ folder), which we'll have to fix as a breaking change since we didn't adjust it at the time (which was an oversight on my part).

Let's match the iOS (and I assume Android? I haven't dug into it) structure of having a single folder where we push files. For macOS we can make it Flutter to match iOS; here we should probably use flutter since the capital letter folder things is an Xcode-project-ism.

So we'd make editableHostAppDirectory be linux/flutter instead of linux, and put .generated_flutter_root (which should lose the . in this new structure, I think) and all these files in that folder, and not put anything in the build directory.

stuartmorgan-g · 2019-04-30T23:00:29Z

packages/flutter_tools/lib/src/linux/build_linux.dart

+  copyDirectorySync(cacheDirectory, buildDirectory);
+
+  // Copy the ICU data.
+  final File icuDestination = fs.file(fs.path.join(getLinuxBuildDirectory(), buildFlag, 'data', _kIcuDataName));


The copy targets not be directly in the build output directory. The data and lib directories are for the actual binary output location; the idea is that the makefile copies everything into those locations so that there's a directory that could be zipped up and moved until there's a real installer plan for Linux. But that's something that should be done by the Makefile on the app side (copying from cache/flutter_library/, or with the above change just flutter/ to the bundle target location), not the flutter_tool side.

stuartmorgan-g

Looks good; holding off on approving until the FDE patch is updated and reviewed since we'll need to land them at ~the same time since it's now a breaking change.

packages/flutter_tools/lib/src/linux/build_linux.dart

jonahwilliams · 2019-05-01T17:52:13Z

Updated

…_asset_sync

stuartmorgan-g

Thanks, looks good structurally!

What's the reason for this new script to be a bash script instead of a dart script with a simple .sh that invokes it? We need exactly this logic for Windows too, and rewriting a .bat version rather than just making this Dart from the start seems like unnecessary extra work (especially since I thought we wanted to move away from having new scripts be something other than Dart anyway).

packages/flutter_tools/bin/linux_backend.sh

jonahwilliams · 2019-05-06T23:53:59Z

I did spend some time investigating moving this to dart, I don't see any big wins yet. File copying constitutes such a big part of the process and yet is so much easier in bash 🤷‍♂️

stuartmorgan-g · 2019-05-07T00:07:26Z

I did spend some time investigating moving this to dart, I don't see any big wins yet.

The big win is not having to then also write it again it batch and then maintaining two ~identical scripts in two different languages for the foreseeable future.

File copying constitutes such a big part of the process and yet is so much easier in bash 🤷‍♂

An earlier version of this patch did at least the folder copy (and I thought some of the other parts) in Dart, and that code was just a few lines. Is there extra complexity somewhere?

stuartmorgan-g · 2019-05-07T00:12:17Z

and then maintaining

And keep in mind that we already know we are going to need to make non-trivial changes to the logic here, because re-copying the files on every build is unsustainable; at some point we really need to address the fact that this new build workflow is a significant regression from FDE's due to needlessly thrashing the timestamps of files that are part of the rebuild/relink analysis steps of the native builds. So there is going to be real maintenance cost going forward.

(And we already have a potential solution to that, in the form of re-using the logic from FDE's existing copy script--which is written in Dart.)

jonahwilliams · 2019-05-07T17:06:35Z

packages/flutter_tools/bin/linux_backend.sh

+  local_engine_flag="--local-engine=${LOCAL_ENGINE}"
+fi
+
+RunCommand "${FLUTTER_ROOT}/bin/flutter" --suppress-analytics               \


How do you feel about this level of breakdown between the tool API and the build script? It could be generalized to support linux, macos, and friends.

The alternative, would be to teach the makefile to invoke the commands separately as part of the build.

I'm fine with the copy and the build build being lumped together like this; I haven't run into anything I can think of that would make it problematic, and it ensures that the engine flags stay matched.

stuartmorgan-g · 2019-05-07T19:48:39Z

packages/flutter_tools/bin/linux_backend.sh

+  local_engine_flag="--local-engine=${LOCAL_ENGINE}"
+fi
+
+RunCommand "${FLUTTER_ROOT}/bin/flutter" --suppress-analytics               \


I'm fine with the copy and the build build being lumped together like this; I haven't run into anything I can think of that would make it problematic, and it ensures that the engine flags stay matched.

packages/flutter_tools/bin/linux_backend.sh

packages/flutter_tools/lib/src/commands/unpack.dart

stuartmorgan-g · 2019-05-07T19:56:17Z

packages/flutter_tools/lib/src/commands/unpack.dart

+        for (final String filename in artifactFiles) {
+          final String sourcePath = fs.path.join(sourceDirectory, filename);
+          final String targetPath = fs.path.join(targetDirectory, filename);
+          if (filename.endsWith('/')) {


I don't remember why I did this; we should probably just check the source type to see if it's a directory or not instead of relying on a marker in the filename.

using isDirectorySync is returning false for some reason, I ended up leaving it as the name check

stuartmorgan-g · 2019-05-10T21:53:17Z

packages/flutter_tools/bin/tool_backend.dart

+  String cacheDirectory;
+  switch (targetPlatform) {
+    case 'linux-x64':
+      cacheDirectory = 'linux/flutter/cache';


Remove 'cache'

stuartmorgan-g · 2019-05-10T21:54:28Z

packages/flutter_tools/bin/tool_backend.dart

+      if (flutterEngine != null) '--local-engine-src-path=$flutterEngine',
+      if (localEngine != null) '--local-engine=$localEngine',
+    ],
+    runInShell: true,


These shouldn't be runInShell

…utter into land_desktop_asset_sync

teach linux to use local engine

4fd436a

googlebot added the cla: yes label Apr 25, 2019

jonahwilliams mentioned this pull request Apr 25, 2019

Add support for local engine to Linux google/flutter-desktop-embedding#355

Merged

fix test and add case for local engine artifacts

e1fcfd5

jonahwilliams requested a review from stuartmorgan-g April 26, 2019 15:39

stuartmorgan-g reviewed Apr 26, 2019

View reviewed changes

jonahwilliams added the tool Affects the "flutter" command-line tool. See also t: labels. label Apr 29, 2019

jonahwilliams added 2 commits April 29, 2019 12:53

Merge branch 'master' of github.com:flutter/flutter into land_desktop…

9629a78

…_asset_sync

Merge branch 'master' of github.com:flutter/flutter into land_desktop…

4279a4a

…_asset_sync

jonahwilliams added 4 commits April 30, 2019 12:43

move asset copy to flutter_tool

c7948bd

Merge branch 'land_desktop_asset_sync' of github.com:jonahwilliams/fl…

e3ffc86

…utter into land_desktop_asset_sync

remove extra flag

0605cce

remove unused imports

5821456

jonahwilliams requested a review from stuartmorgan-g April 30, 2019 20:44

stuartmorgan-g reviewed Apr 30, 2019

View reviewed changes

jonahwilliams added 2 commits April 30, 2019 17:02

only copy to cache directory

ce7edc3

dont forget to commit

42654dd

stuartmorgan-g reviewed May 1, 2019

View reviewed changes

packages/flutter_tools/lib/src/linux/build_linux.dart Outdated Show resolved Hide resolved

fix comment

da2dbd6

jonahwilliams added 2 commits May 3, 2019 10:07

Merge branch 'master' of github.com:flutter/flutter into land_desktop…

6240995

…_asset_sync

Generate config file that can be read by makefile for linux

8c08fd1

jonahwilliams force-pushed the land_desktop_asset_sync branch from b0ff141 to 8c08fd1 Compare May 3, 2019 17:10

jonahwilliams added 6 commits May 3, 2019 12:55

standardize config format

8e9903e

support local engine

d0b8e4a

Delete .project

41854d5

Delete org.eclipse.buildship.core.prefs

1904989

Delete .project

6d3ef01

Delete org.eclipse.buildship.core.prefs

b3c39be

dont add a bunch of weird files

7bfbf92

stuartmorgan-g reviewed May 5, 2019

View reviewed changes

packages/flutter_tools/bin/linux_backend.sh Outdated Show resolved Hide resolved

packages/flutter_tools/bin/linux_backend.sh Outdated Show resolved Hide resolved

fixup backend

51e0d2b

upstream desktop asset syncer

6e8addb

jonahwilliams commented May 7, 2019

View reviewed changes

stuartmorgan-g reviewed May 7, 2019

View reviewed changes

jonahwilliams added 3 commits May 10, 2019 08:29

add dart based tool backend

5191a8c

Update tool_backend.dart

377aaa0

Update tool_backend.dart

9b4dcf4

stuartmorgan-g approved these changes May 10, 2019

View reviewed changes

jonahwilliams added 2 commits May 10, 2019 14:59

address comments

fe92255

Merge branch 'land_desktop_asset_sync' of github.com:jonahwilliams/fl…

f457f80

…utter into land_desktop_asset_sync

dkwingsmt mentioned this pull request May 11, 2019

Mouse entering/exiting a desktop window causes shadow change #32529

Closed

jonahwilliams merged commit 8b0243f into flutter:master May 11, 2019

jonahwilliams deleted the land_desktop_asset_sync branch May 11, 2019 07:08

github-actions bot locked as resolved and limited conversation to collaborators Aug 6, 2021

Conversation

jonahwilliams commented Apr 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Apr 30, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonahwilliams commented May 1, 2019

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jonahwilliams commented May 6, 2019

Uh oh!

stuartmorgan-g commented May 7, 2019

Uh oh!

stuartmorgan-g commented May 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jonahwilliams commented Apr 25, 2019 •

edited

Loading