update-packaging tools need their own target

RESOLVED FIXED

Status

()

Toolkit
Application Update
P3
normal
RESOLVED FIXED
13 years ago
9 years ago

People

(Reporter: preed, Assigned: preed)

Tracking

Trunk
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

13 years ago
Right now, to get the tools necessary to create/manipulate mar files, either a) a full build must be done, or b) you must know which directories need to be compiled, in the right order, to create the two necessary tools, mbsdiff and mar (they typically live in dist/host/bin).

A target needs to be created so you can pull the relevant subset of the tree, and make a target to build these two utilities and that's it.
(Assignee)

Comment 1

13 years ago
Created attachment 214373 [details] [diff] [review]
Create a tools/update-packaging target

This creates an --enable-application=tools/update-packaging target, much like the content/xslt target (per bsmedberg's request).

Tested by:

-- applying patch to a clean tree
-- Running autoconf
-- ./configure --enable-application=tools/update-packaging
-- make

Gives us:

[preed@preed-lx mozilla]$ ls dist/host/bin/
mar  mbsdiff  nsinstall

Which is what is wanted.

What this patch is missing: a proper checkout target, for client.mk.

Right now, checking out the browser mostly works; mozilla/other-licenses/mbsdiff is also needed (it's not pulled by default; maybe it should be?)
Attachment #214373 - Flags: review?(darin)

Comment 2

13 years ago
Comment on attachment 214373 [details] [diff] [review]
Create a tools/update-packaging target

Don't you need to add something to allmakefiles.sh?  Would be good to get bsmedberg to review this, but I know that he is out of town.  I suggest requesting review from him after-the-fact.

Comment 3

13 years ago
s/out of town/a new father/
(Assignee)

Comment 4

13 years ago
(In reply to comment #2)
> (From update of attachment 214373 [details] [diff] [review] [edit])
> Don't you need to add something to allmakefiles.sh?  Would be good to get
> bsmedberg to review this, but I know that he is out of town.  I suggest
> requesting review from him after-the-fact.

I don't *think* it's always required, but I've never added a produt before, so maybe?

I notice that there were no additional entries added to allmakefiles.sh for the xslt move (Bug 304494), and this doesn't create any new Makefiles (in terms of new Makefile.in's).

Totally agree on bsmedberg reviewing, but this is semi-blocking (or at least, related to) bug 327140; there will likely be a second part to this, which is to create an appropriate client.mk CVS pull target for this stuff; we should be able to clean up any horribleness then.
Status: NEW → ASSIGNED

Comment 5

13 years ago
> I don't *think* it's always required, but I've never added a produt before, so
> maybe?
> 
> I notice that there were no additional entries added to allmakefiles.sh for the
> xslt move (Bug 304494), and this doesn't create any new Makefiles (in terms of
> new Makefile.in's).

What about other-licenses/bsdiff/Makefile.in?  How does a Makefile get generated for that if we don't have a line in allmakefiles.sh?
(In reply to comment #5)

> What about other-licenses/bsdiff/Makefile.in?  How does a Makefile get
> generated for that if we don't have a line in allmakefiles.sh?

The patch in bug 321650 adds it.

Comment 7

13 years ago
Comment on attachment 214373 [details] [diff] [review]
Create a tools/update-packaging target

>Index: Makefile.in
...
>+ifeq ($(MOZ_BUILD_APP),content/xslt) # {
> DIRS = \
> 	xpcom/typelib \
> 	xpcom \
> 	parser/expat \
> 	content/xslt/src \
> 	$(NULL)
>+	
>+	SUBMAKEFILES = xpcom/typelib/Makefile
> 
>-SUBMAKEFILES = xpcom/typelib/Makefile

The indentation of SUBMAKEFILES here seems a little wierd to me.
The other assignments to that variable are not similarly indented.

r=darin
Attachment #214373 - Flags: review?(darin) → review+
(Assignee)

Comment 8

13 years ago
(In reply to comment #7)

> The indentation of SUBMAKEFILES here seems a little wierd to me.
> The other assignments to that variable are not similarly indented.

Good point; fixed in the checkin, which I just committed.

Next up will be a change to pull the correct directories.
(Assignee)

Comment 9

13 years ago
Comment on attachment 214373 [details] [diff] [review]
Create a tools/update-packaging target

Per darin's suggestion, request an after-the-fact review from bsmedberg.
Attachment #214373 - Flags: superreview?(benjamin)
This patch has causes make to scan the entire tree for things to be done for 'tools'. Is that really needed? It takes quite some time...

Comment 11

13 years ago
That's not this patch, it was from bug 299988, and yes it's necessary.

Comment 12

13 years ago
Comment on attachment 214373 [details] [diff] [review]
Create a tools/update-packaging target

I'm not sold on the bracing style: it seems to me that the file is even less readable than it was.

The indent of SUBMAKEFILES is a little odd (and really shouldn't be necessary!)
Attachment #214373 - Flags: superreview?(benjamin) → superreview+
(Assignee)

Comment 13

13 years ago
(In reply to comment #12)

> The indent of SUBMAKEFILES is a little odd (and really shouldn't be necessary!)

That was fixed in the final checkin. My bad.
(Assignee)

Comment 14

12 years ago
Reassigning bugs I'm not actively working on back into the triage pool.
Assignee: preed → build
Status: ASSIGNED → NEW
Priority: -- → P3
May be more fixable given the work from bug 380846.
Assignee: build → nobody
(In reply to comment #15)
> May be more fixable given the work from bug 380846.

What else is left? A checkout target which only checks out the needed dirs (instead of all)?
Assignee: nobody → rhelmer
Assignee: rhelmer → nobody
In our brave new Mercurial world, partial checkouts aren't possible any more. We could stick this stuff in its own Mercurial repo if its useful to have it standalone.
Product: Firefox → Toolkit
Resolving fixed by preed's checkins since there hasn't been anyone requesting that these files go in their own repo and there were patches checked in that fixed the rest.
Assignee: nobody → mozpreed
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.