User-Agent: Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.9.2a1pre) Gecko/20090528 Minefield/3.6a1pre Build Identifier: Because we don't have symlinks on OS/2 all the Test*.exe programs get copied over to $(DIST). Make package includes them in the generated zip file. They should be excluded. Unfortunately not all test programs contain the pattern *{tT}est* in their designations, thus adding *Test* and *test* to http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/packager.mk#228 is not enough. Moreover, some tests do copy over files in the components and other subdirs. Reproducible: Always

OS X builds have the same problem, FWIW, since they don't use a packaging manifest (bug 463605). We just build our nightlies with --disable-tests at the moment.

I noticed that it also includes js.exe which does not seem to be packaged for other platforms. If we made a manifest (probably based on http://mxr.mozilla.org/mozilla-central/source/browser/installer/windows/packages-static), that would fix this, too. Or should that just be added to NO_PKG_FILES (as "js" is there, probably for Linux)?

the "js" is probably for OS X, which is the only platform that we ship official builds for that doesn't have a packaging manifest. Linux and Windows both have manifests, so they only package exactly what we say.

(In reply to comment #2) > I noticed that it also includes js.exe which does not seem to be packaged for > other platforms. If we made a manifest (probably based on > http://mxr.mozilla.org/mozilla-central/source/browser/installer/windows/packages-static), > that would fix this, too. Yes, that's what I was thinking also. However, I wanted to wait if something will happen with the mac bug463605. But it appears not. Somehow, I thought it could be an alternative to hook up to the windows manifest (with a few ifdefs where we don't have the same shortlib names or where we don't have anything to package) - but windows people might be not be very happy about this idea.

I am going to fix bug 463605 somehow in the next few months. One thing I might wind up doing is combining all the platform-specific manifests and having one file with #ifdefs, like we do with removed-files.in.

(In reply to comment #5) > I am going to fix bug 463605 somehow in the next few months. One thing I might > wind up doing is combining all the platform-specific manifests and having one > file with #ifdefs, like we do with removed-files.in. That's a word, I've put me on the cc list of that bug. I think we can wait until there is coming up a solution. When there will be a unique manifest file, I could add the bits we need there. If, however, adding us will be too complicated for whatever reason, we could reopen this bug here. Peter, do you agree? If yes, what would be the be the best resolution of this bug here, duplicate or incomplete or ...?

first draft on top of https://bug511642.bugzilla.mozilla.org/attachment.cgi?id=397283 (v2 of Ted's unified patch). I've put the OS/2 bits into an extra-section, though there's some redundancy with windows sections. However, I didn't find a way yet to get the preprocessor.pl script to accept something like #if defined(XP_WIN32) || defined(XP_OS2). I've seen only a few warnings during the packaging process. Nevertheless, I'm sure the structure of the unified package manifest will change until I'm back.

with this patch there's only a warning about necko_wifi.xpt, which is ok, because we don't have it on OS/2 yet.

Great, I thought there would be a lot more ifdefing. I would like it better if you moved the "for OS/2" lines to where you added the ifndef XP_OS2, but let's first see what Ted says.

Comment on attachment 406087 [details] [diff] [review] better patch add some ifdef/ifndef XP_OS2 in the main section -#ifdef XP_WIN32 +#ifndef XP_UNIX I feel uncomfortable with these changes, but I can't say why. I guess it gives the same result (except for OS/2), so I'm probably imagining things. +; for OS/2 +#ifdef XP_OS2 +@BINPATH@/components/brwsrdir@DLL_SUFFIX@ +@BINPATH@/components/brwsrcmp@DLL_SUFFIX@ +#endif + Please stick these up in an #else where you #ifdef'ed the other files.

hm, forgot to recheck the former patch for correct line endings, sorry for the spam

Something's not right here: @BINPATH@/plugins/npnul32.dll +#elifdef XP_OS2 #endif +@BINPATH@/plugins/npnulos2.dll #endif Isn't the #elifdef supposed to be after the #endif?

(In reply to comment #13) > Something's not right here: >> > Isn't the #elifdef supposed to be after the #endif? Yeah, cut 'n' paste mistake, sorry.

Comment on attachment 407813 [details] [diff] [review] for checkin addressed Ted's comments Setting the review flag yourself isn't useful, especially if you expect others to write the commit message for you.

(In reply to comment #15) > Setting the review flag yourself isn't useful, especially if you expect others > to write the commit message for you. I thought that carrying over reviews was standard practice in Bugzilla. Now, if someone comes to this bug, he sees that it's fixed but doesn't easily find out by which patch, because there is only an obsolete one that has review. (And if you use qimportbz for checkin-needed patches, you have to use -p in any case, right?)

The checked-in patch is the one that isn't obsolete. That seems quite clear to me.

Comment on attachment 408047 [details] [diff] [review] this time for real This (basically OS/2-only) packaging change didn't break trunk, and we would like to get it on 1.9.2 as well.

There are some subtle differences on the 1.9.2 branch still which means that we need a slightly changed patch. So I better ask for review again for this (I'm not 100% sure I understand the pre-processor handling).

(In reply to comment #18) > The checked-in patch is the one that isn't obsolete. That seems quite clear to me. As you can see now, things often do not work out as simple as that...

Two patches, one for trunk and one for branch. Still no problem. "wuno: review+", on the other hand, just obscures who did the review. I don't see why you link that to the question which patch landed.

Comment on attachment 409545 [details] [diff] [review] for 1.9.2 Trunk tested, OS/2-only build patch in cross-platform file.

Comment on attachment 409545 [details] [diff] [review] for 1.9.2 approval1.9.2 requests aren't currently being monitored, since we're nearing RC freeze and there are too many outstanding requests, so I'm clearing this request. Feel free to re-request approval if you are confident that it's worth drivers' time to consider whether this non-blocker needs to land for 1.9.2 at this stage.

Comment on attachment 409545 [details] [diff] [review] for 1.9.2 Bug 526630 that makes little sense without this was approved and landed on 1.9.2, so I re-request approval. (We cannot rely on automatic packaging of builds on OS/2 without this.)

Comment on attachment 409545 [details] [diff] [review] for 1.9.2 Moving for when approvals re-open for branch. We'll probably want to take it.

Comment on attachment 409545 [details] [diff] [review] for 1.9.2 Clearing old approval requests now that 1.9.2.4 has shipped. If you believe this patch is still necessary on the 1.9.2 branch please re-request approval along with a risk/benefit analysis explaining why we need it.

Comment on attachment 409545 [details] [diff] [review] for 1.9.2 (In reply to comment #27) re-request approval > along with a risk/benefit analysis explaining why we need it. zero risk (all is ifdef'd os2) patch was checked in on trunk w/o problems 9 months ago. Benefit would be for OS/2 as we cannot do automatic packaging of 3.6.x builds without this patch. See also comment #25

Comment on attachment 409545 [details] [diff] [review] for 1.9.2 a=LegNeato for 1.9.2. Please land this on mozilla-1.9.2 default.