John Ford [:jhford] CET/CEST Berlin Time Assignee
	Description • 14 years ago

We would like to be able to verify that build artifacts that we consume are valid.  The best way to do this is to upload a checksum file along with the build artifacts.  Some possible ways that we could do this:
1. generate a checksum file entry per build artifact in the make target that generates the artifact
2. generate a single checksum file for all artifacts, appending to this file in each make target
3. generate a single checksum file in the make upload step
4. generate a single checksum file on stage
5. checksum on the slaves entirely in buildbot logic

Option 1 and 2 both have the downside that they require modifications in multiple locations in the build system.  Option 2 has the added downside of being very complicated to track a) when the file should be created, b) when the file should be zeroed out and c) whether the current checksum file reflects the artifacts in the dist dir.

Option 4 seems like a non-option as we would be generating the checksum file a) on a different machine and b) after a data transmission has already occured.

Using option 5 alone doesn't seem like the best option as there is no historical record of the checksums for future reference.  

I propose that we initially implement option 3.  Two approaches to option 3 are a) generating a checksums file in the build/upload.py script and b) generate the checksums in a separate script and add the checksum file to the list of files to transfer.

If there are no objections, I am going to implement option 3 b (generate in make upload using a second script)

	(not currently active) Ted Mielczarek
	Comment 1 • 14 years ago

I don't know that I like the idea of generating it in "make upload", seems out of place, but I can understand your reasoning. Are you planning on one line per file, like <checksum> <filename>\n<checksum> <filename> ?

	John Ford [:jhford] CET/CEST Berlin Time Assignee
	Comment 2 • 14 years ago

(In reply to comment #1)
> I don't know that I like the idea of generating it in "make upload", seems out
> of place, but I can understand your reasoning. Are you planning on one line per
> file, like <checksum> <filename>\n<checksum> <filename> ?

What if i were to create a new target next to upload called checksum?

That is the basic format that I planned to use, though, I was thinking of including the filesize.  Our main issue right now is file being off by exactly one bit.  It would be really helpful to know that files that fail to verify their checksum are the right size easily.

Would a modified format of 
<checksum> <filename> <filesize_in_bytes>\n<checksum> <filename> <filesize_in_bytes> be acceptable?

	(not currently active) Ted Mielczarek
	Comment 3 • 14 years ago

I can't say I care about the specific format of the file, I was just interested. I would recommend that you put <filename> at the end of the line, since it'll have spaces for release builds, making your file hard to parse otherwise.

	John Ford [:jhford] CET/CEST Berlin Time Assignee
	Comment 4 • 14 years ago

(In reply to comment #3)
> I can't say I care about the specific format of the file, I was just
> interested. I would recommend that you put <filename> at the end of the line,
> since it'll have spaces for release builds, making your file hard to parse
> otherwise.

that sounds like a great suggestion!  On second though, i'd also like to include the algorithm the hash is in.  I will use the format

<hash> <algo> <size> <filename>

	John Ford [:jhford] CET/CEST Berlin Time Assignee
	Comment 5 • 14 years ago

Submitting this patch to try to make sure it builds on all platforms + mobile

	John Ford [:jhford] CET/CEST Berlin Time Assignee
	Comment 6 • 14 years ago

this patch adds the checksum target to mobile builds

	John Ford [:jhford] CET/CEST Berlin Time Assignee
	Comment 7 • 14 years ago

Strange issue, This file is being generated on tryserver but it isn't being included in the files to upload.

On try server i get 

make[1]: Entering directory `/e/builds/moz2_slave/tryserver-win32/build/obj-firefox/browser/installer'
CHECKSUM FILE START
8f97720e2f3852a1e49a966142d6fa6ed2637be4 sha1 12678959 firefox-4.0b2pre.en-US.win32.zip
aeed017341bea2ded1f7ce8e270f37ab31dfceaf sha1 8750662 firefox-4.0b2pre.en-US.win32.installer.exe
b4d8d6c9ab26aac58fd1386a46695559d9f5e7ff sha1 36591673 firefox-4.0b2pre.en-US.win32.tests.zip
0920dbb9f014aea730586e656309dbda11cd32f5 sha1 10735196 firefox-4.0b2pre.en-US.win32.crashreporter-symbols.zip
f6fe3abdf3193b4938605c546e3a0efbdbe91cb8 sha1 28 firefox-4.0b2pre.en-US.win32.txt
07e56ae814b9842798b273c4db77c7aaa0dca57f sha1 117009 firefox-4.0b2pre.en-US.langpack.xpi
CHECKSUM FILE END
d:/mozilla-build/python25/python2.5.exe /e/builds/moz2_slave/tryserver-win32/build/build/upload.py --base-path ../../dist \
		"../../dist/firefox-4.0b2pre.en-US.win32.zip" \
		"../../dist/install/sea/firefox-4.0b2pre.en-US.win32.installer.exe" \
		 \
		 \
		"../../dist/firefox-4.0b2pre.en-US.win32.tests.zip" \
		"../../dist/firefox-4.0b2pre.en-US.win32.crashreporter-symbols.zip" \
		 \
		"../../dist//firefox-4.0b2pre.en-US.win32.txt" \
		 \
		  ../../dist/firefox-4.0b2pre.en-US.langpack.xpi
http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/jford@mozilla.com-4aa21a214800/tryserver-win32/firefox-4.0b2pre.en-US.win32.zip
http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/jford@mozilla.com-4aa21a214800/tryserver-win32/firefox-4.0b2pre.en-US.win32.installer.exe
http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/jford@mozilla.com-4aa21a214800/tryserver-win32/firefox-4.0b2pre.en-US.win32.tests.zip
http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/jford@mozilla.com-4aa21a214800/tryserver-win32/firefox-4.0b2pre.en-US.win32.crashreporter-symbols.zip
http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/jford@mozilla.com-4aa21a214800/tryserver-win32/firefox-4.0b2pre.en-US.win32.txt
http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/jford@mozilla.com-4aa21a214800/tryserver-win32/firefox-4.0b2pre.en-US.langpack.xpi
Uploading e:\builds\moz2_slave\tryserver-win32\build\obj-firefox\dist\firefox-4.0b2pre.en-US.win32.zip
Uploading e:\builds\moz2_slave\tryserver-win32\build\obj-firefox\dist\install\sea\firefox-4.0b2pre.en-US.win32.installer.exe
Uploading e:\builds\moz2_slave\tryserver-win32\build\obj-firefox\dist\firefox-4.0b2pre.en-US.win32.tests.zip
Uploading e:\builds\moz2_slave\tryserver-win32\build\obj-firefox\dist\firefox-4.0b2pre.en-US.win32.crashreporter-symbols.zip
Uploading e:\builds\moz2_slave\tryserver-win32\build\obj-firefox\dist\firefox-4.0b2pre.en-US.win32.txt
Uploading e:\builds\moz2_slave\tryserver-win32\build\obj-firefox\dist\firefox-4.0b2pre.en-US.langpack.xpi
Running post-upload command: post_upload.py --tinderbox-builds-dir jford@mozilla.com-4aa21a214800 -i 20100715232402 -p firefox --revision 4aa21a214800 --who jford@mozilla.com --builddir tryserver-win32 --release-to-tryserver-builds
Upload complete


but on my local machine i get (i know it is a different platform, but all of the platforms generate the above file list and my logs fell off tbpl).

CHECKSUM FILE START
415665057f693a236b8b546c396efc8a7b92a470 sha1 13476745 firefox-4.0b2pre.en-US.mac64.dmg
9e3932473aaff10a89ac8be13ea7bcfeff7bf8c4 sha1 34691342 firefox-4.0b2pre.en-US.mac64.tests.zip
17b871f925c252f06c9a497ae9d1989956ac07b3 sha1 28 firefox-4.0b2pre.en-US.mac64.txt
f4f67f50c45931dc654f493fa9165f331daefb39 sha1 116706 firefox-4.0b2pre.en-US.langpack.xpi
CHECKSUM FILE END
/usr/bin/python2.6 /Users/jhford/mozilla/make-hashes/mozilla-central/build/upload.py --base-path ../../dist \
                "../../dist/firefox-4.0b2pre.en-US.mac64.dmg" \
                 \
                 \
                 \
                "../../dist/firefox-4.0b2pre.en-US.mac64.tests.zip" \
                 \
                 \
                "../../dist//firefox-4.0b2pre.en-US.mac64.txt" \
                "../../dist//firefox-4.0b2pre.en-US.mac64.sha1-checksums" \
                  ../../dist/firefox-4.0b2pre.en-US.langpack.xpi
 
I am unsure why the file is not included in the upload when i run this patch through try server. (http://hg.mozilla.org/try/rev/4aa21a214800)

	John Ford [:jhford] CET/CEST Berlin Time Assignee
	Comment 8 • 14 years ago

This is the patch I ran through try.  The checksums file is included in the files to upload on my local machine but not when i run on try server.

	John Ford [:jhford] CET/CEST Berlin Time Assignee
	Comment 9 • 14 years ago

Going to be working on these bugs

	John Ford [:jhford] CET/CEST Berlin Time Assignee
	Comment 10 • 14 years ago

I have brought this patch up to date.  When I submitted this build to try server it generates the file, the file is on the filesystem, but the upload target isn't including the file in the list of files to upload.  My guess is that it is figuring out the values of the list of files to upload before it is running the checksum target.  I found that calling checksum then upload works properly.

I don't know if having the upload target depend on the checksum target is the right thing to do, but I'd love to know what is going wrong (and where my assumptions are broken).

======= LOG =======
(relevant makefile code is http://hg.mozilla.org/mozilla-central/file/583cc29d2a32/toolkit/mozapps/installer/packager.mk#l588)

make[1]: Entering directory `/builds/slave/tryserver-linux/build/obj-firefox/browser/installer'
CHECKSUM FILE START
192e247d7b313ed5a4ebb67fb360e7a9c17fa22f sha1 13011128 firefox-4.0b6pre.en-US.linux-i686.tar.bz2
4fa03f49b14c54b26342f8404974f551d17b78ed sha1 39459886 firefox-4.0b6pre.en-US.linux-i686.tests.zip
bffa78169ee1c26831116d5797dfe71a316d559f sha1 19326591 firefox-4.0b6pre.en-US.linux-i686.crashreporter-symbols.zip
66e5b6dd8c365f62a31eb1621897fb09b2acb32b sha1 28 firefox-4.0b6pre.en-US.linux-i686.txt
b561e58ebe89a96e4cba33db3f6921151e384d0f sha1 203576 firefox-4.0b6pre.en-US.langpack.xpi
CHECKSUM FILE END
/tools/python/bin/python2.5 /builds/slave/tryserver-linux/build/build/upload.py --base-path ../../dist \
		"../../dist/firefox-4.0b6pre.en-US.linux-i686.tar.bz2" \
		 \
		 \
		 \
		"../../dist/firefox-4.0b6pre.en-US.linux-i686.tests.zip" \
		"../../dist/firefox-4.0b6pre.en-US.linux-i686.crashreporter-symbols.zip" \
		 \
		"../../dist//firefox-4.0b6pre.en-US.linux-i686.txt" \
		 \
		  ../../dist/firefox-4.0b6pre.en-US.langpack.xpi

	(not currently active) Ted Mielczarek
	Comment 11 • 14 years ago

Comment on attachment 472006 [details] [diff] [review]
mozilla-central patch

This looks pretty good, just a few comments:
1) I know you were having problems with QUOTED_WILDCARD on the .checksums file, because the file didn't exist when the upload target was being executed. Since you're guaranteed to generate it, you don't need to wildcard it here (but you do need to quote it). You should just be able to list "$(DIST)/$(PKG_PATH)/$(PKG_BASENAME).checksums" directly.

2) Instead of duplicating that whole list of files to upload/checksum, just assign them to a variable, like:
UPLOAD_FILES = \
  $(call QUOTED_WILDCARD ...) \
  $(call QUOTED_WILDCARD ...) \
  $(NULL)

3) I like having function-level docstrings to at least give some notion of what a function's purpose is. They don't have to be exhaustive, but a little bit helps.

Fix those issues up and throw it back into my queue for a quick review.

	John Ford [:jhford] CET/CEST Berlin Time Assignee
	Comment 12 • 14 years ago

This patch addresses your issues I hope.  I have run this through try server and so far my Linux build has completed successfully.  I am about to land (plane), so I will check on the status of the other builds later.

http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/jford@mozilla.com-550dbbbcaec7/tryserver-linux/firefox-4.0b7pre.en-US.linux-i686.checksums

	(not currently active) Ted Mielczarek
	Comment 13 • 14 years ago

Comment on attachment 478557 [details] [diff] [review]
patch v4

>diff --git a/browser/build.mk b/browser/build.mk
>--- a/browser/build.mk
>+++ b/browser/build.mk
>@@ -70,6 +70,9 @@ distclean::
> source-package::
> 	@$(MAKE) -C browser/installer source-package
> 
>+checksum::
>+	@$(MAKE) -C browser/installer checksum
>+

I don't think you need this target for anything, do you?

>diff --git a/toolkit/mozapps/installer/packager.mk b/toolkit/mozapps/installer/packager.mk
>--- a/toolkit/mozapps/installer/packager.mk
>+++ b/toolkit/mozapps/installer/packager.mk
>@@ -586,9 +586,11 @@ empty :=
> space = $(empty) $(empty)
> QUOTED_WILDCARD = $(if $(wildcard $(subst $(space),?,$(1))),"$(1)")
> 
>-upload:
>-	$(PYTHON) $(MOZILLA_DIR)/build/upload.py --base-path $(DIST) \
>-		$(call QUOTED_WILDCARD,$(DIST)/$(PACKAGE)) \
>+# This variable defines which OpenSSL algorithm to use to 
>+# generate checksums for files that we upload
>+CHECKSUM_ALGORITHM = 'sha1'
>+
>+UPLOAD_FILES=$(call QUOTED_WILDCARD,$(DIST)/$(PACKAGE)) \
> 		$(call QUOTED_WILDCARD,$(INSTALLER_PACKAGE)) \
> 		$(call QUOTED_WILDCARD,$(DIST)/$(COMPLETE_MAR)) \
> 		$(call QUOTED_WILDCARD,$(wildcard $(DIST)/$(PARTIAL_MAR))) \

