482686 - comm-central should pull Venkman from hg rather than CVS

Status: RESOLVED FIXED

Description

[:Gijs (he/him)]

As in summary. Repo is at http://hg.mozilla.org/venkman/ . Let me know if there are problems, it should be straightforward if the CVS import worked properly (which afaict it has, but you never know).

Comment 1

[Mark Banner (:standard8) (afk 19th - 28th July)]

(In reply to comment #0)
> As in summary.

comm-central actually ;-) TB & SB can build with it even if they don't ship with it.

Comment 2

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Pull Hg instead of cvs

(Untested, but done in the same way as DOMi bug 437064.)

(How) Do we want to handle switching existing cvs checkout to Hg repository ?

Comment 3

[Robert Kaiser]

Comment on attachment 367271 [details] [diff] [review]
(Av1) Pull Hg instead of cvs

Please don't move around where vars/backs are defined, just replace them where they are and leave whatever quoting we have in place. If you feel you want to do a quoting/ordering change/cleanup, please do that in a different bug/patch (and no, I'm not convinced that it's either needed or a good idea).

Thanks for looking into this, though!

Comment 4

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) Pull Hg instead of cvs

Av1, with comment 3 suggestion(s).

Comment 5

[Robert Kaiser]

Comment on attachment 367313 [details] [diff] [review]
(Av2) Pull Hg instead of cvs

I'm deeply sorry I need to r- again, but testing showed (as I feared) that it doesn't work as smooth as we'd like it to.

>+    # Handle special case: initial checkout of Venkman.
>+    if (options.venkman_repo is None
>+            and not os.path.exists(os.path.join(topsrcdir, "mozilla", "extensions", "venkman"))):
>+        options.venkman_repo = DEFAULT_VENKMAN_REPO

This by itself works fine in the case where mozilla/extensions/venkman doesn't exist. We have a case for all already pulled repos though that is different: the directory is in place but pulled from CVS instead of hg, and in this case we fail as we'll try to do a "hg pull -R ./mozilla/extensions/venkman -r tip" - on a cvs checkout.
Somewhere before getting to the block cited above (possibly exactly before this), we need to detect if "mozilla/extensions/venkman/CVS" exists, and if it does, I'd propose to move extensions/venkman to extensions/venkman-cvs (to prevent dataloss for devs who have patches in there) and only then test for the venkman subdir and do an initial checkout.
This is different from inspector as that wasn't pulled from somewhere else before and just would go away with a mozilla-central checkout back then.

>+    if not options.skip_venkman:
>+        do_hg_pull(os.path.join("mozilla", "extensions", "venkman"), options.venkman_repo, options.hg, options.venkman_rev)
>+

Nit: we have single quotes for all other paths in that section, please be consistent and do that here as well.

>-    if not options.skip_venkman:
>-        if os.path.exists(os.path.join(topsrcdir, 'mozilla', 'extensions')):
>-          do_cvs_checkout(VENKMAN_DIRS, VENKMAN_CO_TAG, options.cvsroot, options.cvs, 'mozilla')
>-        else:
>-          print >>sys.stderr, "Warning: mozilla/extensions does not exist, Venkman could not be checked out."
>-          pass

Actually, I think we probably should have that test for mozilla/extensions still in place (even for inspector), as this can happen when --skip-mozilla is present and someone deleted extensions/ or so, and the error message the checkout itself is generating is way less helpful ("abort: No such file or directory: mozilla/extensions/venkman").

Comment 6

[Serge Gautherie (:sgautherie)]

