Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[macOS] Set correct Platform.executable (argv[0]) #33128

Merged
fluttergithubbot merged 3 commits into from
May 8, 2022
Merged

[macOS] Set correct Platform.executable (argv[0]) #33128

fluttergithubbot merged 3 commits into from
May 8, 2022

Conversation

@cbracken
Copy link
Member

@cbracken cbracken commented May 5, 2022

Previously, in the macOS embedder, we set the first argument of
FlutterProjectArgs::command_line_argv to the hardcoded string
"placeholder". As per the documentation of embedder.h, this argument is
required to be the executable name, if present. This value is available
to Flutter applications via Platform.executable in dart:io.

This corrects the value to be the executable name for the current
process. In the case where the correct name cannot be determined, we
fall back to "Flutter" in order to avoid violating the (non-nullable)
type annoation on Platform.executable.

Since the string pointers in command_line_argv are required to
be valid for the lifetime of the Dart runtime, and since the name of the
executable associated with this process will not change over the
lifetime of the app, we hold this in a static.

This also adds the internal method [FlutterEngine executableName] in
order to facilitate testing.

See: https://api.flutter.dev/flutter/dart-io/Platform/executable.html
See: dart-lang/sdk#48427

Issue: flutter/flutter#83921

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@cbracken cbracken added the Work in progress (WIP) Not ready (yet) for review! label May 5, 2022
@flutter-dashboard flutter-dashboard bot added embedder Related to the embedder API platform-macos labels May 5, 2022
@cbracken cbracken mentioned this pull request May 5, 2022
Merged
8 tasks
@cbracken cbracken removed the Work in progress (WIP) Not ready (yet) for review! label May 5, 2022
cbracken
cbracken commented May 5, 2022
View reviewed changes
@stuartmorgan-g
Copy link
Contributor

stuartmorgan-g commented May 5, 2022

Previously, in the macOS embedder, we set the first argument of
FlutterProjectArgs::command_line_argv to the hardcoded string
"placeholder". As per the documentation of embedder.h, this argument is
required to be the executable name, if present. This value is available
to Flutter applications via Platform.executable in dart:io.

Oops, I always thought it was just dropped. In my defense, I don't think it was documented at all when I started using it :)

stuartmorgan-g
stuartmorgan-g suggested changes May 5, 2022
View reviewed changes
@cbracken
Copy link
Member Author

cbracken commented May 6, 2022

Cleanups pushed. PTAL.

@cbracken
Copy link
Member Author

cbracken commented May 6, 2022 •
edited
Loading

It seems like arguments is a closer match to the engine API we're passing it to, and it would be a bit easier to use (not having to do the double-call thing).

Agreed and done.

For any future archaeologists who dig this up, it's worth noting that the path returned by _NSGetExecutablePath and the first argument in [[NSProcessInfo processInfo] arguments] do actually differ slightly:

  • _NSGetExecutablePath returns the unresolved path exactly as you'd find in argv[0] (e.g. invocations like ../../MyApp.app/Contents/MacOS/MyApp will produce an executable path of /path/to/current_dir/../../MyApp.app/Contents/MacOS/MyApp
  • The value from NSProcessInfo appears to be the resolved path /path/MyApp.app/Contents/MacOS/MyApp.

I believe this is is still acceptable because it neither violates the Platform.executable docs, nor the embedder API docs with regards to the command-line arguments.

@cbracken cbracken mentioned this pull request May 6, 2022
Closed
loic-sharma
loic-sharma approved these changes May 6, 2022
View reviewed changes
Copy link
Member

@loic-sharma loic-sharma left a comment

Choose a reason for hiding this comment

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

LGTM

stuartmorgan-g
stuartmorgan-g approved these changes May 7, 2022
View reviewed changes
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

LGTM

cbracken added 3 commits May 7, 2022 16:46
@cbracken
[macOS] Set correct Platform.executable (argv[0])
e49ed0f 
Previously, in the macOS embedder, we set the first argument of
FlutterProjectArgs::command_line_argv to the hardcoded string
"placeholder". As per the documentation of embedder.h, this argument is
required to be the executable name, if present. This value is available
to Flutter applications via Platform.executable in dart:io.

This corrects the value to be the executable name for the current
process. In the case where the correct name cannot be determined, we
fall back to "Flutter" in order to avoid violating the (non-nullable)
type annoation on Platform.executable.

Since the string pointers in command_line_argv are required to
be valid for the lifetime of the Dart runtime, and since the name of the
executable associated with this process will not change over the
lifetime of the app, we hold this in a static.

This also adds the internal method [FlutterEngine executableName] in
order to facilitate testing.

See: https://api.flutter.dev/flutter/dart-io/Platform/executable.html
See: dart-lang/sdk#48427

Issue: flutter/flutter#83921
@cbracken
Review fixups
783accd
@cbracken
One-liner-ise
06d2235
@cbracken cbracken added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label May 8, 2022
@fluttergithubbot fluttergithubbot merged commit 2d1f34c into flutter:main May 8, 2022
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 8, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 8, 2022
@engine-flutter-autoroll
2d1f34c [macOS] Set correct Platform.executable (argv[0]) (flutter/en…
db04030 
…gine#33128)
@cbracken cbracken deleted the platform-executable-macos branch May 9, 2022 16:52
@cbracken cbracken mentioned this pull request May 9, 2022
Merged
8 tasks
cbracken added a commit that referenced this pull request May 10, 2022
@cbracken
Assert Platform.executable non-empty in test (#33221)
25b190f 
When testing executable name for macOS Flutter apps, verify that it's
not the empty string. This was review feedback on
#33128 which was labelled 'waiting
for tree to go green' but it was merged by the bot before I had a chance
to push the update.

Issue: flutter/flutter#83921
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@chinmaygarde chinmaygarde chinmaygarde approved these changes

@stuartmorgan-g stuartmorgan-g stuartmorgan-g approved these changes

@loic-sharma loic-sharma loic-sharma approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

embedder Related to the embedder API platform-macos waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@cbracken @stuartmorgan-g @chinmaygarde @loic-sharma @fluttergithubbot