This target pulls the following properties after the MAR files have been generated. - completeMarHash - completeMarSize - buildid - appVersion These properties are later on used for CreateCompleteUpdateSnippets - http://hg.mozilla.org/build/buildbotcustom/file/tip/process/factory.py#l669 This target avoids using SetMozillaBuildProperties which avoids calling a LoggedRemoteCommand (http://mxr.mozilla.org/build/source/buildbotcustom/steps/misc.py#174) Regarding the patch, it pulls the buildid from dist/l10n-stage/firefox/Shiretoko.app/Contents/MacOS/application.ini which I believe is the en-US unpackaged. I am worried if this is the correct way since I would like to get the buildid from the locale's installer itself rather than the unpackaged en-US even if they *should* be the same. Am I being too much of a paranoic? I am working on another target that will avoid having to call CreateCompleteUpdateSnippets and create the snippet through the new target. Meaning that we might not want this patch after all but I wanted to file in case is worth anything for anyone.

We should really put the build ID into the ident target instead, reading application/platform.ini values fits there (and conveniently, comm-central apps already report the build_id there). We're safe in using l10n-stage (actually $(STAGEDIST) though, as actually, at the point we intend to call this, this _is_ the location the localized build is in. I'm somewhat unsure though if it's a good idea to retrieve properties of the localized MAR through the build system and not retrieve properties for the original MAR in the normal nightly factory through the same path in build system as well.

(In reply to comment #1) > We should really put the build ID into the ident target instead, reading > application/platform.ini values fits there (and conveniently, comm-central apps > already report the build_id there). > agreed > I'm somewhat unsure though if it's a good idea to retrieve properties of the > localized MAR through the build system and not retrieve properties for the > original MAR in the normal nightly factory through the same path in build > system as well. > I am not understanding what you are trying to say. Could you please say it in other words?

Not asking review for this patch. I am leaving it as reference. This is the make target mentioned to generate the snippet but I believe it is a little over killing plus it needs 3 parameters to be passed. I was suggested to move this into a python script.

PS: At the point where we call make ident, the stage contains the non-l10n stripped binaries as made by make unpack. So neither en-US nor l10n at that point. Using the make ident target to pass out the build id sounds fine to me. KaiRo's comment about the code path is that it'd be good to share the code that gets the build properties for the en-US builds and for the l10n builds. I'm not sure how good that argument is, as the l10n builds are in a different location on disk, so things are in a somewhat split anyway.

PPS: This bug should probably have dependencies/blocking set on other bugs, right?

The same as initial patch plus providing the "buildid" through make ident.

(In reply to comment #4) > KaiRo's comment about the code path is that it'd be good to share the code that > gets the build properties for the en-US builds and for the l10n builds. I'm not > sure how good that argument is, as the l10n builds are in a different location > on disk, so things are in a somewhat split anyway. My argument is that esp. the MAR properties should be retrieved the same way for en-US and L10n, and we have the files in the same place for that. What's right is that the extracted builds are in different locations, but the what we need to retrieve from there is basically only the build ID and that can easily just be added to the make ident output.

(In reply to comment #7) > My argument is that esp. the MAR properties should be retrieved the same way > for en-US and L10n, and we have the files in the same place for that. > We will have to wait for bhearsum to be back since he wrote the code for en-US and it is a little trickier since it is used for releases as well. To have in mind that the buildbot class that pulls the properties for en-US pulls many more things than what we need for creation of snippets for locales. > What's right is that the extracted builds are in different locations, but the what we > need to retrieve from there is basically only the build ID and that can easily > just be added to the make ident output. I added the buildid to the make ident target

(In reply to comment #8) > I added the buildid to the make ident target But you forgot to remove it from the other one, actually ;-)

Comment on attachment 380157 [details] [diff] [review] echo properties needed by CreateCompleteUpdateSnippet Does the existing identToProperties function in process/factory.py still work after this change? That would be my only reservation here.

(In reply to comment #10) > (From update of attachment 380157 [details] [diff] [review]) > Does the existing identToProperties function in process/factory.py still work > after this change? This patch has to go in and the same time. It would have not worked without this patch. Already being in use by Mobile: http://mxr.mozilla.org/build/source/buildbotcustom/process/factory.py#3517 http://mxr.mozilla.org/mobile-browser/source/locales/Makefile.in#93

Comment on attachment 380157 [details] [diff] [review] echo properties needed by CreateCompleteUpdateSnippet r-, this needs to use printf, see the corresponding suite bug. buildid should be set only once. Why do we actually have to do this? This looks like we're round-tripping data master-side that could be just done on the slave. I could picture a rather simple python script in the build tools repo.

Comment on attachment 380464 [details] [diff] [review] pull all variables from stdout This shouldn't be necessary, http://hg.mozilla.org/build/buildbotcustom/file/a58e43b245a6/process/factory.py#l908 checks if the output is just a single revision string, and if not, does the rich parsing.

(In reply to comment #12) > (From update of attachment 380157 [details] [diff] [review]) > r-, this needs to use printf, see the corresponding suite bug. > > buildid should be set only once. > > Why do we actually have to do this? This looks like we're round-tripping data > master-side that could be just done on the slave. I could picture a rather > simple python script in the build tools repo. > You are right. I will need a little help to write that simple python script. The make target from attachment 380135 [details] was kind of trying to do that. I actually gave up on the idea just because I don't know python and thought I would be delaying the goal. Could we file then another bug to write that python script? I would like to continue working on porting the current factories into the property driven factory. Sounds fair?

(In reply to comment #8) > (In reply to comment #7) > > My argument is that esp. the MAR properties should be retrieved the same way > > for en-US and L10n, and we have the files in the same place for that. > > > We will have to wait for bhearsum to be back since he wrote the code for en-US > and it is a little trickier since it is used for releases as well. To have in > mind that the buildbot class that pulls the properties for en-US pulls many > more things than what we need for creation of snippets for locales. > I completely agree with KaiRo that we should do the same thing for en-US and l10n. If we're going to retrieve the properties in a different way for the l10n nightlies, we need to switch en-US over to that system too. I don't care much one way or the other as long as we have consistency.

I am going to be working on small python script to generate the snippet by the slave. Sounds good to everyone? Anyone to help me test for the en-US scenario once I have it?

The python script is now under this bug 496196 WONTFIXING this one