(In reply to comment #5)
> I'm deeply sorry I need to r- again

Don't be: this is exactly why I asked if/what you wanted to do about it.

> we need to detect if "mozilla/extensions/venkman/CVS" exists, and if it
> does, I'd propose to move extensions/venkman to extensions/venkman-cvs

I'll do.

> Nit: we have single quotes for all other paths in that section, please be
> consistent and do that here as well.

Well, this file uses both styles :-/
So I'd prefer to be consistent with myself: lean toward double quotes...

> Actually, I think we probably should have that test for mozilla/extensions
> still in place (even for inspector)

Agreed: I'll fix this other issue too.

Comment 7

[Robert Kaiser]

(In reply to comment #6)
> (In reply to comment #5)
> > Nit: we have single quotes for all other paths in that section, please be
> > consistent and do that here as well.
> 
> Well, this file uses both styles :-/
> So I'd prefer to be consistent with myself: lean toward double quotes...

It does, but not as inconsistently as you might think: All path and command names use single-quoting, all text output uses double-quoting, repo URLs use single-quoting (except where in a string that has replacement or matching expressions in it), as do CVS tags, hg revisions use double-quotes (IMHO that's the largest inconsistency, but not in the category of strings, only to CVS tags).

So, please follow this style in that you use the same style for the same type of strings, i.e. single quotes ofr all paths and for the repo URL.

Comment 8

[Justin Wood (:Callek)]

I'd suggest .treestate iteration for if (in the non-clean checkout case) we need to remove venkman (or at least test for CVS). and iterate .treestate ver

Comment 9

[Justin Wood (:Callek)]

(In reply to comment #8)
> I'd suggest .treestate iteration for if (in the non-clean checkout case) we
> need to remove venkman (or at least test for CVS). and iterate .treestate ver

For the record, over IRC KaiRo dislikes this idea, and I'll defer to his decision here.

Comment 10

[Serge Gautherie (:sgautherie)]

Attachment: (Av3) Pull Hg instead of cvs, plus unrelated improvements

Av2, with comment 5 suggestion(s),
plus unrelated improvements.

The main new feature of the unrelated improvements is to be able to use --skip-mozilla to support my experimental SM/1.9.2 :->
(Instead of finding out and manually faking the .treestate file, which I never did.)

I tested with/out all the --skip*, except --skip-mozilla which I always used.

Comment 11

[Robert Kaiser]

Comment on attachment 367901 [details] [diff] [review]
(Av3) Pull Hg instead of cvs, plus unrelated improvements

Please don't mix any thing else than what this bug is about into this patch, file a new bug for anything else than the venkman move.

Comment 12

[Serge Gautherie (:sgautherie)]

Attachment: (Av4) Pull Hg instead of cvs

Av3, fixing past and present but without "future".

Comment 13

[Justin Wood (:Callek)]

Comment on attachment 368170 [details] [diff] [review]
(Av4) Pull Hg instead of cvs

>+def backup_cvs_venkman():
>+    if not os.path.exists(os.path.join(venkmanpath, 'CVS')):
>+        return True

Nit: Why return anything (True/False) when backup_cvs_* does not read any data from the return value?

> repo_config()
> 
>+backup_cvs_venkman()

Not clearing KaiRo's review flag as I feel he should still make a pass at this.

Comment 14

[Serge Gautherie (:sgautherie)]

(In reply to comment #13)
> Nit: Why return anything (True/False) when backup_cvs_* does not read any data
> from the return value?

See (interdiff with) patch Av3 for how this intended to be used (later).

Comment 15

[Justin Wood (:Callek)]

(In reply to comment #14)
> (In reply to comment #13)
> > Nit: Why return anything (True/False) when backup_cvs_* does not read any data
> > from the return value?
> 
> See (interdiff with) patch Av3 for how this intended to be used (later).

Could we do that later thing actually later then?

Comment 16

[Robert Kaiser]

Comment on attachment 368170 [details] [diff] [review]
(Av4) Pull Hg instead of cvs

>+        # Print the exception without its traceback.
>+        sys.excepthook(sys.exc_info()[0], sys.exc_info()[1], None)

Isn't that what python does by itself anyway when you don't  do a try/except in the first place? If so, we could vastly save code by doing just that ;-)

>+        return False
>+
>+    return True

Just do plain return as long as we don't care about the return value and only introduce a return value once we might care.


> def do_cvs_checkout(modules, tag, cvsroot, cvs, checkoutdir):
>-    """Check out a CVS directory into the mozilla/ subdirectory.
>+    """Check out a CVS directory into the checkoutdir subdirectory.

Nit: leave that comment unchanged for now, what it says is still true atm ;-)

>+    # Check whether destination directory exists for these extensions.

Nice idea to group those checks!


Waiting for a reply to the first question before marking +/- as there might be a possible improvement lurking there.

Comment 17

[Serge Gautherie (:sgautherie)]

Attachment: (Av5) Pull Hg instead of cvs [Checkin: Comment 19]

Av4, with comment 16 suggestion(s).

(In reply to comment #16)
> >+        # Print the exception without its traceback.
> 
> Isn't that what python does by itself anyway when you don't  do a try/except in
> the first place?

Python will print the traceback too and abort ... which I agree is what is wanted atm as this is not (yet) error checked :-|

> >-    """Check out a CVS directory into the mozilla/ subdirectory.
> 
> Nit: leave that comment unchanged for now, what it says is still true atm ;-)

No: ChatZilla goes to 'mozilla' but Ldap goes to ''.
(This function does check out "from" 'mozilla/' but not "into".)

Comment 18

[Robert Kaiser]

Comment on attachment 368242 [details] [diff] [review]
(Av5) Pull Hg instead of cvs
[Checkin: Comment 19]

Thanks, this looks good to me now. Thanks for pointing out the checkoutdir thing, you're obviously right on that.

I think we could even make the backup_cvs_venkman() more generic in the future, so that chatzilla may possibly once use the same thing, but we can leave that for later refactoring.

Comment 19

[Serge Gautherie (:sgautherie)]

Comment on attachment 368242 [details] [diff] [review]
(Av5) Pull Hg instead of cvs
[Checkin: Comment 19]


http://hg.mozilla.org/comm-central/rev/c09d5a915273

Comment 20

[Serge Gautherie (:sgautherie)]

I filed bug 484205 for all the follow-up improvements.