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

RESOLVED INCOMPLETE

Status

Release Engineering
General Automation
P2
normal
RESOLVED INCOMPLETE
6 years ago
2 years ago

People

(Reporter: catlee, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [l10n], [Desktop WebRT])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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.
Blocks: 746156
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.
(Reporter)

Comment 2

6 years ago
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

6 years ago
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.
(Reporter)

Comment 4

6 years ago
(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).
(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` ?
(Reporter)

Comment 6

6 years ago
(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
Created attachment 617032 [details] [diff] [review]
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.
Attachment #617032 - Flags: review?(benjamin)

Comment 8

6 years ago
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.
Attachment #617032 - Flags: review?(benjamin) → review+
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.
Created attachment 617044 [details] [diff] [review]
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?
Attachment #617032 - Attachment is obsolete: true
Attachment #617044 - Flags: review+
(Reporter)

Updated

6 years ago
Priority: -- → P2
Whiteboard: [l10n]
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?
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.
Severity: normal → blocker
Priority: P2 → P1
Summary: win32 l10n nightlies busted on mozilla-central → win32 l10n nightlies and win32 and 10.7 en-US update generation busted on mozilla-central
Created attachment 617236 [details] [diff] [review]
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.
Assignee: nobody → myk
Status: NEW → ASSIGNED
Attachment #617236 - Flags: review?(philringnalda)
Attachment #617236 - Flags: review?(catlee)
Created attachment 617243 [details] [diff] [review]
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.
Attachment #617236 - Attachment is obsolete: true
Attachment #617236 - Flags: review?(philringnalda)
Attachment #617236 - Flags: review?(catlee)
Attachment #617243 - Flags: review?(philringnalda)
Attachment #617243 - Flags: review?(catlee)
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.
Attachment #617044 - Flags: checked-in+
Empty commit (formerly known as whitespace change):

http://hg.mozilla.org/mozilla-central/rev/d3783d4eb1ea
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
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.

Updated

6 years ago
Blocks: 731054
(Reporter)

Comment 20

6 years ago
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.
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.
Severity: blocker → normal
Priority: P1 → P2
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.
Attachment #617243 - Flags: review?(philringnalda)
(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

6 years ago
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

6 years ago
CCing Alex and Lukas to track this for switching aurora updates back on after migration.
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.

Updated

6 years ago
Whiteboard: [l10n] → [l10n], [marketplace-beta-]
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.
(Reporter)

Comment 28

6 years ago
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.
Attachment #617243 - Flags: review?(catlee) → review-

Updated

6 years ago
No longer blocks: 731054

Updated

6 years ago
Whiteboard: [l10n], [marketplace-beta-] → [l10n], [Desktop WebRT]
(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.
Assignee: myk → nobody
Status: ASSIGNED → NEW
Summary: win32 l10n nightlies and win32 and 10.7 en-US update generation busted on mozilla-central → automation shouldn't fail when there's more than one application.ini in dist/

Comment 30

5 years ago
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.
(Assignee)

Updated

5 years ago
Product: mozilla.org → Release Engineering
(Reporter)

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.