Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

PR (Issue #1176) - Stop Forcing -gpu host on android emulators #1177

Merged
merged 2 commits into from
Mar 14, 2019
Merged

PR (Issue #1176) - Stop Forcing -gpu host on android emulators #1177

merged 2 commits into from
Mar 14, 2019

Conversation

RyanThomas73
Copy link
Contributor

Resolves #1176

Only adds the -gpu [gpu mode] flags to the android emulator command if the --headless parameter is specified or if an explicit --gpu [gpu mode] cli argument is provided.

@noomorph
Copy link
Collaborator

noomorph commented Mar 5, 2019

@rotemmiz, looks good to me. Shall we merge?

@d4vidi d4vidi self-assigned this Mar 13, 2019
Copy link
Collaborator

@d4vidi d4vidi left a comment

Choose a reason for hiding this comment

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

Thanks for this PR, 'really like it!

}

return undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@RyanThomas73 for backwards compatibility, please return host as the default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@d4vidi Returning host as the default reintroduces the problem where it will always include -gpu host with the emulator command preventing the emulator command from honoring an explicit gpu mode specific in the emulator config via hw.gpu.mode=

Copy link
Contributor Author

@RyanThomas73 RyanThomas73 Mar 13, 2019

Choose a reason for hiding this comment

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

We could choose a specific value to map to undefined so it uses the emulator config default (e.g. detox test .... --gpu default) but I feel its better the way it is in the PR now. The user shouldn't have to explicitly specify default to get the default behavior from the emulator.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, you are right - I was not aware of that.
Nevertheless the issue of back-compatibility still is concerning (keeping the community happy). That being said, I take it that the crucial use cases are of the CI, where typically emulators are run headless, which isn't affected in this case. So, bottom line here - I'll approve and merge.
Thank you :)

@d4vidi d4vidi merged commit e7cbb69 into wix:master Mar 14, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Mar 17, 2019
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants