Closed Bug 708437 Opened 8 years ago Closed 8 years ago

Fennec build race condition: res/values/strings.xml:1: error: Error parsing XML: no element found

Categories

(Firefox for Android :: General, defect, P1)

ARM
Android
defect

Tracking

()

RESOLVED WORKSFORME
Tracking Status
fennec 11+ ---

People

(Reporter: cpeterson, Assigned: glandium)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached file build log
When I build Fennec with MOZ_MAKE_FLAGS="-j2" (or more), I get intermittent build errors about parsing strings.xml. The build problem "goes away" if I use MOZ_MAKE_FLAGS="-j1". This implies the problem may be a makefile race condition.

The generated strings.xml file does exist at obj-android/mobile/android/base/res/values/strings.xml.
Chris, this is untested as it's late here, but I think this should do the trick.

As you can reproduce it, can you test this patch? Thanks.
Assignee: nobody → l10n
Status: NEW → ASSIGNED
Pike: The patch seems to fix my problem. I've tried a couple rebuilds with make -j16 and I have not be able to reproduce the build errors.
Priority: -- → P1
pike, the patch look fine.  did you want to ask someone for a review?  we should get this in soon.
Comment on attachment 579864 [details] [diff] [review]
create an explicit dependency for res/values/strings.xml

Tested locally, too.

Can't come up with a better victim for reviews than Brad.
Attachment #579864 - Flags: review?(blassey.bugs)
It looks like we're not guaranteeing that we finish in our subdirs before we start doing stuff in the current directory.  That seems ... broken.  Ted?
Comment on attachment 579864 [details] [diff] [review]
create an explicit dependency for res/values/strings.xml

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

this looks to be broken in a way that I don't understand. Gonna wait until ted weighs in before reviewing
Blocks: 708015
Comment on attachment 579864 [details] [diff] [review]
create an explicit dependency for res/values/strings.xml

Putting an additional review request on Ted, too.
Attachment #579864 - Flags: review?(ted.mielczarek)
Comment on attachment 579864 [details] [diff] [review]
create an explicit dependency for res/values/strings.xml

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

this looks to be broken in a way that I don't understand. Gonna wait until ted weighs in before reviewing
sorry, hit refresh
Comment on attachment 579864 [details] [diff] [review]
create an explicit dependency for res/values/strings.xml

gonna just let ted handle this
Attachment #579864 - Flags: review?(blassey.bugs)
I am getting this build error again, even with Pike's patch applied.
I think this should work (at least it does for me)
Attachment #582869 - Flags: review?(ted.mielczarek)
Comment on attachment 582869 [details] [diff] [review]
Avoid race condition (res/values/strings.xml:1: error: Error parsing XML: no element found)

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

::: mobile/android/base/locales/Makefile.in
@@ +55,5 @@
>  endif
>  
>  DEFINES += -DAB_CD=$(AB_CD)
>  
> +export realchrome:: ../res/values/strings.xml ;

Is that semicolon necessary for anything? Seems extraneous.
Attachment #582869 - Flags: review?(ted.mielczarek) → review+
Ted's patch fixes my build problem.
The ; is for an empty rule.
You don't need a rule there, do you? Just having the dependency specified should be enough.
Attachment #579864 - Attachment is obsolete: true
Attachment #579864 - Flags: review?(ted.mielczarek)
I only found http://www.gnu.org/software/make/manual/make.html#Empty-Recipes, which mentions the ; as the way to do an empty recipe. I didn't whack up a test case to verify if a rule with just dependencies but no commands for the recipe falls into that category.
From that link: "You may be inclined to define empty recipes for targets that are not actual files, but only exist so that their prerequisites can be remade. However, this is not the best way to do that, because the prerequisites may not be remade properly if the target file actually does exist. See Phony Targets, for a better way to do this. "

see also: http://www.gnu.org/software/make/manual/make.html#Multiple-Targets
Comment on attachment 582869 [details] [diff] [review]
Avoid race condition (res/values/strings.xml:1: error: Error parsing XML: no element found)

I think this failed for me. I tried to port this to embedding/android, and it was funky.
For one, I couldn't et 
export realchrome:: ... 

to work, but just

export:: ../res/values/strings.xml
realchrome:: ../res/values/strings.xml

And then it failed with

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/axel@mozilla.com-4f2868c689aa/try-android-xul/try-android-xul-build426.txt.gz

which fails because strings.xml needs brand.dtd, which is only shoveled to dist by the jar.mn in branding, which is executed through libs and realchrome. Which is post-export.
The problem in embedding/android is quite different, which is why porting the same fix doesn't quite work. Is there a specific reason the branding files are taken from dist/bin instead of the source directory?
(In reply to Mike Hommey [:glandium] from comment #21)
> The problem in embedding/android is quite different, which is why porting
> the same fix doesn't quite work. Is there a specific reason the branding
> files are taken from dist/bin instead of the source directory?

We were doing it that way before /mobile/xul and /mobile/android/ so when we copied and split into those folders, the branding code came along "as is".
(In reply to Mike Hommey [:glandium] from comment #20)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/fe434c1887f5

Backed out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/63065b56fed6

Since after a clobber:
https://tbpl.mozilla.org/php/getParsedLog.php?id=8058945&tree=Mozilla-Inbound
{
make[6]: *** No rule to make target `res/values/strings.xml', needed by `R.java'.  Stop.
}
Whiteboard: [inbound]
PS: I found out the build problem with xul, it's a different order of branding and R.java, see bug 708015 comment 12
OS: Mac OS X → Android
Hardware: All → ARM
Comment on attachment 582869 [details] [diff] [review]
Avoid race condition (res/values/strings.xml:1: error: Error parsing XML: no element found)

This patch got backed out due to build bustage, setting an r- on this and obsoleting.
Attachment #582869 - Attachment is obsolete: true
Attachment #582869 - Flags: review-
Comment on attachment 579864 [details] [diff] [review]
create an explicit dependency for res/values/strings.xml

Resurrecting this patch instead.
Attachment #579864 - Attachment is obsolete: false
Attachment #579864 - Flags: review?(ted.mielczarek)
is anyone still seeing this? I suspect bug 712380 may have fixed it
WORKSFORME
tracking-fennec: --- → 11+
Attachment #579864 - Flags: review?(ted.mielczarek) → review-
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.