Can you fix the indentation here? We like two-space indent after a continuation, and convention has the values start on the second line, so like:
UPLOAD_FILES = \
  $(call ... ) \
  $(call ..) \
  $(NULL)

>@@ -598,6 +600,22 @@ upload:
> 		$(call QUOTED_WILDCARD,$(DIST)/$(PKG_PATH)/$(PKG_BASENAME).txt) \
> 		$(if $(UPLOAD_EXTRA_FILES), $(foreach f, $(UPLOAD_EXTRA_FILES), $(wildcard $(DIST)/$(f))))
> 
>+checksum:
>+	@$(PYTHON) $(MOZILLA_DIR)/build/checksums.py \
>+		-o $(DIST)/$(PKG_PATH)/$(PKG_BASENAME).checksums \

Might make sense to stick the checksum filename in a variable, CHECKSUM_FILE or something, since you use it in three places.

r=me with those changes.

	John Ford [:jhford] CET/CEST Berlin Time Assignee
	Comment 14 • 14 years ago

(In reply to comment #13)
> Comment on attachment 478557 [details] [diff] [review]
> patch v4
> 
> >diff --git a/browser/build.mk b/browser/build.mk
> >--- a/browser/build.mk
> >+++ b/browser/build.mk
> >@@ -70,6 +70,9 @@ distclean::
> > source-package::
> > 	@$(MAKE) -C browser/installer source-package
> > 
> >+checksum::
> >+	@$(MAKE) -C browser/installer checksum
> >+
> 
> I don't think you need this target for anything, do you?

Yes, that is required to run |make checksum| in the objdir.  I tried commenting it out to confirm and got

jhford-wifi:objdir jhford$ make checksum
make: *** No rule to make target `checksum'.  Stop

> 
> >diff --git a/toolkit/mozapps/installer/packager.mk b/toolkit/mozapps/installer/packager.mk
> >--- a/toolkit/mozapps/installer/packager.mk
> >+++ b/toolkit/mozapps/installer/packager.mk
> >@@ -586,9 +586,11 @@ empty :=
> > space = $(empty) $(empty)
> > QUOTED_WILDCARD = $(if $(wildcard $(subst $(space),?,$(1))),"$(1)")
> > 
> >-upload:
> >-	$(PYTHON) $(MOZILLA_DIR)/build/upload.py --base-path $(DIST) \
> >-		$(call QUOTED_WILDCARD,$(DIST)/$(PACKAGE)) \
> >+# This variable defines which OpenSSL algorithm to use to 
> >+# generate checksums for files that we upload
> >+CHECKSUM_ALGORITHM = 'sha1'
> >+
> >+UPLOAD_FILES=$(call QUOTED_WILDCARD,$(DIST)/$(PACKAGE)) \
> > 		$(call QUOTED_WILDCARD,$(INSTALLER_PACKAGE)) \
> > 		$(call QUOTED_WILDCARD,$(DIST)/$(COMPLETE_MAR)) \
> > 		$(call QUOTED_WILDCARD,$(wildcard $(DIST)/$(PARTIAL_MAR))) \
> 
> Can you fix the indentation here? We like two-space indent after a
> continuation, and convention has the values start on the second line, so like:
> UPLOAD_FILES = \
>   $(call ... ) \
>   $(call ..) \
>   $(NULL)

Fixed in the patch I will upload shortly
 
> >@@ -598,6 +600,22 @@ upload:
> > 		$(call QUOTED_WILDCARD,$(DIST)/$(PKG_PATH)/$(PKG_BASENAME).txt) \
> > 		$(if $(UPLOAD_EXTRA_FILES), $(foreach f, $(UPLOAD_EXTRA_FILES), $(wildcard $(DIST)/$(f))))
> > 
> >+checksum:
> >+	@$(PYTHON) $(MOZILLA_DIR)/build/checksums.py \
> >+		-o $(DIST)/$(PKG_PATH)/$(PKG_BASENAME).checksums \
> 
> Might make sense to stick the checksum filename in a variable, CHECKSUM_FILE or
> something, since you use it in three places.

I should have done that from the start, thanks for the suggestion! 

> r=me with those changes.

Yay!  I was also going to change the algorithm to sha512 as we would likely be able to make snippet generation much quicker by doing so.  I will get the cleaned up patch ready, post it here, run through try then set the blocking2.0? flag.

On my macbook, this costs an extra 1.2 seconds.

	John Ford [:jhford] CET/CEST Berlin Time Assignee
	Comment 15 • 14 years ago

(In reply to comment #14)
> Yay!  I was also going to change the algorithm to sha512 as we would likely be
> able to make snippet generation much quicker by doing so.  I will get the
> cleaned up patch ready, post it here, run through try then set the blocking2.0?
> flag.
> 
> On my macbook, this costs an extra 1.2 seconds.

that 1.2 seconds is for the switch of sha1 to sha512.  I'd love to be able to do the rest of that in 1.2 seconds :D

	(not currently active) Ted Mielczarek
	Comment 16 • 14 years ago

(In reply to comment #14)
> > I don't think you need this target for anything, do you?
> 
> Yes, that is required to run |make checksum| in the objdir.  I tried commenting
> it out to confirm and got

Sure, but why do you need that target at all? The checksum file gets built when you run "make upload", so why are you ever going to call "make checksum" directly?

	John Ford [:jhford] CET/CEST Berlin Time Assignee
	Comment 17 • 14 years ago

(In reply to comment #16)
> (In reply to comment #14)
> > > I don't think you need this target for anything, do you?
> > 
> > Yes, that is required to run |make checksum| in the objdir.  I tried commenting
> > it out to confirm and got
> 
> Sure, but why do you need that target at all? The checksum file gets built when
> you run "make upload", so why are you ever going to call "make checksum"
> directly?

I used it a lot during testing.  I guess it is only really useful for local testing/debugging of the checksum script.

	John Ford [:jhford] CET/CEST Berlin Time Assignee
	Comment 18 • 14 years ago

Updated, unbitrotted and corrected patch.  As this revision of the patch is to address specific review comments, I am carrying the r+ forward.  I am asking for approval to land this patch on mozilla-central.  This patch generates a file with checksums and filesizes for all build artifacts.  This file is going to be used first to detect problems with data transfer between build machines and test machines.  There is also a possibility of this speeding up the release automation process by using the checksums generated during the build for creating snippets instead of downloading the browsers to generate their checksum later on in the process.  The script that is run (checksum.py) could also be extended to satisfy the requirements of bug 583910, which is about storing a secure list of checksums of important update related files.

I tested on try server last night and found that it worked properly.  I have included links to some sample checksum files generated.  

The risk associated with this patch is that it could break the upload of builds.  This would be immediately clear because it would turn builds red and would stop tests from working.  This patch does not touch the shipping bits generated as part of the build.

Example Output:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/jford@mozilla.com-5f4a19b4ce77/tryserver-linux/firefox-4.0b8pre.en-US.linux-i686.checksums
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/jford@mozilla.com-5f4a19b4ce77/tryserver-linux64/firefox-4.0b8pre.en-US.linux-x86_64.checksums
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/jford@mozilla.com-5f4a19b4ce77/tryserver-macosx64/firefox-4.0b8pre.en-US.mac64.checksums
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/jford@mozilla.com-5f4a19b4ce77/tryserver-win32/firefox-4.0b8pre.en-US.win32.checksums

	Michael Wu [:mwu]
	Comment 19 • 14 years ago

http://hg.mozilla.org/mozilla-central/rev/ab6d8c5a300a

	bhearsum@mozilla.com (:bhearsum)
	Comment 21 • 14 years ago

bug 607396 for doing this for l10n

	David E. Ross
	Comment 23 • 14 years ago

When will this become effective?  

This bug report was used to close bug #607978 as a duplicate.  Bug #607978 requested checksums for PARTIAL .mar files.  There is no reference to partial .mar files in bug #578393, and the latest releases -- released after bug #578393 was marked as resolved -- still do not provide checksums for partial .mar files.

	David E. Ross
	Comment 24 • 14 years ago

Per <https://bugzilla.mozilla.org/show_bug.cgi?id=607978#c8>, bug #607978 is not a duplicate of this bug.

	Armen [:armenzg]
	Comment 25 • 14 years ago

Uploading checksums for builds has affected L10n Verification for beta7 (this could have been only noticed on an staging release).

This is now fixed but for more details see bug 609932.