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

Add flutter SDK v1.20.1 #87344

Closed
wants to merge 3 commits into from
Closed

Add flutter SDK v1.20.1 #87344

wants to merge 3 commits into from

Conversation

jota3
Copy link
Contributor

@jota3 jota3 commented Aug 10, 2020

Important: Do not tick a checkbox if you haven’t performed its action. Honesty is indispensable for a smooth review process.

After making all changes to a cask, verify:

Additionally, if adding a new cask:

  • Named the cask according to the token reference.
  • brew cask audit --new-cask {{cask_file}} worked successfully.
  • brew cask install {{cask_file}} worked successfully.
  • brew cask uninstall {{cask_file}} worked successfully.
  • Checked the cask was not already refused.
  • Checked the cask is submitted to the correct repo.

Casks/flutter.rb Outdated
conflicts_with cask: "dart"
depends_on macos: ">= :catalina"

app "flutter"
Copy link
Member

Choose a reason for hiding this comment

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

There is no app for this, only a binary, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a full sdk so it includes a few binaries (main one being flutter), sources, examples ... so I used app to expose the whole set in the Applications folder. Do you have any better suggestion ?

Copy link
Member

Choose a reason for hiding this comment

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

suite maybe?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, suite seems correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, updated, thanks.

@miccal
Copy link
Member

miccal commented Aug 10, 2020

Does this not also require the Android SDK, Android Studio, CocoaPods and a full install of Xcode to be functional?

@jota3
Copy link
Contributor Author

jota3 commented Aug 10, 2020

Does this not also require the Android SDK, Android Studio, CocoaPods and a full install of Xcode to be functional?

@miccal They are not part of the system requirements of the SDK (see the official docs).

@miccal
Copy link
Member

miccal commented Aug 10, 2020

Does this not also require the Android SDK, Android Studio, CocoaPods and a full install of Xcode to be functional?

@miccal They are not part of the system requirements of the SDK (see the official docs).

True, but won't flutter doctor immediately report all these things missing as an issue, and hence not function without them?

@miccal
Copy link
Member

miccal commented Aug 10, 2020

Please also note that the addition of flutter to Cask has been refused in the past:

#72679
#63170
#46579

and hence it may be wise to wait until @vitorgalvao has had a chance to review and comment on this PR before making any changes.

Casks/flutter.rb Outdated
app "flutter"
binary "flutter/bin/flutter"

uninstall rmdir: [
Copy link
Member

Choose a reason for hiding this comment

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

rmdir is for empty directories. You want delete. But this can be removed, this would already be done automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, removed, thanks.

Casks/flutter.rb Outdated
"#{appdir}/flutter",
]

caveats <<~EOS
Copy link
Member

Choose a reason for hiding this comment

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

Remove this. If we had this caveat for every app that needed it, half the casks would have it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I wasn't sure but I couldn't find anything regarding this in the docs, I may have missed it though. Otherwise it might be worth a note ?

sha256 "a6364d48455bd7eb749bcbd0b7c71b985bccb0149ea0dd95c6bef3bbb2e9f91b"

# storage.googleapis.com/flutter_infra/releases/stable/macos/ was verified as official when first introduced to the cask
url "https://storage.googleapis.com/flutter_infra/releases/stable/macos/flutter_macos_#{version}-stable.zip"
Copy link
Member

Choose a reason for hiding this comment

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

How do we get here from the homepage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

flutter.dev > Get Started > macOS > Get the Flutter SDK

@vitorgalvao vitorgalvao added the awaiting user reply Issue needs response from a user. label Aug 10, 2020
@vitorgalvao
Copy link
Member

Please also note that the addition of flutter to Cask has been refused in the past

Thank you for remembering those things!

This name did ring a bell and I checked. The last time this was discussed, good arguments were made for this as a cask and I gave the thumbs up but the user ended up not submitting.

@jota3
Copy link
Contributor Author

jota3 commented Aug 11, 2020

@vitorgalvao CI is failing now. Is desc stanza only mandatory for suite type ? [Edit] Just noticed desc missing is just a warning. So I'm not sure why CI fails, it passes locally.

@miccal
Copy link
Member

miccal commented Aug 12, 2020

@jota3 the desc stanza is relatively new -- see Homebrew/brew#8137 and #87436.

Copy link
Contributor

@andreiborisov andreiborisov left a comment

Choose a reason for hiding this comment

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

Seems to work fine, would love for it to be merged😍 Let me know if I can help.

Copy link
Contributor

@andreiborisov andreiborisov left a comment

Choose a reason for hiding this comment

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

Please also symlink dart binary:

  binary "flutter/bin/dart"

depends_on macos: ">= :catalina"

suite "flutter"
binary "flutter/bin/flutter"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
binary "flutter/bin/flutter"
binary "flutter/bin/dart"
binary "flutter/bin/flutter"

conflicts_with cask: "dart"
depends_on macos: ">= :catalina"

suite "flutter"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
suite "flutter"

I don't think there is a point in installing SDK in the Applications folder. It's just a Git repo without any GUI applications. Probably should be deleted.

@andreiborisov andreiborisov mentioned this pull request Aug 16, 2020
10 tasks
@andreiborisov
Copy link
Contributor

An updated version of the PR: #87690

@laeo
Copy link

laeo commented Aug 19, 2020

Hello, any updates on this PR? When it can be merged?

@vitorgalvao
Copy link
Member

Closing in favour of #87690.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting user reply Issue needs response from a user.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants