[flutter_tools] Add additional bash entrypoint for running dart sdk directly by christopherfujino · Pull Request #57257 · flutter/flutter

christopherfujino · 2020-05-14T22:13:40Z

Description

The bash side of #53470.

The windows parts of this PR were reviewed in #56845.

This PR:

Moves the previous //bin/flutter shell script to //bin/shared.sh.
The main execution code in //bin/shared.sh is wrapped in the function shared::execute().
The definition of the variables PROG_NAME and BIN_DIR and the functions follow_links() and path_uri(), as these will be defined in the entrypoints, before invoking shared::execute().
At the end of shared_execute(), there is a switch-case over the BIN_NAME, or the filename of our entrypoint, which determines whether or not we invoke the dart binary directly or the flutter_tools snapshot we've created.
Creates two new entrypoint scripts, //bin/flutter and //bin/dart, which at this point have all the same code. It defines the variables and functions from step 3, and then invokes shared::execute "$@", passing it the command-line args.
Updates .cirrus.yml to trigger all steps if editing one of the bash scripts in flutter/bin.

To help code review, I uploaded a diff from master's //bin/flutter and this branch's //bin/shared.sh, which should be easier to read than the PR diff.

Note, I merged in this PR, which was already code reviewed. Thus, the changes to *.bat files should be approved already. Feel free to double check however :).

Related Issues

Part of #57063.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I signed the [CLA].
I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
I read the [Tree Hygiene] wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read [Handling breaking changes].

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: Replace with a link to your migration guide

…_windows

christopherfujino · 2020-05-15T18:48:47Z

Argh, looks like I borked CI again, lemme investigate...

bin/dart

bin/shared.sh

Co-authored-by: Jonah Williams <[email protected]>

christopherfujino · 2020-05-16T00:36:28Z

cc @gspencergoog and @jcollins-g for bash sanity check

gspencergoog

Also, try and wrap the comments at 80 columns.

bin/dart

gspencergoog · 2020-05-16T01:22:35Z

bin/dart

+
+# Convert a filesystem path to a format usable by Dart's URI parser.
+function path_uri() {
+  # Reduce multiple leading slashes to a single slash.


What about multiple embedded slashes? e.g. "/foo//bar//baz".
Also, what about things like "/foo/../bar/./baz"?

Actually, this looks like a bug in follow_links. Why don't you just fix follow_links to not emit double-leading slashes if $PWD is "/", which should be the only time it will emit a double slash.

gspencergoog · 2020-05-16T01:31:11Z

bin/flutter

@@ -35,158 +34,10 @@ function path_uri() {
  echo "$1" | sed -E -e "s,^/+,/,"


Same here: I think follow_links just needs to not emit double-slashes.

bin/dart.bat

bin/flutter.bat

bin/internal/shared.bat

bin/shared.sh

Co-authored-by: Greg Spencer <[email protected]>

jonahwilliams

LGTM

jonahwilliams · 2020-05-18T16:10:00Z

(for batch script :) )

gspencergoog · 2020-05-18T16:29:22Z

bin/shared.sh

+  # For "flock", the lock is released when the file descriptor goes out of
+  # scope,  i.e. when this function returns.  The lock is released via
+  # a trap when using "shlock".
+  if hash flock 2>/dev/null; then


What about using the mkdir method described in this FAQ, at least as a fallback when flock doesn't exist? It seems more reliable than shlock, and would probably work on platforms without either one: http://mywiki.wooledge.org/BashFAQ/045

It goes something like this:

lockdir=/tmp/myscript.lock if mkdir "$lockdir" then echo >&2 "successfully acquired lock" # Remove lockdir when the script finishes, or when it receives a signal trap 'rm -rf "$lockdir"' 0 # remove directory when script finishes # Optionally create temporary files in this directory, because # they will be removed automatically: tmpfile=$lockdir/filelist else echo >&2 "cannot acquire lock, giving up on $lockdir" exit 0 fi

@gspencergoog both of your suggestions (this one and the change to follow_links()) seem worth doing. However, is it all right if I make those changes in a follow-up PR so that if it breaks the world, we still have these scripts factored out into multiple entrypoints.

As far as I can tell, the problems you've pointed out won't be exacerbated by this change, right?

Sure, that's fine. We should address them, though.

Sure, that's fine. We should address them, though.

pinky swear :)

Filed #57512 to track.

There are some poorly implemented userland filesystems where the mkdir trick doesn't work, but those are edge cases on an edge case. It's a pretty solid backup for flock.

There are some poorly implemented userland filesystems where the mkdir trick doesn't work, but those are edge cases on an edge case. It's a pretty solid backup for flock.

sigh, it seems like BASH scripting is all about choosing which edge cases you want to optimize for...

