747394 - automation shouldn't fail when there's more than one application.ini in dist/

Status: RESOLVED INCOMPLETE

(Release Engineering :: General, defect, P2)

Description

[Chris AtLee [:catlee]]

something landed yesterday which adds a new file to the dist directory: dist/l10n-stage/firefox/webapprt/application.ini

this then breaks further steps which try and echo out the buildid, etc. assuming there's only one application.ini to be found.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

catlee: any chance you can point me to those further steps?  I tested l10n repacks on Windows and Mac with my patch for bug 746156, using the process documented at <https://developer.mozilla.org/en/Creating_a_Language_Pack#L10n_binary_repack>, and they seemed to work correctly.  But perhaps I missed some step.

Comment 2

[Chris AtLee [:catlee]]

For the purposes of this bug, we do something like this afterwards:

INIFILE=`find dist/l10n-stage -maxdepth 5 -type f -name application.ini`
BUILDID=`python config/printconfigsetting.py $INIFILE App BuildID`

In this case, the find command returns two files:
dist/l10n-stage/firefox/application.ini
dist/l10n-stage/firefox/webapprt/application.ini

And so INIFILE contains "dist/l10n-stage/firefox/application.ini\ndist/l10n-stage/firefox/webapprt/application.ini"

which causes the printconfigsetting.py command to fail and print a usage message instead, which is then used as the BUILDID for subsequent steps.

Comment 3

[Axel Hecht [:Pike]]

More things I found: linux doesn't fail, but it also uses a different command, i.e., -maxdepth 4. Different build factory?

I commented in the webapprt bug, this breaks while creating the incremental mar, which is hard to test locally. It might be easier to change the manifest name for webapprt, but I say that without knowing any possible fallout on that side.

Comment 4

[Chris AtLee [:catlee]]

