Skip to content
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

fix: Cleanup old web-ext run artifacts dir when we are connecting to an android device #1856

Closed
wants to merge 1 commit into from

Conversation

Vishalghyv
Copy link
Contributor

@Vishalghyv Vishalghyv commented Mar 7, 2020

fix: Added cleaning up old files In android
Calling cleanupCallbacks.
Also used
if (selectedArtifactsDir)
{log.debug('Cleaning up artifacts directory on the Android device...');
await adbUtils.clearArtifactsDir(selectedAdbDevice);}

But this caused in decrease of coverage.
So just this.cleanupCallbacks functions is called.

(Fixes #1591)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 77d5970 on Vishalghyv:master into 9735a1f on mozilla:master.

@rpl
Copy link
Member

rpl commented Mar 12, 2020

Hi @Vishalghyv, the changes in this pull requests cannot cleaning up the old files stored by a previous web-ext run command, just calling the cleanupCallbacks is not going to do that.

The approach suggested by @Rob--W in #1591 (comment) is to make web-ext run to remove the directories from the target device sdcard that starts with the web-ext-artifacts- prefix, which is what we use for generating the unique name for the temporary directory on the target device.

In the src/extension-runners/firefox-android.js module that you are changing in this PR there is already some code that is interacting with the target device over adb, take a look at that code and think about how you could achieve what Rob is suggesting.

As a side note, you don't need to close the pull requests to apply changes on them, you should create a branch (e.g. using something like git checkout -b branchname, see https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/proposing-changes-to-your-work-with-pull-requests for more docs about that), create a pull request for that branch and then keep updating that branch with more commits based on the review and feedback comments (or to fix test failures in the continuous integration jobs).

@Vishalghyv
Copy link
Contributor Author

Vishalghyv commented Mar 13, 2020

Hey @rpl , Thanks for reviewing my pull request.
Firstly, Yeah I created lot of pull request for small changes, Will certainly use the guide.
I looked through the code as suggested Just wanted to ask the way of solution.
Since.selectedArtifactsDir is where artifacts are stored
So will the below code work

if (this.selectedArtifactsDir) {
await this.adbUtils.clearArtifactsDir(this.selectedAdbDevice);
}
After calling check adbCheckRuntimePermession

If this is not the way, I was thinking of using runShellCommands and directly removing directory using the relative path but checking few conditions before it like directory and permissions

@rpl
Copy link
Member

rpl commented Mar 20, 2020

Hey @rpl , Thanks for reviewing my pull request.
Firstly, Yeah I created lot of pull request for small changes, Will certainly use the guide.
I looked through the code as suggested Just wanted to ask the way of solution.
Since.selectedArtifactsDir is where artifacts are stored
So will the below code work

if (this.selectedArtifactsDir) {
await this.adbUtils.clearArtifactsDir(this.selectedAdbDevice);
}
After calling check adbCheckRuntimePermession

If this is not the way, I was thinking of using runShellCommands and directly removing directory using the relative path but checking few conditions before it like directory and permissions

We are already trying to clear the artifacts dir on the device when web-ext is shutting down, but there are still chances that we may have not been able to remove them (e.g. because we got disconnected from the device or emulator we were connected to over adb, as also mentioned by Rob in this comment #1591 (comment)).

this.selectedArtifactsDir would not be set to the same path of a previous run, because it has a timestamp as a suffix as you can see here:

artifactsDir = `/sdcard/web-ext-artifacts-${Date.now()}`;

It would be good to add to the ADBUtils class a new method that does what I described in #1856 (comment)

runShellCommands may definitely be useful, but I would also suggest to look if the underlying npm dependency (adbkit) has any additional API method that may be useful to achieve what we need (take a look to the API doc here: https://github.com/openstf/adbkit )

@rpl rpl changed the title fix: Added deleition of cleaning of old files of android on start fix: Cleanup old web-ext run artifacts dir when we are connecting to an android device Mar 20, 2020
@rpl
Copy link
Member

rpl commented Mar 23, 2020

superseded by #1866

@rpl rpl closed this Mar 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Android development is not cleaning up old files
3 participants