Closed
Bug 482686
Opened 13 years ago
Closed 13 years ago
comm-central should pull Venkman from hg rather than CVS
Categories
(MailNews Core :: Build Config, defect)
MailNews Core
Build Config
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: Gijs, Assigned: sgautherie)
References
Details
Attachments
(1 file, 4 obsolete files)
8.14 KB,
patch
|
kairo
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
(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
Assignee | ||
Comment 2•13 years ago
|
||
(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•13 years ago
|
||
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-
Assignee | ||
Comment 4•13 years ago
|
||
Av1, with comment 3 suggestion(s).
Attachment #367271 -
Attachment is obsolete: true
Attachment #367313 -
Flags: review?(kairo)
![]() |
||
Comment 5•13 years ago
|
||
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-
Assignee | ||
Comment 6•13 years ago
|
||
(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•13 years ago
|
||
(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•13 years ago
|
||
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•13 years ago
|
||
(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.
Assignee | ||
Comment 10•13 years ago
|
||
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)
![]() |
||
Updated•13 years ago
|
Attachment #367901 -
Flags: review?(kairo) → review-
![]() |
||
Comment 11•13 years ago
|
||
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.
Assignee | ||
Comment 12•13 years ago
|
||
Av3, fixing past and present but without "future".
Attachment #367901 -
Attachment is obsolete: true
Attachment #368170 -
Flags: review?(kairo)
Comment 13•13 years ago
|
||
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+
Assignee | ||
Comment 14•13 years ago
|
||
(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•13 years ago
|
||
(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•13 years ago
|
||
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.
Assignee | ||
Comment 17•13 years ago
|
||
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)
![]() |
||
Updated•13 years ago
|
Attachment #368242 -
Flags: review?(kairo) → review+
![]() |
||
Comment 18•13 years ago
|
||
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.
Assignee | ||
Comment 19•13 years ago
|
||
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]
Assignee | ||
Comment 20•13 years ago
|
||
I filed bug 484205 for all the follow-up improvements.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Flags: in-testsuite-
You need to log in
before you can comment on or make changes to this bug.
Description
•