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

Remove Rainbow, avoid race in creating directories #306

Merged
merged 1 commit into from Feb 15, 2016

Conversation

quozl
Copy link
Contributor

@quozl quozl commented Jan 20, 2016

Sugar depended on Rainbow for clearing the activity instance/ and tmp/ directories. But Rainbow is no longer used downstream.

  • remove support for Rainbow,
  • avoid race when creating directories; don't check they exist before we create.

@samdroid-apps
Copy link
Contributor

This patch is a NAK from my perspective.

As @godiard said on the mailing list, what if the user has 2 activity instances open? When you close the first instance, it will clear it's instance dir and leave it in a bad state. Please clear it at a different time to address that issue.

Otherwise it looks like a good patch.

@quozl
Copy link
Contributor Author

quozl commented Jan 21, 2016

Any reason why not say this on mailing list where the discussion is?

@quozl quozl changed the title Remove Rainbow, clear activity instance/ and tmp/ Remove Rainbow, avoid race in creating directories Feb 3, 2016
Sugar depended on Rainbow for clearing the activity instance/ and tmp/
directories.  But Rainbow is no longer used downstream.

- remove support for Rainbow,

- avoid race when creating directories; don't check they exist before we
  create.
@quozl
Copy link
Contributor Author

quozl commented Feb 15, 2016

Ping? Substantial change to patch, please review.

@samdroid-apps
Copy link
Contributor

From Gonzalo's reply to your initial mailing list post:

A few issues:
...

  • Temporary directories are shared between instances, if we remove temporary directories
    at activity start or stop, we need check if there are other instances of the activity running.

Please address this. Currently, your patch description says it will delete files from under the activity's eyes if a 2nd instance is launched.

Also, where is the code that actually removes the directories?

@quozl
Copy link
Contributor Author

quozl commented Feb 15, 2016

Currently, your patch description says it will delete files

No, it doesn't. Look at the commit carefully?

@samdroid-apps
Copy link
Contributor

Ah, I see. I just edited the initial pull request comment to reflect the new commit message.

This looks fine to me. I'll just test 'n' merge.

@samdroid-apps samdroid-apps merged commit 8d4298e into sugarlabs:master Feb 15, 2016
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.

None yet

2 participants