Skip to content

Allow --machine/--allow-embedding to be passed to devtools_tool serve #6670

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.

Sign up for GitHub

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

Jump to bottom
Merged
DanTup merged 2 commits into from
Nov 8, 2023
Merged

Allow --machine/--allow-embedding to be passed to devtools_tool serve #6670

DanTup merged 2 commits into from
Nov 8, 2023

Conversation

@DanTup
Copy link
Contributor

@DanTup DanTup commented Nov 7, 2023

The arg parser allows additional arguments, but not additional "options", so we need to add this explicitly* to allow --machine to work. The code already passes the extra args through to the server correctly, it just wasn't allowing this option to be provided on the command line.

* Or use the AllowAnythingParser, but looks like that would affect the existing options.

@DanTup DanTup requested a review from CoderDake as a code owner November 7, 2023 15:12
@DanTup DanTup marked this pull request as draft November 7, 2023 15:17
@DanTup
Copy link
Contributor Author

DanTup commented Nov 7, 2023

(converted to draft... there are other flags we pass in VS Code like --try-ports we also need to support)

@DanTup
Copy link
Contributor Author

DanTup commented Nov 7, 2023

Actually, I'm ditching --try-ports from VS Code since we're passing 10 which is the default value, but we did need allow-embedding.

@DanTup DanTup marked this pull request as ready for review November 7, 2023 15:24
@DanTup DanTup changed the title Allow --machine to be passed to devtools_tool serve Allow --machine/--allow-embedding to be passed to devtools_tool serve Nov 7, 2023
@DanTup DanTup force-pushed the allow-serve-machine branch from de7f4c5 to eb66845 Compare November 8, 2023 17:05
@DanTup
Copy link
Contributor Author

DanTup commented Nov 8, 2023

(this is ready for review now and hopefully the bots will be green 🤞)

kenzieschmoll
kenzieschmoll approved these changes Nov 8, 2023
View reviewed changes
Copy link
Member

@kenzieschmoll kenzieschmoll left a comment

Choose a reason for hiding this comment

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

I'm surprised that these weren't passed through automatically since we pass any unhandled arguments along to the dds command. but lgtm

@DanTup
Copy link
Contributor Author

DanTup commented Nov 8, 2023

I'm surprised that these weren't passed through automatically since we pass any unhandled arguments along to the dds command

I was too.. seems to be a difference between arguments and the named options. There's an AllowAnythingParser that would allow these, but I think we'd then have to also read the other options ourselves, so since we only need these two I figured this was better. We can always revisit if we want to pass more in future.

@DanTup DanTup merged commit 7cdf89a into flutter:master Nov 8, 2023
@DanTup DanTup deleted the allow-serve-machine branch November 8, 2023 22:03
@DanTup DanTup mentioned this pull request Feb 14, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kenzieschmoll kenzieschmoll kenzieschmoll approved these changes

@CoderDake CoderDake Awaiting requested review from CoderDake

Assignees

No one assigned

Labels

release-notes-not-required

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@DanTup @kenzieschmoll