-
Notifications
You must be signed in to change notification settings - Fork 395
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
chore: Bundle WebDriverAgent-runner.app with package #331
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -126,7 +126,7 @@ async function checkForDependencies (opts = {}) { | |||
} | |||
|
|||
async function bundleWDASim (xcodebuild, opts = {}) { | |||
if (!_.isFunction(xcodebuild?.retrieveDerivedDataPath)) { | |||
if (xcodebuild && !_.isFunction(xcodebuild.retrieveDerivedDataPath)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nits]
Perhaps once you up to date npm modules, gulp can compile ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the gulp plugins module must be at 5.2.0 or higher
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably worth updating that to make this line more readable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The scripts run from 'node' directly (for performance sake) so no transpiling. Do you mind if I just leave this as is (I do like this syntax though).
package.json
Outdated
@@ -88,6 +88,7 @@ | |||
"WebDriverAgentLib", | |||
"WebDriverAgentRunner", | |||
"WebDriverAgentTests", | |||
"XCTWebDriverAgentLib" | |||
"XCTWebDriverAgentLib", | |||
"WebDriverAgentRunner-Runner.app" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How the package size will be?
I think it's better to make .app
to tar
to pack this module when we push this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, we don't want hundreds of files being downloaded. Probably need a mechanism to pack/unpack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(zip might make sense since we have code for all that already)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. I made it shell out on zip
from the terminal. It will only work in MacOS but that's fine because we need to be in a MacOS environment to produce the .app
One more change. I made it clear the contents of DerivedData (just WebDriverAgent-*) so that there's only one there and it's not using an old .app |
@mykola-mokhnach pushed fixes. |
'Xcode', 'DerivedData'); | ||
log.info(`Clearing contents of '${derivedDataPath}/WebDriverAgent-*'`); | ||
for (const wdaPath of | ||
await fs.glob('WebDriverAgent-*', {cwd: derivedDataPath, absolute: true}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
await fs.glob('WebDriverAgent-*', {cwd: derivedDataPath, absolute: true}) | ||
) { | ||
log.info(`Deleting existing WDA: '${wdaPath}'`); | ||
await fs.rimraf(wdaPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could optimise the cleanup by wrapping all these promises with B.all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want that to be in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a simple change, but it's up to you to decide
], {cwd: rootDir}); | ||
|
||
// Moved DerivedData/WebDriverAgent-* from Library to uncompressed folder | ||
const derivedDataPath = path.resolve(os.homedir(), 'Library', 'Developer', 'Xcode', 'DerivedData'); | ||
// Move DerivedData/WebDriverAgent-* from Library to "uncompressed" folder | ||
const wdaPath = (await fs.glob(`${derivedDataPath}/WebDriverAgent-*`))[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the same approch with cwd
and absolute
here. Also, it makes sense to verify the resulting array has at least one item
// Compress the "uncompressed" bundle as a Zip | ||
const pathToZip = path.resolve(pathToBundles, `webdriveragent-xcode_${xcodeVersion}.zip`); | ||
await zip.toArchive( | ||
pathToZip, {cwd: path.join(rootDir, 'uncompressed')} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cwd: uncompressedDir
I think these variables could have more descriptive names to make the code more readable
'Build', 'Products', 'Debug-iphonesimulator', wdaAppBundle); | ||
const appBundleZipPath = path.join(rootDir, `${wdaAppBundle}.zip`); | ||
await fs.rimraf(appBundleZipPath); | ||
log.info(`Created './${wdaAppBundle}.zip'`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
also a suggestion for the code, that does the extraction operation: Use zip.readEntries helper with onEntry callback. This allows to only extract only the necessary entry(ies) without extracting the whole archive, which makes the deployment process faster |
@dpgraham can we also archive the runner to .ipa for real devices? |
As Appium? Then, no. |
@KazuCocoa yeah, but can I for example build the ipa (with my enterprise signing configs) for the first time, and reuse it on other machines? |
ah, you asked more general questions. https://github.com/appium/ruby_lib_core/blob/cf546a0c62f815df4bb3ece4932b8033fad25ae2/test/test_helper.rb#L188-L212 also may help to use |
okay thanks Kazu, I will dig into those stuffs 👍 |
This bundles the WebDriverAgent-runner.app with the package
Justification
Using IDB, you can easily install and launch WebDriverAgent-runner.app with a couple of commands.
This speeds up the installation significantly and is a huge boost for cloud providers (like myself). We won't have to build a different WDA for every single Xcode version and we'll get much faster session start times.
My next step for this is to have our CI pipeline automate the publishing of this module so that the Xcode that's used to bundle the .app is consistent.