should be able to clone from a profile as a basis

RESOLVED FIXED

Status

Testing
Mozbase
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: Jeff Hammel, Assigned: mihneadb)

Tracking

({dataloss})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mozmill-2.0-][mozmill-next?][mozbase][good first bug][mentor=jhammel])

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

7 years ago
Currently, if you're using a pre-specified profile, Mozmill operates
directly on that profile.  As a side-effect, you can't guarantee that
the profile you are operating on has not been changed by the tests.
Usually, in a practical use case, this is a bad thing, since you can't
really reset the profile to the state that it was before.  

Instead of doing this, Mozmill (Mozprofile actually) could clone from
a basis profile each time it needs to reset.
(Reporter)

Comment 1

7 years ago
If this is +ed for 2.0, I may take this as part of bug 641956 as I will need to improve profile handling there anyway
Severity: normal → enhancement
Whiteboard: [mozmill-2.0?]
(Reporter)

Comment 2

7 years ago
It may also be worthwhile when this bug is taken or subsequently to introduce a command-line flag that allows the profile to accumulate state.  I know thunderbird wants this (bug 579929).
(Reporter)

Updated

7 years ago
Assignee: nobody → jhammel
I wouldn't mark it as enhancement. Anyone who is using Mozmill on an existing profile and runs any of our tests will swipe away all the profile data. This is a dataloss situation, which we do not really prevent at the moment.

I talked with Jeff about that earlier this week and cloning an existing profile seems to be the best way to fix this problem.
Severity: enhancement → critical
Keywords: dataloss
(Reporter)

Comment 4

