comm-central should pull Venkman from hg rather than CVS

RESOLVED FIXED

Status

defect
--
critical
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Gijs, Assigned: sgautherie)

Tracking

Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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).
(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.
Assignee: build-config → nobody
Product: SeaMonkey → MailNews Core
QA Contact: build-config → build-config
Summary: SeaMonkey should pull Venkman from hg rather than CVS → comm-central should pull Venkman from hg rather than CVS
Version: unspecified → Trunk
Posted patch (Av1) Pull Hg instead of cvs (obsolete) — Splinter Review
(Untested, but done in the same way as DOMi bug 437064.)

(How) Do we want to handle switching existing cvs checkout to Hg repository ?
Assignee: nobody → sgautherie.bz
Status: NEW → ASSIGNED
Attachment #367271 - Flags: review?(kairo)
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!
Attachment #367271 - Flags: review?(kairo) → review-
Posted patch (Av2) Pull Hg instead of cvs (obsolete) — Splinter Review
Av1, with comment 3 suggestion(s).
Attachment #367271 - Attachment is obsolete: true
Attachment #367313 - Flags: review?(kairo)
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").
Attachment #367313 - Flags: review?(kairo) → review-
(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.
(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.
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
(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.
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.
Attachment #367313 - Attachment is obsolete: true
Attachment #367901 - Flags: review?(kairo)
Attachment #367901 - Flags: review?(kairo) → review-
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.
Posted patch (Av4) Pull Hg instead of cvs (obsolete) — Splinter Review
Av3, fixing past and present but without "future".
Attachment #367901 - Attachment is obsolete: true
Attachment #368170 - Flags: review?(kairo)
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.
Attachment #368170 - Flags: review+
(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).
(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 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.
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".)
Attachment #368170 - Attachment is obsolete: true
Attachment #368242 - Flags: review?(kairo)
Attachment #368170 - Flags: review?(kairo)
Attachment #368242 - Flags: review?(kairo) → review+
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 on attachment 368242 [details] [diff] [review]
(Av5) Pull Hg instead of cvs
[Checkin: Comment 19]


http://hg.mozilla.org/comm-central/rev/c09d5a915273
Attachment #368242 - Attachment description: (Av5) Pull Hg instead of cvs → (Av5) Pull Hg instead of cvs [Checkin: Comment 19]
Blocks: 484205
I filed bug 484205 for all the follow-up improvements.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Flags: in-testsuite-
Blocks: 508392
You need to log in before you can comment on or make changes to this bug.