(In reply to Axel Hecht [:Pike] from comment #3)
> More things I found: linux doesn't fail, but it also uses a different
> command, i.e., -maxdepth 4. Different build factory?

No, same factory, different step. There are two instances of this find command, one to find the current build's application.ini (with -maxdepth 5) and one to find the previous build's application (with -maxdepth 4).

Comment 5

[Myk Melez [:myk] [@mykmelez]]

(In reply to Axel Hecht [:Pike] from comment #3)
> I commented in the webapprt bug, this breaks while creating the incremental
> mar, which is hard to test locally. It might be easier to change the
> manifest name for webapprt, but I say that without knowing any possible
> fallout on that side.

We used to call it a different name, and we can do so again, if it's hard to fix this otherwise.  But is it necessary to traverse farther than the dist/l10n-stage/firefox/ directory?  Could we fix this by specifying `-maxdepth 1` ?

Comment 6

[Chris AtLee [:catlee]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #5)
> (In reply to Axel Hecht [:Pike] from comment #3)
> > I commented in the webapprt bug, this breaks while creating the incremental
> > mar, which is hard to test locally. It might be easier to change the
> > manifest name for webapprt, but I say that without knowing any possible
> > fallout on that side.
> 
> We used to call it a different name, and we can do so again, if it's hard to
> fix this otherwise.  But is it necessary to traverse farther than the
> dist/l10n-stage/firefox/ directory?  Could we fix this by specifying
> `-maxdepth 1` ?

I'm not sure, but I suspect -maxdepth 5 is in there to find application.ini for osx builds, which has paths like dist/l10n-stage/firefox/FirefoxNightly.app/Contents/MacOS/application.ini

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: rename application.ini to webapprt.ini

This patch should fix the problem (building now to make sure it doesn't break WebappRT; and requesting review from bsmedberg, who reviewed the original patch), although it still seems like something better fixed in build config.

Comment 8

[Benjamin Smedberg]

Comment on attachment 617032 [details] [diff] [review]
patch v1: rename application.ini to webapprt.ini

There's not really any need to rename the source file: just have the makefile name it:

+webapprt.ini: application.ini.in $(DEPTH)/config/buildid $(topsrcdir)/config/milestone.txt
 	$(PYTHON) $(topsrcdir)/config/Preprocessor.py $(DEFINES) $< > $@

If you do really want to rename the source file, please make sure that it is an hg move and not a file add/removal.

But I agree that this is not ideal and the build step should just look for the file in the specific location it is known to be in instead of running "find" for it.

Comment 9

[Justin Wood (:Callek)]

Comment on attachment 617032 [details] [diff] [review]
patch v1: rename application.ini to webapprt.ini

allow me to nit that webbapprt.ini be hg renamed instead of a new file as far as hg is concerned.

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: doesn't rename application.ini.in

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #8)
> Comment on attachment 617032 [details] [diff] [review]
> patch v1: rename application.ini to webapprt.ini
> 
> There's not really any need to rename the source file: just have the
> makefile name it:
> 
> +webapprt.ini: application.ini.in $(DEPTH)/config/buildid
> $(topsrcdir)/config/milestone.txt
>  	$(PYTHON) $(topsrcdir)/config/Preprocessor.py $(DEFINES) $< > $@

Ah, good point, done.


> If you do really want to rename the source file, please make sure that it is
> an hg move and not a file add/removal.

I generated the patch from a Git repository in which I "git mv"ed the file, which "git status" recognized; not sure why it came out as an add/remove in the patch.  But in any case this is not an issue in this version of the patch.


> But I agree that this is not ideal and the build step should just look for
> the file in the specific location it is known to be in instead of running
> "find" for it.

Yup.


catlee: Where is the code in question?

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Erm, I can land this, but I'd much rather first have a crack at the code I busted, to see if I can make it more robust against this innocent application.ini file in a subdirectory of the Firefox directory.  Can someone point me at that code?

Comment 12

[Chris AtLee [:catlee]]

the code in question is around here:

http://hg.mozilla.org/build/buildbotcustom/file/f40fec804a5e/process/factory.py#l3819

Comment 13

[Phil Ringnalda (:philor)]

Now that yesterday's nightly had two application.inis, generating partial updates failed when it tried the same sort of find to find the application.ini in yesterday's, leaving them red and sad, which probably means that we need to land the bustage fix, force a nightly on it and wait for it to upload and then fail in update generation, land a whitespace change, force a nightly on it, and get back to green.

mozilla-central is closed.

Comment 14

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: fix the build script

I don't know how to test this, but it looks correct, and the equivalent "find" command finds appropriate file in test directory hierarchies on my local machine.

It would probably be even better to construct the application.ini path explicitly rather than finding it, but I would need to get better acquainted with this code to figure out which properties hold the correct values for the name of the subdirectory of dist/l10n-stage/ (f.e. "firefox", so perhaps self.productName?) and the .app package (f.e. "FirefoxNightly.app") inside it.

philor, catlee: Not sure who should review, so asking both of you.

Comment 15

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v4: fix another FindFile(application.ini)

This tries to fix another place where the build script finds application.ini.  I looked through the rest of the file, and all the rest of the references to application.ini appear to look for it in a specific directory, so these two look to be the only ones in this file affected by the WebappRT checkin.

Not being the expert in this code, I'm uncertain whether the conditional should be checking `platform` or `updatePlatform`.  And, as I mentioned before, I don't know how to test this thoroughly.

Comment 16

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 617044 [details] [diff] [review]
patch v2: doesn't rename application.ini.in

No one in #build this morning had an environment suitable for testing patch v4 (attachment 617243 [details] [diff] [review]), so, in the interest of fixing nightly builds and reopening the tree, khuey approved and I landed patch v2 (attachment 617044 [details] [diff] [review]), which works around the build script issue.

http://hg.mozilla.org/mozilla-central/rev/97170e7a5f84


I then used the self-serve build API to trigger a rebuild of nightly builds.

https://tbpl.mozilla.org/?rev=97170e7a5f84


The Mac and Windows nightlies are expected to fail in one of the packaging steps that has the issue.  Once builds complete, I'll commit a whitespace change and trigger another round of nightlies through the API, which should succeed, after which we can reopen the tree.


In the long run, I still think the packaging steps should be made robust against the presence of an application.ini file in a subdirectory of the Firefox bindir.  But we can pursue that goal at our leisure once we solve the immediate problem of busted nightlies and a closed tree.

Comment 17

[Myk Melez [:myk] [@mykmelez]]

Empty commit (formerly known as whitespace change):

http://hg.mozilla.org/mozilla-central/rev/d3783d4eb1ea

Comment 18

[Myk Melez [:myk] [@mykmelez]]

Unfortunately, the retriggered nightly builds are still failing because of the horkage in the previous .mar:

========= Started set props: previous_buildid (results: 2, elapsed: 0 secs) (at 2012-04-21 16:51:07.908602) =========
python build/config/printconfigsetting.py 'build/obj-firefox/i386/previous/Contents/MacOS/application.ini
previous/Contents/MacOS/webapprt/application.ini' App BuildID
in dir /builds/slave/m-cen-osx64-ntly/. (timeout 1200 secs)

- https://tbpl.mozilla.org/php/getParsedLog.php?id=11098242&tree=Firefox&full=1

Comment 19

[Phil Ringnalda (:philor)]

Sadly, my recovery plan was obviously fatally flawed, but only obviously the second it failed: we download the previous complete.mar, not the previous build, so although we would now create a good and usable complete.mar, we can't actually get to the point of creating one until the latest-mozilla-central/firefox-14.0a1.en-US.win32.complete.mar and latest-mozilla-central/firefox-14.0a1.en-US.mac.complete.mar symlinks stop pointing to busted ones, and either point to nothing or to the last good one.

Comment 20

[Chris AtLee [:catlee]]

I just moved
firefox-14.0a1.en-US.mac.complete.mar
firefox-14.0a1.en-US.win32.complete.mar
firefox-14.0a1.en-US.win64-x86_64.complete.mar

from latest-mozilla-central into latest-mozilla-central/badmars

hopefully this will allow the next builds to succeed.

Comment 21

[Phil Ringnalda (:philor)]

Reopened m-c at 23:30 (having faith that Windows will be fine since Mac was).

profiling and ux are busted, and need to have their mac/win32/win64 mars moved aside, but so far I haven't seen any others busted, which must mean that way fewer than I would have guessed do updates.

Comment 22

[Phil Ringnalda (:philor)]

Comment on attachment 617243 [details] [diff] [review]
patch v4: fix another FindFile(application.ini)

The only thing I actually bring to the table is the ability to tell red from orange from green, so I think catlee's is the review you want.

Comment 23

[Nick Thomas [:nthomas] (UTC+12)]

(In reply to Phil Ringnalda (:philor) from comment #21)
> profiling and ux are busted, and need to have their mac/win32/win64 mars
> moved aside, but so far I haven't seen any others busted, which must mean
> that way fewer than I would have guessed do updates.

I've moved those mars away, but that may have been premature for ux until it merges the fix.

Comment 24

[Axel Hecht [:Pike]]

So today's mac and linx builds seem to pass, but they don't upload snippets, at least for the mac case, http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla-l10n/1335272781.1335272935.8457.gz&fulltext=1 says

======== BuildStep started ========
upload complete snippet skipped
=== Output ===
=== Output ended ===
======== BuildStep ended ========

Not seeing windows builds yet, I think.

Comment 25

[Axel Hecht [:Pike]]

CCing Alex and Lukas to track this for switching aurora updates back on after migration.

Comment 26

[John Hopkins (:jhopkins)]

I ran into a problem with FindFile returning > 1 result.  In my case, it was trying to find a tar.bz2 and since we had just did a version bump, it found a thunderbird-14 and thunderbird-15 file (a clobber will resolve this).

I suggest we augment FindFile with FindSingleFile and/or FindNewestFile.  FindSingleFile would assert(matches == 1) and thus fail on multiple results.  FindNewestFile would always return one match, even if there is more than one match to the file glob.

Comment 27

[Myk Melez [:myk] [@mykmelez]]

FindNewestFile wouldn't necessarily return the correct file in the case identified here of a subdirectory containing an application.ini file, and FindSingleFile would correctly fail in that case, but that wouldn't be much better than the current failure (more identifiable, perhaps, but still a failure to package a reasonable directory structure).  They might well be useful for other cases, however.

Comment 28

[Chris AtLee [:catlee]]

Comment on attachment 617243 [details] [diff] [review]
patch v4: fix another FindFile(application.ini)

Review of attachment 617243 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is just masking the problem.

What these steps are really trying to do is figure out what the buildid is. If we want to fix this for realz, then buildbot should call something like 'make echo-buildid' and use the output as the buildid. We shouldn't be guessing where application.ini is.

Comment 29

[Myk Melez [:myk] [@mykmelez]]

(In reply to Chris AtLee [:catlee] from comment #28)
> If we want to fix this for realz, then buildbot should call something like
> 'make echo-buildid' and use the output as the buildid.

We do want to fix this for realz, although our workaround suffices for now.


> We shouldn't be guessing where application.ini is.

I agree.  However, we're already doing so.  The patch on this bug would just make that guessing more robust.


But it's your prerogative whether or not to take it.  However, unfortunately I don't think I'll be able to fix the underlying issue in the foreseeable future, so I'm updating the Assignee field accordingly.

Comment 30

[Neustradamus]

Hello guys,

There is a problem with application.ini since Firefox/Thunderbird 11 and SeaMonkey 2.8.

Can you read : https://bugzilla.mozilla.org/show_bug.cgi?id=723493 and resolve this bug?

I would like to use new versions.