7 years ago
(In reply to comment #3)
> I wouldn't mark it as enhancement. Anyone who is using Mozmill on an existing
> profile and runs any of our tests will swipe away all the profile data. This is
> a dataloss situation, which we do not really prevent at the moment.

I'm not sure I understand or agree with this assessment.  Currently, reseting the profile for a profile specified by --profile (in MozProfile or in MozRunner in legacy versions) removes the preferences and addons set by the runner (though not those specified by the tests).  So, if the tests do not change anything (I realize this is unlikely) then the profile will be as it was before the run.  Of course this only applies to specified profiles -- transient profiles (if no --profile is specified) will be wiped, though there is a bug about whether this is correct behaviour or not.

I also disagree that this is a bug.  This is, for better or worse, designed behaviour, and whether it is a bug or not depends on your point of view.  From the point of view of MozMill + MozProfile + MozRunner as a test harness associated with hg.m.o/qa/mozmill-tests, it is a good idea to clone from a profile such that it is not corrupted.  However, the slated target for MozMill is a general automation tool.  You may *want* to modify the profile in place!  You may want to use MozMill to generate a profile that may be used externally.  For instance, fuzzing and profile testing.  I would hate to throw this use-case out.

To summarize:
- MozProfile should have the ability to clone the profile; in fact, MozProfile should have much more functionality than it currently has
- MozMill should have a way to really reset the profile via cloning; perhaps/probably this should be the default
- but we shouldn't throw away the ability to have MozMill (etc) actually being able to add/delete/etc to a profile as this is useful to

Updated

7 years ago
Whiteboard: [mozmill-2.0?] → [mozmill-2.0+]

Comment 5

7 years ago
Not really dataloss.  any test tool will munge the profile.  That's why we tell people not to run test tools on their profiles.
Severity: critical → normal
Whiteboard: [mozmill-2.0+] → [mozmill-next?]
Whiteboard: [mozmill-next?] → [mozmill-2.0-][mozmill-next?]
Blocks: 568943
(Reporter)

Updated

7 years ago
Assignee: jhammel → nobody
Whiteboard: [mozmill-2.0-][mozmill-next?] → [mozmill-2.0-][mozmill-next?][mozbase]
(Reporter)

Updated

7 years ago
Whiteboard: [mozmill-2.0-][mozmill-next?][mozbase] → [mozmill-2.0-][mozmill-next?][mozbase][good-first-bug]
TPS has need of this functionality as well.  I may volunteer to implement this soon if it isn't picked up by someone else.
(Reporter)

Updated

7 years ago
Component: Mozmill → Mozbase
QA Contact: mozmill → mozbase
Whiteboard: [mozmill-2.0-][mozmill-next?][mozbase][good-first-bug] → [mozmill-2.0-][mozmill-next?][mozbase][good first bug][mentor=jhammel]
(Assignee)

Comment 7

5 years ago
(In reply to Jeff Hammel [:jhammel] from comment #4)
> To summarize:
> - MozProfile should have the ability to clone the profile; in fact,
> MozProfile should have much more functionality than it currently has

By cloning a profile you mean just copying the whole directory?

> - MozMill should have a way to really reset the profile via cloning;
> perhaps/probably this should be the default
> - but we shouldn't throw away the ability to have MozMill (etc) actually
> being able to add/delete/etc to a profile as this is useful to

Maybe I'm missing something but from what I see, MozMill interacts with the profile via mozrunner, is that correct? Should the reset via cloning functionality be implemented in mozrunner (since MozMill already seems to call this here [1]) ?

[1] https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/__init__.py#L302
(Reporter)

Comment 8

5 years ago
(In reply to Mihnea Dobrescu-Balaur (:mihneadb) from comment #7)
> (In reply to Jeff Hammel [:jhammel] from comment #4)
> > To summarize:
> > - MozProfile should have the ability to clone the profile; in fact,
> > MozProfile should have much more functionality than it currently has
> 
> By cloning a profile you mean just copying the whole directory?

There may be nuances encountered that aren't obvious but yes, a recursive copy should be sufficient.  I was thinking something like:

@classmethod
def clone(cls, path, **kwargs):
    """instantiate a mozprofile instance via cloning
    - path : path of the basis to clone
    - kwargs: arguments to the profile ctor
    """
    # NOTE: this is a cartoon
    tempdir = tempfile.mkdtemp()
    shutil.copytree(path, tempdir)
    return cls(tempdir, **kwargs)

I don't know if there are reasons not to do it this way, but this LGTM.  I'm open to architectural suggestions here.

> > - MozMill should have a way to really reset the profile via cloning;
> > perhaps/probably this should be the default
> > - but we shouldn't throw away the ability to have MozMill (etc) actually
> > being able to add/delete/etc to a profile as this is useful to
> 
> Maybe I'm missing something but from what I see, MozMill interacts with the
> profile via mozrunner, is that correct? Should the reset via cloning
> functionality be implemented in mozrunner (since MozMill already seems to
> call this here [1]) ?
> 
> [1]
> https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/__init__.
> py#L302

This bug was filed a long time ago when mozprofile was part of mozrunner and both lived in the mozmill repository.  Currently, mozmill in production is on 1.5 and is uninterested in upgrading to mozbase software until 2.0 which has no ETA. If we end up using this in mozmill and other harnesses, mozrunner will (probably) need a front end to this functionality.
(Assignee)

Comment 9

5 years ago
(In reply to Jeff Hammel [:jhammel] from comment #8)
> (In reply to Mihnea Dobrescu-Balaur (:mihneadb) from comment #7)
> > (In reply to Jeff Hammel [:jhammel] from comment #4)
> > > To summarize:
> > > - MozProfile should have the ability to clone the profile; in fact,
> > > MozProfile should have much more functionality than it currently has
> > 
> > By cloning a profile you mean just copying the whole directory?
> 
> There may be nuances encountered that aren't obvious but yes, a recursive
> copy should be sufficient.  I was thinking something like:
> 
> @classmethod
> def clone(cls, path, **kwargs):
>     """instantiate a mozprofile instance via cloning
>     - path : path of the basis to clone
>     - kwargs: arguments to the profile ctor
>     """
>     # NOTE: this is a cartoon
>     tempdir = tempfile.mkdtemp()
>     shutil.copytree(path, tempdir)
>     return cls(tempdir, **kwargs)
> 
> I don't know if there are reasons not to do it this way, but this LGTM.  I'm
> open to architectural suggestions here.

That's exactly what I was thinking, plus some checks and cleanup after the job is done. I guess I'll come up with a patch and if you have any other suggestions I can add to that.

> 
> > > - MozMill should have a way to really reset the profile via cloning;
> > > perhaps/probably this should be the default
> > > - but we shouldn't throw away the ability to have MozMill (etc) actually
> > > being able to add/delete/etc to a profile as this is useful to
> > 
> > Maybe I'm missing something but from what I see, MozMill interacts with the
> > profile via mozrunner, is that correct? Should the reset via cloning
> > functionality be implemented in mozrunner (since MozMill already seems to
> > call this here [1]) ?
> > 
> > [1]
> > https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/__init__.
> > py#L302
> 
> This bug was filed a long time ago when mozprofile was part of mozrunner and
> both lived in the mozmill repository.  Currently, mozmill in production is
> on 1.5 and is uninterested in upgrading to mozbase software until 2.0 which
> has no ETA. If we end up using this in mozmill and other harnesses,
> mozrunner will (probably) need a front end to this functionality.

OK, I'll look into the easiest way to integrate this into mozrunner.
(Assignee)

Comment 10

5 years ago
Created attachment 711268 [details] [diff] [review]
implement the clone operation

I guess deleting the cloned profile on cleanup() makes the most sense. Let me know if you think otherwise.
Assignee: nobody → mihneadb
Attachment #711268 - Flags: review?(jhammel)
(Reporter)

Comment 11

5 years ago
Comment on attachment 711268 [details] [diff] [review]
implement the clone operation

Sorry its taken a bit to get to this.

I've thought about this a bit and instead of just the path to clone from you will probably also want the path to clone to, which will be None by default (identical to the Profile.__init__'s argument).  You may want to clone to a particular path, in which case you specify it.  Or you may not and leave it as None, and keep the  tempfile logic as you have.

In general, I don't like instances to have knowledge of the factory that created them.  In some cases, it is unavoidable, but I don't think think think this is one of them.

Currently to persist changes to the filesystem past cleanup(), as in the command line case, `restore` is set to False in the ctor. I think this should be identical for clones:  if restore=False, just keep it around.  If restore=True, delete the clone.  In any case, I don't think that having cloned as an argument to the ctor is a good idea.

As far as how to achieve this, I would probably decorate the cleanup (and probably __del__ method on the instantiated object before returning it from `clone()`.  There are alternatives....if you wanted to touch a special _im_a_clone attribute and have cleanup() check for this, I think that's a lot better that passing an argument as its not exposed API.

Anyway, this is mostly a good attempt.  I feel like this is mostly my fault for leading you down the path, but I didn't think of these issues until now.  In any case, despite my r-, excellent work and thanks.
Attachment #711268 - Flags: review?(jhammel) → review-
(Assignee)

Comment 12

5 years ago
Created attachment 712892 [details] [diff] [review]
implement the clone operation

Ok then, so let's just have the clone classmethod that takes in both path arguments and optionally you can just pass in restore=True/False as a kwarg.
Attachment #711268 - Attachment is obsolete: true
Attachment #712892 - Flags: review?(jhammel)
(Reporter)

Comment 13

5 years ago
Comment on attachment 712892 [details] [diff] [review]
implement the clone operation

This is mostly good but now we're not cleaning up the profile if restore=False.  We should do this.  Again, I'd recommend a decorator.  r- for this, though I'm happy to help out with this 

+            rmtree(tempdir) # copytree requires that dest does not exist

:sigh: one of my least favorite python bugs :/  The worst part is it is one line of code that can be changed in stdlib
Attachment #712892 - Flags: review?(jhammel) → review-
(Assignee)

Comment 14

5 years ago
(In reply to Jeff Hammel [:jhammel] from comment #13)
> Comment on attachment 712892 [details] [diff] [review]
> implement the clone operation
> 
> This is mostly good but now we're not cleaning up the profile if
> restore=False.  We should do this.  Again, I'd recommend a decorator.  r-
> for this, though I'm happy to help out with this

Oh, so you mean even if restore=False, if this is a clone, remove the profile? Ok, will do!

> 
> +            rmtree(tempdir) # copytree requires that dest does not exist
> 
> :sigh: one of my least favorite python bugs :/  The worst part is it is one
> line of code that can be changed in stdlib

I don't like it either. :(
(Assignee)

Comment 15

5 years ago
Created attachment 713897 [details] [diff] [review]
also remove clones when restore is false

Is this what you meant? I need the MethodType thing if I want the method to stay bound to the object.

If restore is True, then the cloned profile remains on the disk. Do we want that?
Attachment #712892 - Attachment is obsolete: true
Attachment #713897 - Flags: feedback?(jhammel)
(Reporter)

Comment 16

5 years ago
(In reply to Mihnea Dobrescu-Balaur (:mihneadb) from comment #14)
> (In reply to Jeff Hammel [:jhammel] from comment #13)
> > Comment on attachment 712892 [details] [diff] [review]
> > implement the clone operation
> > 
> > This is mostly good but now we're not cleaning up the profile if
> > restore=False.  We should do this.  Again, I'd recommend a decorator.  r-
> > for this, though I'm happy to help out with this
> 
> Oh, so you mean even if restore=False, if this is a clone, remove the
> profile? Ok, will do!

Yeah, sorry if this wasn't clear from comment 11 . To re-explain, and hopefully not re-confuse you ;), my line of thought was in existing `restore=False`/`restore=True` cases, the former means "I want all the changes kept for the profile" and the latter means "I want to revert the profile to the state it was before Profile was instantiated". I found this line of thought directly extensible to clone().  For the `restore=False`, its obvious what to do: the clone stays around (with whatever changes mozprofile, etc makes to it).  For the case where `restore=True`, before the clone, the profile didn't exist.  So to restore to its previous state to me says to remove the clone, it was ephemeral.  Otherwise clone(..., restore=True) gives you a copy of the profile as it was before the clone started, which doesn't make any sense, really (if you wanted to do that, you could copy the directory yourself).  Make sense?
(Assignee)

Comment 17

5 years ago
Created attachment 715107 [details] [diff] [review]
implement the clone functionality

After confusing, de-confusing and re-confusing myself, I think I got it! :)

Btw, wouldn't it be easier to use the create_new attr instead of performing magic on the methods?
Attachment #713897 - Attachment is obsolete: true
Attachment #713897 - Flags: feedback?(jhammel)
Attachment #715107 - Flags: feedback?(jhammel)
(Reporter)

Comment 18

5 years ago
Comment on attachment 715107 [details] [diff] [review]
implement the clone functionality

A few nits:

+            """Deletes a cloned profile even if restore == False"""

So that's wrong ;)  Please fix the docstring.

+                if self.restore:
+                    if os.path.exists(self.profile):

this should be a one line 

+        c.cleanup = types.MethodType(cleanup_clone(cls.cleanup), c)
+        c.__del__ = types.MethodType(cleanup_clone(cls.__del__), c)

shouldn't this be c.__del__ = c.cleanup = ... ?

So now I'm confusing myself wrt this bug ;)  My apologies that I haven't been clearer about things and thanks for bearing through the confusion.  So I notice that you don't call the function that you pass the decorator (Profile.cleanup, the original).  My intention was that the decorated function would call the original function and so do whatever needed to be done normally on cleanup and then rmtree the path.  Does that make sense?

So I'll give this an f+ given understanding of that.  The code looks good, I just evidently can't communicate what I want :/
Attachment #715107 - Flags: feedback?(jhammel) → feedback+
(Assignee)

Comment 19

5 years ago
Created attachment 715202 [details] [diff] [review]
implement the clone functionality

My bad, I forgot to call the wrapped method.

Sorry for this taking so long, I think it's ok now.
Attachment #715107 - Attachment is obsolete: true
Attachment #715202 - Flags: review?(jhammel)
(Reporter)

Comment 20

5 years ago
Comment on attachment 715202 [details] [diff] [review]
implement the clone functionality

Nice :) Looks about perfect, and I meant to add using types.MethodType is going beyond what's really needed (assuming that that all works out; i actually haven't done this for a long time, and as you might imagine not very often).   I'm sorry for the lengthy process my self, and self-contradictory and vague directions which, in the end, you did quite well with and despite of :)

As an aside, you could have only bound cleanup on restore=True as an argument to clone and I probably wouldn't have noticed or even if I did I may not have said anything, but this is certainly the correct way and not that as the method could conceivably be changed during the instance lifetime (there are probably bugs that exist in this regard; that said, how often will people do that or will it be a good idea?)
Attachment #715202 - Flags: review?(jhammel) → review+
(Assignee)

Comment 21

5 years ago
Well, you can't change a method without using types.MethodType if you still want it to work the same way as the original one (as a bound method). Or if you can, I did not find the way. :(

(Not sure what you mean by binding cleanup only on restore=True.)
(Reporter)

Comment 22

5 years ago
(In reply to Mihnea Dobrescu-Balaur (:mihneadb) from comment #21)
> Well, you can't change a method without using types.MethodType if you still
> want it to work the same way as the original one (as a bound method). Or if
> you can, I did not find the way. :(

So I think for an instance method that this is absolutely correct.  However, you can and I often have gotten away without binding the method, in the formal sense (since you can hang whatever functions you want on e.g. an instantiated object...well, those without rules saying you can't, anyway).

For instance, this sample code works fine: http://k0s.org/hg/config/file/efeb2cc78f30/python/bind.py

While I've used an object for this, for my convenience (I find it easier to double complex decorators this way), i've done the same with functions so long as the appropriate `self` argument is taken into account.  It is not bound to the object; it is just unbound callable hanging off the object that the first argument happens to be the method instance. Make sense? (I mean, the explanation,not necessarily python :P).  If not maybe we can figure it out on irc or email.

> (Not sure what you mean by binding cleanup only on restore=True.)

More rhetorics! ;)  So you have:

+        def cleanup_clone(fn):
+            """Deletes a cloned profile when restore is True"""
+            def wrapped(self):
+                fn(self)
+                if self.restore and os.path.exists(self.profile):
+                        rmtree(self.profile, onerror=self._cleanup_error)
+            return wrapped
+
+        c = cls(path_to, **kwargs)
+        c.__del__ = c.cleanup = types.MethodType(cleanup_clone(cls.cleanup), c)

Which, as I've said, is what I'm calling correct. Since the goal is to rm the clonedir IFF restore=True is set, you could conceivably do it like:

+        def cleanup_clone(fn):
+            """Deletes a cloned profile when restore is True"""
+            def wrapped(self):
+                fn(self)
+                if os.path.exists(self.profile):
+                    rmtree(self.profile, onerror=self._cleanup_error)
+            return wrapped
+        c = cls(path_to, **kwargs)
+        if c.restore:
+            c.__del__ = c.cleanup = types.MethodType(cleanup_clone(cls.cleanup), c)

That is, bind according to the information at ctor time.  Again, let's email/irc if I'm still not explaining this well.  I went against my own general principle of being Socratic, as it wasn't a comment on what your code did, it was a comment on the contrast of what not to do, and its only karmically viable that said attempt at communication instead resulted in more confusion.  My fault, my apologies.
(Reporter)

Comment 23

5 years ago
:mihnea, could i trick you into writing a test for this?  Can be pretty basic.  Otherwise I'll write a follow up bug
(Assignee)

Comment 24

5 years ago
Ah, ok, I see what you meant. I didn't think that's what you meant because I couldn't see the point of doing it that way :).

I'll get back with a test.
(Reporter)

Comment 26

5 years ago
(In reply to Mihnea Dobrescu-Balaur (:mihneadb) from comment #24)
> Ah, ok, I see what you meant. I didn't think that's what you meant because I
> couldn't see the point of doing it that way :).

Heh, yes.  Reading back I can see the confusion a-brewin'. Well, lesson learned on my part ;) Bugzilla == not the right forum.  I knew that but since we're not in the same timezone these days, I made a poor decision to try to use it as one.

> I'll get back with a test.

Awesome, thank you very much!  And thanks for the fix too for the *oldest outstanding mozbase bug*!  That should come with a trophy or something

Leaving the bug open for the test
(Assignee)

Comment 27

5 years ago
Created attachment 715983 [details] [diff] [review]
test this

This tests both possibilities (restore being True or False).


I'll wait for that trophy! :)
Attachment #715983 - Flags: review?(jhammel)
(Reporter)

Comment 28

5 years ago
Comment on attachment 715983 [details] [diff] [review]
test this

Mostly okay, but copy+paste ftl.  I've written bug 843286 for having a self-executing template to avoid the copy+paste reflectively magnifying developer time sink bug.  The tests are pretty basic, but better a safety net than none IMHO.

+"""
+test nonce in prefs delimeters
+see https://bugzilla.mozilla.org/show_bug.cgi?id=722804
+"""

Nope!

And you'll need a license block.

+import time

You don't use this

+from mozprofile.prefs import Preferences

You don't use this
Attachment #715983 - Flags: review?(jhammel) → review-
(Assignee)

Comment 29

5 years ago
Created attachment 716201 [details] [diff] [review]
test

Whoops!
Attachment #715983 - Attachment is obsolete: true
Attachment #716201 - Flags: review?(jhammel)
(Reporter)

Comment 30

5 years ago
Comment on attachment 716201 [details] [diff] [review]
test

lgtm, thanks!
Attachment #716201 - Flags: review?(jhammel) → review+
(Reporter)

Comment 31

5 years ago
pushed: https://github.com/mozilla/mozbase/commit/a6b8e5b001e38baaf7fb256a1ec83db5deb4db0c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Blocks: 819664
See Also: → bug 1173380
See Also: bug 1173380
You need to log in before you can comment on or make changes to this bug.