Skip to content

[SDK] add Dart to path#56845

Closed
jonahwilliams wants to merge 4 commits intoflutter:masterfrom
jonahwilliams:dart_on_path_windows
Closed

[SDK] add Dart to path#56845
jonahwilliams wants to merge 4 commits intoflutter:masterfrom
jonahwilliams:dart_on_path_windows

Conversation

@jonahwilliams
Copy link
Contributor

@jonahwilliams jonahwilliams commented May 10, 2020

Description

The windows implementation of placing a dart executable on the PATH. This moves all of the shared logic into internal/shared.bat and adds a CALL to invoke this from both flutter.bat and dart.bat.

To keep the code simple, the bootstrap process is identical whether calling into dart or flutter.

@jonahwilliams
Copy link
Contributor Author

This wouldn't be landed until we're ready with the bash implementation too

@jonahwilliams jonahwilliams marked this pull request as ready for review May 11, 2020 17:42
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

Looks fine.

bin/dart.bat Outdated
SET shared_bin=%FLUTTER_ROOT%/bin/internal/shared.bat
CALL "%shared_bin%"

SET flutter_tools_dir=%FLUTTER_ROOT%\packages\flutter_tools
Copy link
Member

Choose a reason for hiding this comment

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

nit: some of the variables set here seem unused? e.g. snapshot_path and engine_stamp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

bin/dart.bat Outdated
REM ---------------------------------- NOTE ----------------------------------
REM
REM Please keep the logic in this file consistent with the logic in the
REM `flutter` script in the same directory to ensure that Flutter continues to
Copy link
Member

Choose a reason for hiding this comment

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

Does this comment need updating (i.e. is the linux side also getting restructured?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

REM ---------------------------------- NOTE ----------------------------------
REM
REM Please keep the logic in this file consistent with the logic in the
REM `flutter` script in the same directory to ensure that Flutter continues to
Copy link
Member

Choose a reason for hiding this comment

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

this may need updating (see other comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@goderbauer goderbauer added the tool Affects the "flutter" command-line tool. See also t: labels. label May 12, 2020
@jonahwilliams
Copy link
Contributor Author

Updated to remove unused variables, and reference update pwsh location and disable-dartdev for flutter.

@jonahwilliams jonahwilliams requested a review from goderbauer May 14, 2020 22:12
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

@jonahwilliams
Copy link
Contributor Author

This was pulled into @christopherfujino 's change to land in one go

@jonahwilliams jonahwilliams deleted the dart_on_path_windows branch May 18, 2020 17:47
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants