1008239 - [dolphin] ./build.sh gecko-update-fota and gecko-update-fota-full didn't work

Status: RESOLVED FIXED

(Firefox OS Graveyard :: GonkIntegration, defect)

Description

[Kai-Zhen Li [:seinlin][:kli]]

Can't build update package for dolphin and error message is:
--
Install system fs image: out/target/product/scx15_sp7715ga/system.img
Using yaffs2 filesystem
Extracting partitions from recovery fstab
Mounting /system from 
Mounting /data   from 
Generating FOTA update package
mkdir -p `dirname out/target/product/scx15_sp7715ga/fota/partial/update.zip` || true
usage: build-flash-fota.py [options]
build-flash-fota.py: error: argument --system-location: expected one argument
make: *** [out/target/product/scx15_sp7715ga/fota/partial/update.zip] Error 2

Comment 1

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: PR for master

Dave, could you help me review this patch? Thanks!

Comment 2

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: PR for v1.4

Comment 3

[Dave Hylands [:dhylands]]

Can you provide the recovery.fstab for a device using ubifs?

I can't really review this properly without seeing what that file looks like.

Comment 4

[Dave Hylands [:dhylands]]

And how does bug 1007570 impact this one?

Comment 5

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: fstab.scx15

Hi Dave, now only Dolphin use nand flash with ubifs. Its fstab is attached. Thanks!

Comment 6

[Kai-Zhen Li [:seinlin][:kli]]

(In reply to Dave Hylands [:dhylands] (away - back May 16) from comment #4)
> And how does bug 1007570 impact this one?

I think it's different to bug 1007570.

Comment 7

[Gabriele Svelto [:gsvelto] (PTO)]

I think you will need bug 1007570 because attachment 8422147 [details] looks like a Linux fstab file.

Comment 8

[Dave Hylands [:dhylands]]

My take on this is that bug 1007570 should solve the problem that this bug is trying to solve, but IMHO it does it in a cleaner fashion.

Comment 9

[Kai-Zhen Li [:seinlin][:kli]]

Dave and Gabriele, Thanks! But we still need to have ubifs for B2G_FOTA_FSTYPE. I will update the PR based on bug 1007570 and have ubifs supported.

Comment 10

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: bug-1008239-set-correct-fstab.diff

Sam, could you review and merge this patch?

Comment 11

[Kai-Zhen Li [:seinlin][:kli]]

Dave, I updated PR, after Sam merge the patch into their repo, ./build.sh gecko-update-fota and gecko-update-fota-full can work properly for ubifs.

Comment 12

[Dave Hylands [:dhylands]]

Comment on attachment 8420169 [details] [review]
PR for master

I like this much better.

Comment 13

[Carsten Book [:Tomcat]]

master: https://github.com/mozilla-b2g/gonk-misc/commit/996b5c6a2fd2b8a0124c0eab80eb72a4daece7bc

Comment 14

[Ryan VanderMeulen [:RyanVM]]

v1.4: https://github.com/mozilla-b2g/gonk-misc/commit/81e894545b87ea2cf9e78f56a2cd1d2a5adc7a25

Comment 15

[Kai-Zhen Li [:seinlin][:kli]]

Lianxiang, 

Per our offline discussion,  
Using 'device/sprd/scx15/nand/fstab.scx15' will casue error in recovery ROM.
Need to use 'device/sprd/scx15/nand/recovery.fstab' for dolphin.
But the current format of recovery.fstab may cause an error in build fota update.mar.

Can you help to update 'device/sprd/scx15/nand/recovery.fstab' in this format?
---
/dev/block/platform/../../by-name/system          /system     ubifs defaults  defaults
...

Here is also a reference
https://github.com/Seinlin/device-mako/blob/b2g-4.4.2_r1/fstab.mako

Comment 16

[Kai-Zhen Li [:seinlin][:kli]]

Comment on attachment 8423797 [details] [diff] [review]
bug-1008239-set-correct-fstab.diff

Obsolete this as it will cause error in recovery ROM.

Comment 17

[Zhou Lianxiang]

(In reply to Kai-Zhen Li from comment #15)
> Lianxiang, 
> 
> Per our offline discussion,  
> Using 'device/sprd/scx15/nand/fstab.scx15' will casue error in recovery ROM.
> Need to use 'device/sprd/scx15/nand/recovery.fstab' for dolphin.
> But the current format of recovery.fstab may cause an error in build fota
> update.mar.
> 
> Can you help to update 'device/sprd/scx15/nand/recovery.fstab' in this
> format?
> ---
> /dev/block/platform/../../by-name/system          /system     ubifs defaults
> defaults
> ...
> 
> Here is also a reference
> https://github.com/Seinlin/device-mako/blob/b2g-4.4.2_r1/fstab.mako

On our device, we use ubifs, I can not find any "../by-name/system" in /dev folder.
why not change the Android.mk file to parse the 'device/sprd/scx15/nand/recovery.fstab'?

diff --git a/Android.mk b/Android.mk
index c169929..15a8988 100644
--- a/Android.mk
+++ b/Android.mk
@@ -381,8 +381,8 @@ define detect-partitions
     )
 
     $(if $(filter recovery, $(FSTAB_TYPE)),
-      $(eval B2G_FOTA_SYSTEM_PARTITION := $(shell grep -v '^\#' $(1) | grep '^/system\s\+' | awk '{ print $$3 }'))
-      $(eval B2G_FOTA_DATA_PARTITION := $(shell grep -v '^\#' $(1) | grep '^/data\s\+' | awk '{ print $$3 }'))
+      $(eval B2G_FOTA_SYSTEM_PARTITION := $(shell grep -v '^\#' $(1) | perl -ne 'print $$1 if /^(\w+)\s+\/system\s+/ or /^\/system\s+\w+\s+(\w+)/'))
+      $(eval B2G_FOTA_DATA_PARTITION := $(shell grep -v '^\#' $(1) | perl -ne 'print $$1 if /^(\w+)\s+\/data\s+/ or /^\/data\s+\w+\s+(\w+)/'))
     ),
 
     $(if $(filter ext%, $(B2G_FOTA_FSTYPE)),

Comment 18

[James Zhang (Spreadtrum)]

Any progress here, can we use Lianxiang's patch?

Comment 19

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: recovery.fstab

For dolphin, it needs to use the attached recovery.fstab in order to make the recovery ROM work properly. This file looks like a linux fstab, but it can be recognized by makefile.

Comment 20

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: Do more check in Makefile

Dave, do you think it is reasonable to do one more check for the fstab in comment 19?

Comment 21

[Kai-Zhen Li [:seinlin][:kli]]

(In reply to Kai-Zhen Li from comment #20)
> Created attachment 8428563 [details] [diff] [review]
> Do more check in Makefile
> 
> Dave, do you think it is reasonable to do one more check for the fstab in
> comment 19?

Probably, I think we could also simply this check as below.

   $(if $(FSTAB_FILE),
     $(eval STARTS_WITH_DEV := $(shell grep '^/dev/' $(1) | grep '/system'))
-    $(if $(filter /system, $(STARTS_WITH_DEV)),
+    $(eval STARTS_WITH_NAME := $(shell grep -v '^\#' $(1) | grep -v '^/' | grep '/system'))
+    $(if $(filter /system, $(STARTS_WITH_DEV) $(STARTS_WITH_NAME)),
       $(eval FSTAB_TYPE := linux))

Comment 22

[Dave Hylands [:dhylands]]

Comment on attachment 8428563 [details] [diff] [review]
Do more check in Makefile

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

So why are you storing this into a variable called STARTS_WITH_DEV ?

None of the lines that you're matching start with dev.

I think that we really need examples in the comments of the various style of fstab that are being parsed here.

We seem to have 3 variants:

1 - style that starts with /dev:

/dev/block/platform/msm_sdcc.1/by-name/system /system ext4 ro,barrier=1 wait

2 - android style?:

/system	yaffs2	system

3 - This new style:

system /system	ubifs defaults defaults

Interestingly, all 3 styles share the attribute that the file system comes in the word after the word with /system (as a standalone word) in it.

Comment 23

[Kai-Zhen Li [:seinlin][:kli]]

Yes we need to handle 3 styles. 
attachment 8428563 [details] [diff] [review] is my WIP, just to verify if my idea does work. STARTS_WITH_DEV is used only one time, that's why I try to reuse it. 
But this is confused, so I got another solution in comment 21. 
Finally, I think the code could be as below where style 1 and 3 will be recognized as linux format.

---
   $(if $(FSTAB_FILE),
     $(eval STARTS_WITH_DEV := $(shell grep '^/dev/' $(1) | grep '/system'))
     $(eval STARTS_WITH_NAME := $(shell grep -v '^\#' $(1) | grep -v '^/' | grep '/system'))
     $(if $(filter /system, $(STARTS_WITH_DEV) $(STARTS_WITH_NAME)),
       $(eval FSTAB_TYPE := linux))
---

Dave, do you think this is reasonable?

Comment 24

[Dave Hylands [:dhylands]]

That looks better :)

I'd still like to see a comment with the 3 different styles go into Android.mk

Comment 25

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: https://github.com/mozilla-b2g/gonk-misc/pull/172

Dave, Comment is also updated. Could you review this patch? Thanks!

Comment 26

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: Pull request for v1.4

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gonk-misc/commit/531cf670e485649c69746e46d567929fcd54cbc5
v1.4:   https://github.com/mozilla-b2g/gonk-misc/commit/e1dbe8086e0c4663a4e8a45e1b4f6048f455a925

Comment 28

[James Zhang (Spreadtrum)]

Reopen.
As talked in email, we should add tools/update-tools/bin/gonk/update-binary ubifs support.

Comment 29

[Kai-Zhen Li [:seinlin][:kli]]

Dave, 

Now update.mar for dolphin can be built but failed to update and the root cause is update-binary can't mount ubifs partition.

I think there could be 2 possible solutions,

1. Update tools/update-tools/bin/gonk/update-binary to the one which can support ubifs.

2. Able to define the update-binary used for update.zip in build time. 
   Such as, when a device exports a path of update-binary, tools/update-tools/update_tools.py will use if for update.zip.

Do you think which solution is more reasonable?

Comment 30

[Jason Smith [:jsmith]]

Not a vendor bug. This is a gonk bug.

Comment 31

[Dave Hylands [:dhylands]]

(In reply to Kai-Zhen Li from comment #29)
> Dave, 
> 
> Now update.mar for dolphin can be built but failed to update and the root
> cause is update-binary can't mount ubifs partition.
> 
> I think there could be 2 possible solutions,
> 
> 1. Update tools/update-tools/bin/gonk/update-binary to the one which can
> support ubifs.
> 
> 2. Able to define the update-binary used for update.zip in build time. 
>    Such as, when a device exports a path of update-binary,
> tools/update-tools/update_tools.py will use if for update.zip.
> 
> Do you think which solution is more reasonable?

So where is the source for the update-binary?

If the source is in a repo that we control, then I think that 1 makes sense. If the source is not in a repo that we control then I think that 2 is probably more appropriate. Although I tend to favor using command line arguments over environment variables, unless the environment variable is already part of the build.

Comment 32

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: add an option to specify update-binary

Comment 33

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: add a new option '--update-bin' for build-tools.

Comment 34

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: Pull request for v1.4

Comment 35

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: A-sample-to-specify-update-binary-for-update.zip.patch

Comment 36

[Dave Hylands [:dhylands]]

Comment on attachment 8435758 [details] [review]
add an option to specify update-binary

I think that the calculated path for the prebuilt update-binary is wrong.

If I'm wrong, then r+, but otherwise I think that this change would break existing usage.

Comment 37

[Dave Hylands [:dhylands]]

Comment on attachment 8435759 [details] [review]
add a new option '--update-bin' for build-tools.

I added a comment on the PR about using wildcard.

Comment 38

[Kai-Zhen Li [:seinlin][:kli]]

(In reply to Dave Hylands [:dhylands] from comment #36)
> Comment on attachment 8435758 [details] [review]
> add an option to specify update-binary
> 
> I think that the calculated path for the prebuilt update-binary is wrong.
> 
> If I'm wrong, then r+, but otherwise I think that this change would break
> existing usage.

I verified when --update-bin is not specified, the pre-built one will be used, 'tools/update-tools/bin/gonk/update-binary'. So this won't break existing usage.

Comment 39

[Kai-Zhen Li [:seinlin][:kli]]

(In reply to Dave Hylands [:dhylands] from comment #37)
> Comment on attachment 8435759 [details] [review]
> add a new option '--update-bin' for build-tools.
> 
> I added a comment on the PR about using wildcard.

PR is updated, I verified it does work properly with/without TARGET_UPDATE_BINARY.

Comment 40

[Kai-Zhen Li [:seinlin][:kli]]

Comment on attachment 8435758 [details] [review]
add an option to specify update-binary

I added comment in PR, could you review again? Thanks!

Comment 41

[Dave Hylands [:dhylands]]

Comment on attachment 8435758 [details] [review]
add an option to specify update-binary

I'm happy as long as you tested it.

Thanks.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

In the future, can we please do follow-up work in new bugs? Tracking becomes a real pain when there are multiple landings over a week's time.

B2G/master: https://github.com/mozilla-b2g/B2G/commit/b5a208385d0b85531e8f355267b40e7b77966ad1

gonk-misc/master: https://github.com/mozilla-b2g/gonk-misc/commit/dbb66e540194a187326cece28ae0b51cdd500184
gonk-misc/v1.4:   https://github.com/mozilla-b2g/gonk-misc/commit/2522872ebf00b768e517c6f18d63a583fd5f5baf

Comment 43

[Kai-Zhen Li [:seinlin][:kli]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #42)
> In the future, can we please do follow-up work in new bugs? Tracking becomes
> a real pain when there are multiple landings over a week's time.
> 

Ryan, Thanks! We should do as you suggested in the future.

Comment 44

[Kai-Zhen Li [:seinlin][:kli]]

(In reply to Kai-Zhen Li from comment #35)
> Created attachment 8435767 [details] [diff] [review]
> A-sample-to-specify-update-binary-for-update.zip.patch

James, this is an example how device specify update-binary for update.zip. Please do not land this patch as I didn't test it. The name of the binary file is not limited to 'update-binary'. Could you help to add one for dolphin? Thanks!

Comment 45

[James Zhang (Spreadtrum)]

But how to build the update-binary?
Can you give us the origin source code? Lianxiang said this binray which is built on my side can't work fine.

Comment 46

[Kai-Zhen Li [:seinlin][:kli]]

(In reply to James Zhang from comment #45)
> But how to build the update-binary?
> Can you give us the origin source code? Lianxiang said this binray which is
> built on my side can't work fine.

The original source is
https://android.googlesource.com/platform/bootable/recovery.git

In dolphin, The source code is under "bootable/recovery/updater" and it can be built by.
$ ./build.sh updater