484205 - Additional fixes to client.py, after bug 482686

Status: RESOLVED FIXED

(MailNews Core :: Build Config, defect)

Description

[Serge Gautherie (:sgautherie)]

See bug 482686 comment 10 about my ideas,
and bug 482686 comment 18 about KaiRo's idea.

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Bug 482686 comment 10 part

*Don't rewrite |TREE_STATE_FILE| when already up to date.
*More smoothly handle |os.rename()| potential failures.
*Split |fixup_repo_options()| into individual functions.
*Execute (new) functions only when useful.

Comment 2

[Justin Wood (:Callek)]

(In reply to comment #1)
> Created an attachment (id=368276) [details]
> (Av1) Bug 482686 comment 10 part
> 
> *More smoothly handle |os.rename()| potential failures.

What failures are you concerned about, and why should we pretend like everything is ok (by just skipping said sections) when we encounter one of these failures?

Holding off on review +/- pending this answer

Comment 3

[Justin Wood (:Callek)]

Comment on attachment 368276 [details] [diff] [review]
(Av1) Bug 482686 comment 10 part

Thought of something worth r-'ing on. If you disagree with me, comment and we can discuss.

>+def fixup_mozilla_repo_options(options):
>     # Handle special case: initial checkout of Mozilla.
>+    # See fixup_comm_repo_options().

Nit: python style function comments |"""| as above; also elsewhere.

> if action in ('checkout', 'co'):
>     if not options.skip_comm:
>+        fixup_comm_repo_options(options)
>         do_hg_pull('.', options.comm_repo, options.hg, options.comm_rev)
> 
>-    if not options.skip_mozilla:
>+    if not options.skip_mozilla and move_to_stable():
>+        # Update Comm repository configuration.
>+        repo_config()
>+        fixup_mozilla_repo_options(options)

Slight dilema here, repo_config needs to happen after move_to_stable, but in the future we may need it to also happen before fixup_comm_repo_options. and should _not_ depend on --skip-mozilla.  Can you come up with a solution to this.

Comment 4

[Serge Gautherie (:sgautherie)]

(In reply to comment #2)
> What failures are you concerned about,

"existing destination directory (name)" is the one I tested.
Also "Open/Locked files in source dir" ... and whatever others.

> and why should we pretend like
> everything is ok (by just skipping said sections) when we encounter one of
> these failures?

I'm assuming the current setup is working;
then, if parts of the update are skipped, the result is not fully up to date but "should" still work.

In short, that's similar (logic) as what the check for 'mozilla/extensions' does:
being unable to do one part does not have to prevent doing the others.


(In reply to comment #3)
> Nit: python style function comments |"""| as above; also elsewhere.

I'll do.

> Slight dilema here, repo_config needs to happen after move_to_stable, but in
> the future we may need it to also happen before fixup_comm_repo_options. and
> should _not_ depend on --skip-mozilla.  Can you come up with a solution to
> this.

That's exactly what I wondered about when I did it.
I have no solution ftb because I'm unclear as what this TREE_STATE_FILE is (or will be) all about. (I mean "in details".)
So I chose to code what is now, and revisit it later if need be.

Comment 5

[Justin Wood (:Callek)]

(In reply to comment #4)
> (In reply to comment #2)

Fair enough on your setup, how does the following (as an alternate) sound [seriously if you disagree lets discuss rather than just going with my word here].

place repo_config as the first thing we do once we know we are checking out.

do move_to_stable() at the very start of repo_config, bailing early in repo_config() if it fails (per your return False from move_to_stable)...

something like:

def repo_config():
  if !move_to_stable():
    return
  import ConfigParser
  ...

if action in ('checkout', 'co'):
  repo_config()
  if not options.skip_comm:
    ...

Comment 6

[Serge Gautherie (:sgautherie)]

Attachment: (Av1a) Bug 482686 comment 10 part

Av1, with comment 3 suggestion(s).

***

Iirc:

(In reply to comment #5)
> something like:

That would not work as is, because if move_to_stable() fails then do_hg_pull() would execute over the cvs dir :-/

Comment 7

[Justin Wood (:Callek)]

Comment on attachment 368427 [details] [diff] [review]
(Av1a) Bug 482686 comment 10 part

Came up with 1 last nit.

>+        print >>sys.stderr, "Error: Mozilla directory renaming failed."

Nit: (2 places) s/Error/Warning/ please since we aren't failing out on this.

I'm ok with actually failing out and leaving it as a real error though.

Comment 8

[Serge Gautherie (:sgautherie)]

(In reply to comment #6)
> That would not work as is, because if move_to_stable() fails then do_hg_pull()
> would execute over the cvs dir :-/

(I guess I'm too tired tonight, yet I'll make a last attempt...)
My comment was wrong:

cvs/hg mix applies to venkman and is not affected by repo_config(),
so your comment 5 proposal should be just fine wrt to current code.

Yet, my patch adds 2 new "features":
*be able to use --skip-mozilla to support (my) experimental SM/1.9.2 :->
*make sure to not do_hg_pull('mozilla', ...) over the unintended/trunk repo..
To achieve these, the dependency must be --skip-mozilla > move_to_stable() > do_hg_pull().

So I think it's best to settle by what we discussed iirc:
*check in this r+ patch, with "Warning" nit, for now.
*revisit the repo_config() issue in a follow-up bug, when we do have an new case to handle (and actually know what we want to do).

Comment 9

[Justin Wood (:Callek)]

(In reply to comment #8)
> (In reply to comment #6)
> > That would not work as is, because if move_to_stable() fails then do_hg_pull()
> > would execute over the cvs dir :-/
> 
> (I guess I'm too tired tonight, yet I'll make a last attempt...)
> My comment was wrong:
> 
> cvs/hg mix applies to venkman and is not affected by repo_config(),
> so your comment 5 proposal should be just fine wrt to current code.
> 
> Yet, my patch adds 2 new "features":
> *be able to use --skip-mozilla to support (my) experimental SM/1.9.2 :->

Having mozilla-trunk should be fine wrt a .treestate already in place.

> *make sure to not do_hg_pull('mozilla', ...) over the unintended/trunk repo..
> To achieve these, the dependency must be --skip-mozilla > move_to_stable() >
> do_hg_pull().
> 
> So I think it's best to settle by what we discussed iirc:
> *check in this r+ patch, with "Warning" nit, for now.
> *revisit the repo_config() issue in a follow-up bug, when we do have an new
> case to handle (and actually know what we want to do).

I'm ok with this. If I (or someone else) later [such as after sleep] feels things need to change, we can easily fix.

Comment 10

[Mark Banner (:standard8)]

(In reply to comment #9)
> (In reply to comment #8)
> > Yet, my patch adds 2 new "features":
> > *be able to use --skip-mozilla to support (my) experimental SM/1.9.2 :->
> 
> Having mozilla-trunk should be fine wrt a .treestate already in place.

I'm also failing to see why you need to modify --skip-mozilla to support 1.9.2.

Pulling 1.9.2 is easy. First run you do:

python client.py checkout --mozilla-repo=http://hg.mozilla.org/mozilla-central/

after that you can just do

python client.py checkout

as normal. The first run will set the .treestate file, but because you've specified the repo it will get mozilla-central. After that it already has created the .treestate file so you can use checkout as normal.

Unless there's some other case you're thinking of, of course.

Comment 11

[Robert Kaiser]

As a note, I don't feel too well with continuing client.py in cases where errors are spotted (and maybe ending it with output that looks like things were OK, including a 0 return value on the shell, which sound like "everything OK" to scripts), as that means we're compiling builds with inconsistent comm+mozilla states, both on private machines and buildbots and we still think everything's OK, which is wrong.
If any step fails, the script should fail and tell what failed. If we support an ignore-error case, it should be a non-default option.

Comment 12

[Robert Kaiser]

If the only thing this is trying to achieve is support for building with mozilla-central, this is already supported and just needs better documentation. Once you have a .treestate file _and_ and mozilla-central copy in mozilla/, you can just do a normal client.py checkout and it updates all those repos (including m-c) just fine. One simple version to get into this state is what comment #10 says.

(In reply to comment #8)
> cvs/hg mix applies to venkman and is not affected by repo_config(),
> so your comment 5 proposal should be just fine wrt to current code.

Erm, you would pull a new venkman into a possibly obsolete mozilla/ dir and not care? And when the user realizes that, you'd pull the extensions again anyways?

Comment 13

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) 1 optimization, plus some reorderings [Checkin: Comment 16]

Let's do these parts yet.

Comment 14

[Justin Wood (:Callek)]

Comment on attachment 368427 [details] [diff] [review]
(Av1a) Bug 482686 comment 10 part

(From update of attachment 368427 [details] [diff] [review])
Ok having thought on it, I'm rescinding my r+...

* Use my c#5 suggestion. (Your reasons for not doing it are not very strong imo)

>+        # Print the exception without its traceback.
>+        sys.excepthook(sys.exc_info()[0], sys.exc_info()[1], None)
>+        print >>sys.stderr, "Error: Mozilla directory renaming failed."
>+        # Abort operation(s).
>+        return False

Drop the True/False values from the return(s) [multiple places]
And use sys.exit("Error: ...") instead. It automatically prints to stderr and exits the app (I do not agree with continuing as if everything is ok)

Since I'm getting rid of the return False option in move_to_stable, no if there, just do it in repo_config (if it fails we'll exit)

>-    if not options.skip_venkman:
>+    if not options.skip_venkman and backup_cvs_venkman():
>+        fixup_venkman_repo_options(options)

Since you're splitting these fixup_*_options up, lets do the backup_cvs_venkman at the start of the fixup function, and keep fixup on its own line, no need to test the backup* part here, as I'm doing away with the option to fail and continue.

Comment 15

[Justin Wood (:Callek)]

Comment on attachment 372293 [details] [diff] [review]
(Bv1) 1 optimization, plus some reorderings
[Checkin: Comment 16]

this is good and ok to land as is. if you want to proceed with the other updates(with my fixes in c#14) just provide a new patch.

Comment 16

[Serge Gautherie (:sgautherie)]

Comment on attachment 372293 [details] [diff] [review]
(Bv1) 1 optimization, plus some reorderings
[Checkin: Comment 16]


http://hg.mozilla.org/comm-central/rev/5c061c0df29c

Comment 17

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) Improve current code and documentation, use sys.exit() more [Checkin: See comment 28]

This is with comment 14 (and previous comments) suggestions.

Note that this does not (attempt to) solve my "--skip-mozilla" case.

Comment 18

[Serge Gautherie (:sgautherie)]

(In reply to comment #9)
> > *be able to use --skip-mozilla to support (my) experimental SM/1.9.2 :->
> 
> Having mozilla-trunk should be fine wrt a .treestate already in place.

But this file does not exist before branching (from m-central to m-1.9.1) happens!


(In reply to comment #10)
> Pulling 1.9.2 is easy.

Your steps are for an initial checkout (on top of nothing).

> Unless there's some other case you're thinking of, of course.

My case is wanting to keep working on m-c without branching to 1.9.1.


(In reply to comment #12)
> [...] support for building with mozilla-central,
> this is already supported and just needs better documentation.
> Once you have a .treestate file

Documenting and coding this to get an easy support is my point...

Comment 19

[Robert Kaiser]

(In reply to comment #18)
> My case is wanting to keep working on m-c without branching to 1.9.1.

then do a "touch .treestate" before the fact or a "rm -rf mozilla; mv .mozilla-trunk mozilla" after the fact and it's done.

Comment 20

[Mark Banner (:standard8)]

(In reply to comment #19)
> (In reply to comment #18)
> > My case is wanting to keep working on m-c without branching to 1.9.1.
> 
> then do a "touch .treestate" before the fact or a "rm -rf mozilla; mv
> .mozilla-trunk mozilla" after the fact and it's done.

Alternately, "python client.py checkout --mozilla-repo=http://hg.mozilla.org/mozilla-central/" works on first run, just like the Thunderbird tinderboxes do: http://hg.mozilla.org/build/buildbot-configs/file/e022c8818673/thunderbird/config.py#l151

Comment 21

[Mark Banner (:standard8)]

(In reply to comment #18)
> (In reply to comment #10)
> > Pulling 1.9.2 is easy.
> 
> Your steps are for an initial checkout (on top of nothing).
> 
> > Unless there's some other case you're thinking of, of course.
> 
> My case is wanting to keep working on m-c without branching to 1.9.1.

Ok so I just saw this. The initial checkout using --mozilla-repo will create a .treestate file, after that, we won't be moving you to 1.9.1 because of the .treestate file being in existence.

So, I think --mozilla-repo covers initial checkout, .treestate covers existing checkouts... what other case is there that we'd need --skip-mozilla? ... unless you don't want to update your mozilla repo, in which case you can just do hg pull -u in the comm-central directory.

Comment 22

[Serge Gautherie (:sgautherie)]

(In reply to comment #19)
> then do a "touch .treestate" before the fact or a "rm -rf mozilla; mv
> .mozilla-trunk mozilla" after the fact and it's done.

Yes, these are the two manual solutions,
and yes, the former is much better than the latter.

And my point is to actually document/code this case...


(In reply to comment #20)
> Alternately, "python client.py checkout --mozilla-repo=..." works on first run,

No more than before, see my comment 18.


(In reply to comment #21)
> Ok so I just saw this. The initial checkout using --mozilla-repo will create a
> .treestate file, after that, we won't be moving you to 1.9.1 because of the
> .treestate file being in existence.

(What's the point repeating the same thing yet again? The initial checkout case is not the problem.)

> .treestate covers existing checkouts...

It does *not* because that file does not exist in *existing before branching* checkout! See my comment 18.

> what other case is there that we'd need --skip-mozilla? ... unless
> you don't want to update your mozilla repo, in which case you can just do hg
> pull -u in the comm-central directory.

I do want to update m-c: what I don't want is branch to m-1.9.1.

Comment 23

[Robert Kaiser]

(In reply to comment #22)
> > .treestate covers existing checkouts...
> 
> It does *not* because that file does not exist in *existing before branching*
> checkout! See my comment 18.

The whole point of introducing it was that existing _old_ checkouts should _always_ convert to 1.9.1 _unless explicitely requested otherwise_ and I don't see any reason at all to change that.

Comment 24

[Mark Banner (:standard8)]

(In reply to comment #22)
> It does *not* because that file does not exist in *existing before branching*
> checkout! See my comment 18.

Ah so that's the use case. In which case the question becomes how many of these do you have? It sounds like you have hundreds not a couple, and sometime in the next couple of months we'll probably be branching comm-central anyway.

Comment 25

[Justin Wood (:Callek)]

Comment on attachment 372857 [details] [diff] [review]
(Cv1) Improve current code and documentation, use sys.exit() more
[Checkin: See comment 28]

>+    # No cvs backup needed as DOM Inspector was part (and removed from)
>+    # Mozilla hg repository.
>+
>+    if options.inspector_repo is None and \
>+            not os.path.exists(os.path.join(topsrcdir, 'mozilla', 'extensions', 'inspector')):
>         options.inspector_repo = DEFAULT_INSPECTOR_REPO
Nit: blank line

Good to land, lets split off any .tree-state/m-c-1.9.1 related stuff for another bug... I do have a few ideas how to handle but I'm going to want to update my tree again anyway before I bother... (and I'll welcome ideas)

Comment 26

[Serge Gautherie (:sgautherie)]

(In reply to comment #25)
> (From update of attachment 372857 [details] [diff] [review])
> >+    # No cvs backup needed as DOM Inspector was part (and removed from)
> >+    # Mozilla hg repository.
> >+
> >+    if options.inspector_repo is None and \
> >+            not os.path.exists(os.path.join(topsrcdir, 'mozilla', 'extensions', 'inspector')):
> >         options.inspector_repo = DEFAULT_INSPECTOR_REPO
> Nit: blank line

I'm not sure what you want...

> let's split off any .tree-state/m-c-1.9.1 related stuff for
> another bug...

Agreed.

Comment 27

[Justin Wood (:Callek)]

(In reply to comment #26)
> (In reply to comment #25)
> > (From update of attachment 372857 [details] [diff] [review] [details])
> > >+    # No cvs backup needed as DOM Inspector was part (and removed from)
> > >+    # Mozilla hg repository.
> > >+
> > >+    if options.inspector_repo is None and \
> > >+            not os.path.exists(os.path.join(topsrcdir, 'mozilla', 'extensions', 'inspector')):
> > >         options.inspector_repo = DEFAULT_INSPECTOR_REPO
> > Nit: blank line
> 
> I'm not sure what you want...

Just remove the blank line there......

Comment 28

[Serge Gautherie (:sgautherie)]

Comment on attachment 372857 [details] [diff] [review]
(Cv1) Improve current code and documentation, use sys.exit() more
[Checkin: See comment 28]


http://hg.mozilla.org/comm-central/rev/4270b7ebb156
Cv1, with comment 27 suggestion(s).

Comment 29

[Serge Gautherie (:sgautherie)]

(In reply to comment #23)

> The whole point of introducing it was that existing _old_ checkouts should
> _always_ convert to 1.9.1 _unless explicitely requested otherwise_ and I don't
> see any reason at all to change that.

I never wanted to change that: I'm only saying the "unless explicitly requested otherwise" part is not (fully) coded!


(In reply to comment #24)
> the question becomes how many of these do you have?

One. But I don't see what difference it makes (now and later).


(In reply to comment #25)
> lets split off any .tree-state/m-c-1.9.1 related stuff for
> another bug... I do have a few ideas how to handle but I'm going to want to
> update my tree again anyway before I bother... (and I'll welcome ideas)

Too much bashing on that issue: I'm giving up.


(In reply to comment #0)
> and bug 482686 comment 18 about KaiRo's idea.

This would still depend on bug 475011...

Comment 30

[Serge Gautherie (:sgautherie)]

(In reply to comment #29)
> (In reply to comment #0)
> > and bug 482686 comment 18 about KaiRo's idea.
> This would still depend on bug 475011...

Will be done in bug 524682.