mach build needs to create partial mars so it can upload and do sendchange before finishing

RESOLVED FIXED

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jlund, Assigned: mshal)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(firefox34 fixed, firefox35 fixed, firefox-esr31 fixed, b2g-v2.0 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

consists of ripping out partial gen logic from mozharness and adding it into mach build
Posted patch partial.patch (obsolete) — Splinter Review
This adds partial mar generation to the build. It's a temporary hack until funsize is ready, then it can be ripped out again.
Assignee: nobody → mshal
Attachment #8509920 - Flags: review?(mh+mozilla)
Comment on attachment 8509920 [details] [diff] [review]
partial.patch

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

::: build/gen_mach_buildprops.py
@@ +28,5 @@
>          pass
>  
>      return (sha1Hash, size)
>  
> +def getMarProperties(filename, martype):

I'd rather have a boolean flag like partial=False and have this function choose between 'complete' and 'partial' instead of relying on the caller getting it right.

@@ +88,5 @@
>      args = parser.parse_args()
>  
> +    json_data = getMarProperties(args.complete_mar_file, 'complete')
> +    if args.partial_mar_file:
> +        json_data.update(getMarProperties(args.partial_mar_file, 'partial'))

So, these calls would become
  getMarProperties(args.complete_mar_file)
and
  getMarProperties(args.partial_mar_file, partial=True)

::: tools/update-packaging/Makefile.in
@@ +99,5 @@
> +	  "$(STAGE_DIR)/$(PKG_UPDATE_BASENAME).partial.$${SRC_BUILD_ID}-$${DST_BUILD_ID}.mar" previous current && \
> +	$(if $(MOZ_SIGN_CMD),$(MOZ_SIGN_CMD) -f mar "$(STAGE_DIR)/$(PKG_UPDATE_BASENAME).partial.$${SRC_BUILD_ID}-$${DST_BUILD_ID}.mar" &&) \
> +	rm -f $(STAGE_DIR)/previous.mar; \
> +	else echo "No previous MAR found; not creating a partial MAR"; \
> +	fi

Where can I look for the original code for that?
Flags: needinfo?(mshal)
Comment on attachment 8509920 [details] [diff] [review]
partial.patch

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

::: tools/update-packaging/Makefile.in
@@ +80,5 @@
> +
> +automation-partial-patch: complete-patch
> +	rm -rf current current.work previous
> +	mkdir current previous
> +	latestmar=$$(ssh -l $(UPLOAD_USER) -i $(UPLOAD_SSH_KEY) $(UPLOAD_HOST) ls -1t $(LATEST_MAR_DIR) | grep $(MOZ_PKG_PLATFORM).complete.mar$$ | head -n 1); \

You want quotes around ls -1t through head -n 1, to make the whole command run on the upload host, instead of retrieving the whole list and then grepping locally.

@@ +82,5 @@
> +	rm -rf current current.work previous
> +	mkdir current previous
> +	latestmar=$$(ssh -l $(UPLOAD_USER) -i $(UPLOAD_SSH_KEY) $(UPLOAD_HOST) ls -1t $(LATEST_MAR_DIR) | grep $(MOZ_PKG_PLATFORM).complete.mar$$ | head -n 1); \
> +	  if test -n "$$latestmar"; then \
> +	  wget -O $(STAGE_DIR)/previous.mar --no-check-certificate http://$(UPLOAD_HOST)/$(LATEST_MAR_DIR)/$$latestmar && \

I know this comes from the current code, but that --no-check-certificate rubs me the wrong way.

@@ +84,5 @@
> +	latestmar=$$(ssh -l $(UPLOAD_USER) -i $(UPLOAD_SSH_KEY) $(UPLOAD_HOST) ls -1t $(LATEST_MAR_DIR) | grep $(MOZ_PKG_PLATFORM).complete.mar$$ | head -n 1); \
> +	  if test -n "$$latestmar"; then \
> +	  wget -O $(STAGE_DIR)/previous.mar --no-check-certificate http://$(UPLOAD_HOST)/$(LATEST_MAR_DIR)/$$latestmar && \
> +	(cd previous; \
> +	  MAR=$(LIBXUL_DIST)/host/bin/mar$(HOST_BIN_SUFFIX) perl $(topsrcdir)/tools/update-packaging/unwrap_full_update.pl '../$(STAGE_DIR)/previous.mar') && \

MAR=$(MAR_BIN)

@@ +88,5 @@
> +	  MAR=$(LIBXUL_DIST)/host/bin/mar$(HOST_BIN_SUFFIX) perl $(topsrcdir)/tools/update-packaging/unwrap_full_update.pl '../$(STAGE_DIR)/previous.mar') && \
> +	(cd current; \
> +	  MAR=$(MAR_BIN) perl $(topsrcdir)/tools/update-packaging/unwrap_full_update.pl '../$(DIST)/$(COMPLETE_MAR)') && \
> +	find current -name \*.pgc -print -delete && \
> +	find previous -name \*.pgc -print -delete && \

Both current and previous are first removed and unpacked. How can we end up with .pgc files there?

@@ +96,5 @@
> +	MAR=$(MAR_BIN) \
> +	MBSDIFF=$(MBSDIFF_BIN) \
> +	  $(srcdir)/make_incremental_update.sh \
> +	  "$(STAGE_DIR)/$(PKG_UPDATE_BASENAME).partial.$${SRC_BUILD_ID}-$${DST_BUILD_ID}.mar" previous current && \
> +	$(if $(MOZ_SIGN_CMD),$(MOZ_SIGN_CMD) -f mar "$(STAGE_DIR)/$(PKG_UPDATE_BASENAME).partial.$${SRC_BUILD_ID}-$${DST_BUILD_ID}.mar" &&) \

Why not invoke $(MAKE) partial-patch with the right values for SRC_BUILD and DST_BUILD?
Attachment #8509920 - Flags: review?(mh+mozilla) → feedback+
(In reply to Mike Hommey [:glandium] from comment #4)
> > +	  wget -O $(STAGE_DIR)/previous.mar --no-check-certificate http://$(UPLOAD_HOST)/$(LATEST_MAR_DIR)/$$latestmar && \
> 
> I know this comes from the current code, but that --no-check-certificate
> rubs me the wrong way.

Ah, old code, that does no good with http:// anyway. We can't switch to https://ftp-ssl just yet, the wget on windows doesn't have a cert bundle with the CA in it.
Posted patch partial.patch (obsolete) — Splinter Review
I'm not sure about the .pgc thing - looks like it was a workaround for bug 783784 / bug 785748. I don't see any pgc files in the tests that I've run, so maybe it's not needed anymore? This patch still has it, though all other review comments should be addressed.
Attachment #8509920 - Attachment is obsolete: true
Attachment #8510640 - Flags: review?(mh+mozilla)
Comment on attachment 8510640 [details] [diff] [review]
partial.patch

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

::: tools/update-packaging/Makefile.in
@@ +82,5 @@
> +	rm -rf current current.work previous
> +	mkdir current previous
> +	latestmar=$$(ssh -l $(UPLOAD_USER) -i $(UPLOAD_SSH_KEY) $(UPLOAD_HOST) 'ls -1t $(LATEST_MAR_DIR) | grep $(MOZ_PKG_PLATFORM).complete.mar$$ | head -n 1'); \
> +	  if test -n "$$latestmar"; then \
> +	  wget -O $(STAGE_DIR)/previous.mar http://$(UPLOAD_HOST)/$(LATEST_MAR_DIR)/$$latestmar && \

either do this now or in a followup bug, but it would be good to have the fact that it's downloading from http instead of https from something hardcoded in the tree. IOW, it would be better to allow switching to https without having to land something.

@@ +96,5 @@
> +	SRC_BUILD=previous DST_BUILD=current \
> +	  $(MAKE) partial-patch && \
> +	rm -f $(STAGE_DIR)/previous.mar; \
> +	else echo "No previous MAR found; not creating a partial MAR"; \
> +	fi

This would be nicer with proper indentation in the shell script.
Attachment #8510640 - Flags: review?(mh+mozilla) → review+
Posted patch partial.patchSplinter Review
Includes indentation fix, plus a one-liner addition to set partialMarUrl that I missed. r+ carried forward
Attachment #8510640 - Attachment is obsolete: true
Attachment #8511359 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/44aadcfb5f57
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
it turns out partials are not working. this is due to a missing buildbot property: previous_buildid

right now we have mach generating props for partials:
'partialMarFilename': u'firefox-36.0a1.en-US.mac.partial.20141101040202-20141102040206.mar',
'partialMarHash': u'8247923d6ba5faf6432bd12e004e27d3bb0a46fe7344a79a29dfa6ddab65670f74ad4f163ee18858f38ed6d54e086fbd18c978b3f0fb7b665dc19ad630aca800',
'partialMarSize': 168285,
'partialMarUrl': u'http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/11/2014-11-02-04-02-06-ash/firefox-36.0a1.en-US.mac.partial.20141101040202-20141102040206.mar'

Also, after looking at http://mxr.mozilla.org/build/source/tools/scripts/updates/balrog-submitter.py?rev=25f7fb5bab4f#66 it seems balrog doesn't care about these props individually but instead looks them up within the partialInfo key.

we need mach to:
    - add 'previous_buildid' to that mach_build_properties.json

we then need mozharness to:
    - add {'partialInfo': [ # all the partial props[1] including 'previous_buildid']} to buildbot props

[1] http://mxr.mozilla.org/build/source/buildbotcustom/process/factory.py?rev=0fefa828e5f9#2521
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This should set the partialInfo property similar to the NightlyBuildFactory: http://mxr.mozilla.org/build/source/buildbotcustom/process/factory.py#2521

The same code from the ReleaseBuildFactory will need to be done as part of the releases followup in bug 1094363.
Attachment #8517628 - Flags: review?(mh+mozilla)
Comment on attachment 8517628 [details] [diff] [review]
bug1087104-partialInfo

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

::: build/gen_mach_buildprops.py
@@ +102,5 @@
>      if args.partial_mar_file:
>          json_data.update(getMarProperties(args.partial_mar_file, partial=True))
> +
> +        # Pull the previous buildid from the partial mar filename.
> +        res = re.match(r'.*\.([0-9]*)-[0-9]*.mar', args.partial_mar_file)

You want +es instead of *s.
Attachment #8517628 - Flags: review?(mh+mozilla) → review+
Ok, I used +es for the [0-9] parts.

inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/edea136c8f99
https://hg.mozilla.org/mozilla-central/rev/edea136c8f99
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.