There's another case that is an edge case and a problem: If I'm mounting the flutter dir over a file share, and I access it from both a Linux system and a Mac system simultaneously, then the locking won't hold, since one is using flock and one is using shlock (or mkdir). They'll happily overwrite each other then.

This is true. Using the same repo from multiple host OSes is not supported anyway (the cached artifacts can't be shared), but we should detect this.

BTW, I put up a draft PR to try and address some of this (although it doesn't address the two-hosts problem): #57590

christopherfujino · 2020-05-18T17:58:55Z

Also, try and wrap the comments at 80 columns.

Done

gspencergoog

jonahwilliams added 4 commits May 10, 2020 10:40

[flutter] and Dart to path

54f4bdb

move shared to internal

c574745

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

2fc7cb8

…_windows

address comments and update with latest changes

83eca82

googlebot added the cla: yes label May 14, 2020

christopherfujino requested review from jonahwilliams and removed request for jonahwilliams May 15, 2020 18:35

add two new entrypoints to bash script

db8096d

christopherfujino force-pushed the add-dart-bash-wrapper branch from 95737fb to db8096d Compare May 15, 2020 18:41

christopherfujino requested review from jonahwilliams and removed request for jonahwilliams May 15, 2020 18:47

debugging, remove every pre-submit test but analyze

f19f8d9

christopherfujino force-pushed the add-dart-bash-wrapper branch from c9835be to afd26c4 Compare May 15, 2020 19:58

add further debugging

5fa4c52

christopherfujino force-pushed the add-dart-bash-wrapper branch from afd26c4 to 5fa4c52 Compare May 15, 2020 20:15

christopherfujino added 5 commits May 15, 2020 13:28

fix

f7b6b92

remove debugging

952cf07

re-enable all cirrus tasks

227de9f

run all CI when changing bash

a0bc719

Merge branch 'pr/56845' into add-dart-bash-wrapper

859746e

jonahwilliams reviewed May 16, 2020

View reviewed changes

bin/dart Outdated Show resolved Hide resolved

jonahwilliams reviewed May 16, 2020

View reviewed changes

bin/shared.sh Outdated Show resolved Hide resolved

christopherfujino and others added 2 commits May 15, 2020 17:24

Update bin/dart

53349e9

Co-authored-by: Jonah Williams <[email protected]>

Update bin/shared.sh

9081e6c

Co-authored-by: Jonah Williams <[email protected]>

christopherfujino requested review from gspencergoog and jcollins-g May 16, 2020 00:36

gspencergoog reviewed May 16, 2020

View reviewed changes

comment typo fix

11f6d95

Co-authored-by: Greg Spencer <[email protected]>

christopherfujino and others added 4 commits May 16, 2020 19:40

comment typo fix

f8a155b

Co-authored-by: Greg Spencer <[email protected]>

comment typo fix

ee2ce80

Co-authored-by: Greg Spencer <[email protected]>

comment typo fix

b00316d

Co-authored-by: Greg Spencer <[email protected]>

comment typo fix

ebd239f

Co-authored-by: Greg Spencer <[email protected]>

jonahwilliams approved these changes May 18, 2020

View reviewed changes

gspencergoog reviewed May 18, 2020

View reviewed changes

wrap comments

cc814f0

christopherfujino mentioned this pull request May 18, 2020

[flutter_tools] Improve bash entrypoints #57512

Closed

gspencergoog approved these changes May 18, 2020

View reviewed changes

christopherfujino added waiting for tree to go green and removed waiting for tree to go green labels May 18, 2020

christopherfujino merged commit 06cd79d into flutter:master May 18, 2020

christopherfujino deleted the add-dart-bash-wrapper branch May 18, 2020 19:45

christopherfujino mentioned this pull request May 18, 2020

[flutter_tools] warn if flutter/bin/cache/dart-sdk/bin/dart appears in path #57518

Closed

jonahwilliams mentioned this pull request Jun 3, 2020

[flutter_tools] expose dartdev/Dart SDK though the flutter SDK #57063

Closed

github-actions bot locked as resolved and limited conversation to collaborators Jul 30, 2021

		@@ -35,158 +34,10 @@ function path_uri() {
		echo "$1" \| sed -E -e "s,^/+,/,"

Conversation

christopherfujino commented May 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Checklist

Breaking Change

Uh oh!

christopherfujino commented May 15, 2020

Uh oh!

Uh oh!

Uh oh!

christopherfujino commented May 16, 2020

Uh oh!

gspencergoog left a comment

Choose a reason for hiding this comment

Uh oh!

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!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented May 18, 2020

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christopherfujino commented May 18, 2020

Uh oh!

gspencergoog left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

christopherfujino commented May 14, 2020 •

edited

Loading