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

Status: RESOLVED WORKSFORME

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

Description

[Chris Peterson [:cpeterson]]

Attachment: 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.

Comment 1

[Axel Hecht [:Pike]]

Attachment: create an explicit dependency for 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.

Comment 2

[Chris Peterson [:cpeterson]]

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.

Comment 3

[Doug Turner (:dougt)]

pike, the patch look fine.  did you want to ask someone for a review?  we should get this in soon.

Comment 4

[Axel Hecht [:Pike]]

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.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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 6

[Brad Lassey [:blassey] (use needinfo?)]

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

Comment 7

[Axel Hecht [:Pike]]

Comment on attachment 579864 [details] [diff] [review]
create an explicit dependency for res/values/strings.xml

Putting an additional review request on Ted, too.

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

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

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

sorry, hit refresh

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 579864 [details] [diff] [review]
create an explicit dependency for res/values/strings.xml

gonna just let ted handle this

Comment 11

[Chris Peterson [:cpeterson]]

I am getting this build error again, even with Pike's patch applied.

Comment 12

[Mike Hommey [:glandium]]

Attachment: Avoid race condition (res/values/strings.xml:1: error: Error parsing XML: no element found)

I think this should work (at least it does for me)

Comment 13

[(not currently active) 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.

Comment 14

[Chris Peterson [:cpeterson]]

Ted's patch fixes my build problem.

Comment 15

[Axel Hecht [:Pike]]

The ; is for an empty rule.

Comment 16

[(not currently active) Ted Mielczarek]

You don't need a rule there, do you? Just having the dependency specified should be enough.

Comment 17

[Axel Hecht [:Pike]]

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.

Comment 18

[(not currently active) Ted Mielczarek]

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 19

[Axel Hecht [:Pike]]

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/[e-mail]-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.

Comment 20

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe434c1887f5

Comment 21

[Mike Hommey [:glandium]]

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?

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

(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".

Comment 23

[Ed Morley [:emorley]]

(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.
}

Comment 24

[Axel Hecht [:Pike]]

PS: I found out the build problem with xul, it's a different order of branding and R.java, see bug 708015 comment 12

Comment 25

[Axel Hecht [:Pike]]

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.

Comment 26

[Axel Hecht [:Pike]]

Comment on attachment 579864 [details] [diff] [review]
create an explicit dependency for res/values/strings.xml

Resurrecting this patch instead.

Comment 27

[Brad Lassey [:blassey] (use needinfo?)]

is anyone still seeing this? I suspect bug 712380 may have fixed it

Comment 28

[Chris Peterson [:cpeterson]]

WORKSFORME