When doing the push to the mirrors, we would like also to update the product details transparently. This is easily scriptable task: https://wiki.mozilla.org/Release_Management/Release_Notes_and_Product_Details#How_to_update which release management is doing by hand during the beta cycle and the release. One way of doing that could be to update this script: http://hg.mozilla.org/build/tools/file/tip/scripts/release/stage-tasks.py to do it on the fly when the push to mirror is done Or in this one if it is run at each release: http://hg.mozilla.org/build/tools/file/tip/release/final-verification.sh Both scripts found from this page: https://wiki.mozilla.org/Release:Release_Automation_on_Mercurial:Updates_through_Shipping#Push_to_mirrors

The scripting part of this seems easy. There's at least a couple of other things that will need to happen: * Create an svn account for "ffxbld" * Install svn on machines that can run "postrelease"

Ben, I guess those are not blocking issues?

(In reply to Sylvestre Ledru [:sylvestre] from comment #2) > Ben, I guess those are not blocking issues? Nope! Just work items that need to get done.

After a chat with Ben and Rail on IRC, this should be the best solution: <rail> so you would have LATEST_FIREFOX_VERSION.php.template, LATEST_FIREFOX_VERSION.php, require_once("LATEST_FIREFOX_VERSION.php") and generate LATEST_FIREFOX_VERSION.php using LATEST_FIREFOX_VERSION.php.template (jinja2!) as a part of post-release <rail> and probably you want to have multiple files (a file per variable), so you don't touch beta while running a release post-release job

So, I implemented most of the proposal of comment #4. However, I could not use the template idea for the history file (firefox, fennec or thunderbird). This for two reasons: 1) we need to keep the history. We would have to update the template too. 2) we need a marker to know where we are. Example: With: '31.0' => '2014-07-22', // {{ NEXT_MAJOR }} ); AFAIK, with jinja2, we cannot get '31.0' => '2014-07-22', '32.0' => '2014-09-04', // {{ NEXT_MAJOR }} ); There are 3 patches here: * 0001-Update-the-product-details-from-stage-tasks.py.patch Add a new function updateProductDetail(product, version), called from stage-tasks.py From the 'version', this function will guess if it is a beta, aurora, release, dot release or esr and update the relevant files. product can be thunderbird, fennec or firefox * 0002-Add-unitary-tests-for-the-product-details-update.patch nose unitary tests. it will also clone the svn product details to test * templates-update-product-details.diff update product details. Two main changes: - use templates for manage the data - adding marker to be able to insert new history data at the right place. For an obvious reason, I haven't yet implemented the svn commit. Obviously, despite all the unit tests, I would need help to test that in real life situation. For example, the configuration (svn login + path to product detail) needs to be improved.

I can also implement a sanity check in PHP if you think that it is relevant.

Comment on attachment 8477351 [details] [diff] [review] templates-update-product-details.diff Review of attachment 8477351 [details] [diff] [review]: ----------------------------------------------------------------- Even though the patch looks OK to me (except a typo below), I don't trust my PHP knowledge to properly review this. Also would be better to get a review from the module owner (if any). ::: history/firefoxHistory.class.php @@ +200,5 @@ > '24.4.0' => '2014-03-18', > '24.5.0' => '2014-04-29', > '29.0.1' => '2014-05-09', > + '24.6.0' => '2014-06-10', > +z // NEXT_STABILITY typo? ("z" at the beginning)

Comment on attachment 8477352 [details] [diff] [review] 0001-Update-the-product-details-from-stage-tasks.py.patch Review of attachment 8477352 [details] [diff] [review]: ----------------------------------------------------------------- ::: scripts/release/product_details_update.py @@ +5,5 @@ > +import time > +import subprocess > + > +targetSVNDirectory = "product-details.svn" > +loginSVN = "sledru%40mozilla.com" I doubt we can use this account... I don't think that we have svn installed on the slaves... @@ +14,5 @@ > + > +def initSVN(): > + # stage-tasks.sh has been run from the workdir > + if not os.path.isdir(targetSVNDirectory): > + subprocess.call(["svn", "co", "svn+ssh://" + loginSVN + "@svn.mozilla.org/libs/product-details", targetSVNDirectory]) instead of using subprocess.call can you use run_cmd() like in http://hg.mozilla.org/build/tools/file/3e0df48625f6/scripts/release/tag-release.py#l57 or retry() for commands involving networking (because they can fail) like in http://hg.mozilla.org/build/tools/file/3e0df48625f6/scripts/release/tag-release.py#l171 @@ +17,5 @@ > + if not os.path.isdir(targetSVNDirectory): > + subprocess.call(["svn", "co", "svn+ssh://" + loginSVN + "@svn.mozilla.org/libs/product-details", targetSVNDirectory]) > + > + # Sanity check > + svnStatus = subprocess.check_output(["svn", "status", targetSVNDirectory]) the same here, run_cmd() is logging friendly. @@ +21,5 @@ > + svnStatus = subprocess.check_output(["svn", "status", targetSVNDirectory]) > + if len(svnStatus) != 0: > + print "Uncommited changes: " > + print svnStatus > + print "Failing" Using logging here would be better - timestamps for free, back traces, logging levels, etc. @@ +48,5 @@ > +# We cannot use a template mechanism for history file > +# because we need to keep the markers > +# Do it by hand > + > + f = open("history/" + filename, 'r') os.path.join() would look better here :) with open(...) as ...: would be in tact with the code above. @@ +71,5 @@ > +def newAurora(product, version): > + if product == "fennec": > + updateVersionTemplate("mobile_alpha_version.php", version) > + > + if product == "firefox": A nit. elif sounds safer here and in the similar places below. @@ +145,5 @@ > + > +def isAurora(version): > + if re.match('.*a2$', version): > + return True > + return False We already maintain ANY_VERSION_REGEX http://hg.mozilla.org/build/tools/file/3e0df48625f6/lib/python/build/versions.py#l13 which is covered by tests and can be used here. Something like m = re.match(ANY_VERSION_REGEX, version) and depending on what you want: return m and m.groups()[2] == "a" # aurora return m and m.groups()[2] == "b" # beta return m and m.groups()[3] == "esr" # esr for major/minor it will look like this: if m and all(e is None for n in m.groups()[1:]): # only groups()[0] is not None # either major or minor release if len(m.groups()[0].count(".") > 1: print "minor" else: print "major" @@ +177,5 @@ > + return False > + > + > +def isTBRelease(version): > + if re.match('^(\d+)\.[\d.]*\d$', version): Hmmm, I think this matches any major/minor release version... @@ +185,5 @@ > + > +def updateProductDetail(product, version): > + retval = os.getcwd() > + > + os.chdir("product-details.svn") I would avoid chdirs because they may change pwd for the rest of the script if this function is wrapped in try/except.

Comment on attachment 8477351 [details] [diff] [review] templates-update-product-details.diff Review of attachment 8477351 [details] [diff] [review]: ----------------------------------------------------------------- Per IRC, I think you can do better with this part: > 2) we need a marker to know where we are. > Example: > With: > '31.0' => '2014-07-22', > // {{ NEXT_MAJOR }} > ); > AFAIK, with jinja2, we cannot get > '31.0' => '2014-07-22', > '32.0' => '2014-09-04', > // {{ NEXT_MAJOR }} > ); But if it doesn't work out, I'm fine with this. And as Rail says, you need a real reviewer for this. The best I can give you is feedback+

Comment on attachment 8477352 [details] [diff] [review] 0001-Update-the-product-details-from-stage-tasks.py.patch Review of attachment 8477352 [details] [diff] [review]: ----------------------------------------------------------------- As a general comment on this and the 2 other build/tools patches, there's a lot of things that need to move to different places. Any helper methods should live in lib/python, but be sure you look for other things in there that would fulfill your needs first. Tests should go in lib/python/mozilla_buildtools/test. Some other comments inline: ::: scripts/release/product_details_update.py @@ +5,5 @@ > +import time > +import subprocess > + > +targetSVNDirectory = "product-details.svn" > +loginSVN = "sledru%40mozilla.com" In addition to what Rail says, these are magic strings and need to be passed to functions - not globals. They'll have to get passed in all the way from stage-tasks.py. @@ +8,5 @@ > +targetSVNDirectory = "product-details.svn" > +loginSVN = "sledru%40mozilla.com" > +fxHistoryFile = "firefoxHistory.class.php" > +mobileHistoryFile = "mobileHistory.class.php" > +tbHistoryFile = "thunderbirdHistory.class.php" These ones are _probably_ okay to stay as defined globals. @@ +12,5 @@ > +tbHistoryFile = "thunderbirdHistory.class.php" > + > + > +def initSVN(): > + # stage-tasks.sh has been run from the workdir I think this method as a whole should just move to stage-tasks.py. I don't really see anything re-usable about it. @@ +14,5 @@ > + > +def initSVN(): > + # stage-tasks.sh has been run from the workdir > + if not os.path.isdir(targetSVNDirectory): > + subprocess.call(["svn", "co", "svn+ssh://" + loginSVN + "@svn.mozilla.org/libs/product-details", targetSVNDirectory]) I strongly suggest using both -- wrap the run_cmd in a retry for any network operations (and use run_cmd instead of subprocess for local operations). If there's any network operations that could end up with a dirty state if they fail, you can pass a cleanup function to deal with it. Also, I think you should clobber before you checkout. Otherwise the second time this runs on a slave, you'll end up with an old rev and either fail to commit the changes, or miss out on anything that's changed since it was last run. @@ +25,5 @@ > + print "Failing" > +# sys.exit(1) > + > + > +def updateVersionTemplate(filename, version): I don't think this method is necessary. See below. @@ +67,5 @@ > + f.write(newdata) > + f.close() > + > + > +def newAurora(product, version): These new* methods look like they can be refactored into something simpler. Some sort of lookup objects would probably work well, and get rid of most of the repetitive code. For example: PRODUCT_BRANCH_TEMPLATES = { "fennec": { "aurora": { "version_template": "templates/mobile_alpha_version.php.tpl", }, "beta": { "version_template": "templates/mobile_beta_version.php.tpl", "history_info": { "template": "templates/mobileHistory.class.php.tpl", "marker": "NEXT_DEVELOPMENT", }, "major": { "version_template": "templates/mobile_latest_version.php.tpl", "history_info": { "template": "templates/mobileHistory.class.php.tpl", "marker": "NEXT_MAJOR" }, }, "minor": { "version_template": "templates/mobile_latest_version.php.tpl", "history_info": { "template": "templates/mobileHistory.class.php.tpl", "marker": "NEXT_STABILITY" }, }, } } And then you can have a simpler function like: def updateProductDetails(product, type_, version): info = PRODUCT_VERSION_TEMPLATES.get(product, {}).get(type_) template = info["version_template"] history_info = info.get("history_info") history_info = PRODUCT_VERSION_TEMPLATES.get(product, {}).get(type_) if template: updateVersionTemplate(template, version) if history_info: addNewVersion(history_info["marker"], history_info["template"], version) @@ +145,5 @@ > + > +def isAurora(version): > + if re.match('.*a2$', version): > + return True > + return False +1 to what Rail says. These should live in versions.py, too. @@ +185,5 @@ > + > +def updateProductDetail(product, version): > + retval = os.getcwd() > + > + os.chdir("product-details.svn") +1. This magic string should be passed in from stage-tasks.py, too. But I think you can get rid of this method almost entirely if you use the structure I showed above. ::: scripts/release/stage-tasks.py @@ +397,5 @@ > bouncer_aliases=bouncer_aliases) > + > + if 'product-details' in args: > + initSVN() > + updateProductDetail(productName, version) You need to SVN commit at some point. I would recommend putting the initSVN() stuff, the call to updateProductDetail, and the svn commit in a helper function in this file. I don't think the SVN operations belong in the library.

Comment on attachment 8477353 [details] [diff] [review] 0002-Add-unitary-tests-for-the-product-details-update.patch Review of attachment 8477353 [details] [diff] [review]: ----------------------------------------------------------------- Nothing to say about this one that wasn't covered in the other review.

Comment on attachment 8477353 [details] [diff] [review] 0002-Add-unitary-tests-for-the-product-details-update.patch Skipping review for 2 reasons: already r-'ed, probably will completely change after the code changes.

Comment on attachment 8480728 [details] [diff] [review] 0001-Update-the-product-details-from-stage-tasks.py.patch I am not super happy about my changes in stage-tasks.py. It needs a bit of polish (like the loginSVN). Besides that product_details_update.py, should match your expectations. I just realized that we will also need a php interpreter on the server (for the json exporter). FYI, a (stage-tasks => stage_tasks) symlink is needed to be able to import it Python does not manage - in the file name

That should be closer to your expectations.

I didn't move the test on purpose because it seems that the python files are not imported (I guess it is because stage-tasks stuff are scripts)

PHP update (various minor fixes) + sanity check added (sanity_check.php)

(In reply to Sylvestre Ledru [:sylvestre] from comment #19) > Comment on attachment 8480728 [details] [diff] [review] > 0001-Update-the-product-details-from-stage-tasks.py.patch > I just realized that we will also need a php interpreter on the server (for > the json exporter). Yeah. Please file a separate bug for this. Someone in RelEng can take care of it.

(In reply to Sylvestre Ledru [:sylvestre] from comment #19) > FYI, a (stage-tasks => stage_tasks) symlink is needed to be able to import it > Python does not manage - in the file name This isn't necessary. Anything that you need to import should be in lib/python somewhere - not in stage-tasks.py.

Comment on attachment 8481384 [details] [diff] [review] 0001-Update-the-product-details-from-stage-tasks.py.patch Review of attachment 8481384 [details] [diff] [review]: ----------------------------------------------------------------- ::: scripts/release/product_details_update.py @@ +1,1 @@ > +from jinja2 import Environment, FileSystemLoader This stuff should all go into lib/python somewhere. Maybe lib/python/release/product_details.py? The code itself looks quite good. I want to play with it a bit myself, but I don't see any issues visually. @@ +170,5 @@ > + > + if template: > + updateVersionTemplate(targetSVNDirectory, template, version) > + > + if "version_template_2" in info: What is version_template_2? This seems like a magic string that could use some explaining. ::: scripts/release/stage-tasks.py @@ +73,4 @@ > r'bing/win32/en-US/Firefox-Bing\ Setup.exe': r'bing/win32/en-US/Firefox\ Setup\ %(version)s.exe', > } > > +LOGIN_SVN = "ffxbld" This should be an argument that's passed to the script. ffxbld is a fine default, though. @@ +290,5 @@ > +def exportJSON(targetSVNDirectory): > + retval = os.getcwd() > + os.chdir(targetSVNDirectory) > + run_cmd(["php", "export_json.php"]) > + os.chdir(retval) I think you're OK to go without tests on these two, but if you are going to test them, please put them in lib/python/util/svn.py and put the tests in lib/python/mozilla_buildtools/test/test_util_svn.py. @@ +432,5 @@ > + updateProductDetail(targetSVNDirectory, productName, version) > + # Export to json > + exportJSON(targetSVNDirectory) > + # Commit to svn > + #commitSVN(targetSVNDirectory, productName, version) Please wrap all of these in a single call for the sake of consistency.

Comment on attachment 8481384 [details] [diff] [review] 0001-Update-the-product-details-from-stage-tasks.py.patch Review of attachment 8481384 [details] [diff] [review]: ----------------------------------------------------------------- ::: scripts/release/product_details_update.py @@ +91,5 @@ > + > +} > + > + > +def updateVersionTemplate(targetSVNDirectory, templateFile, version): It'd be really nice to have docstrings to describe what these methods do, come to think of it.

Comment on attachment 8481387 [details] [diff] [review] 0002-Add-unitary-tests-for-the-product-details-update-A-s.patch Review of attachment 8481387 [details] [diff] [review]: ----------------------------------------------------------------- The content of the tests themselves seem fine, just some structural stuff to deal with. These patches are greatly improved overall, very nice! ::: scripts/release/tests/test_product_detail.py @@ +1,1 @@ > +from product_details_update import updateProductDetail, getTypeFromVersion Per my other comments, these need to move into the standard location. @@ +8,5 @@ > + > + > +def ContentCheckfile(filename, value): > + print open(filename).read() > + print value Don't print, use log. Or just remove these if they're temporary debugging things. @@ +17,5 @@ > + with open(filename) as f: > + assert re.findall(regexp,f.read(),re.MULTILINE) != [] > + > + > +def test_product_detail_fennec_aurora(): You should use the standard class layout for tests. Eg: class TestProductDetails(unittest.TestCase): def _checkfile(fn, value): ... def testFirefoxAurora(self): ... @@ +30,5 @@ > + > +def test_product_detail_fennec_beta(): > + updateProductDetail(targetSVNDirectory, "fennec", "32.0b3") > + ContentCheckfile(targetSVNDirectory+"/mobile_beta_version.php", "'32.0b3'") > + ContentCheckfileRegexp(targetSVNDirectory+"/history/firefoxHistory.class.php", "'32.0b3' => '\d+-\d+-\d+',") This is a great place where mock can help you test better. Rather than using a regex to look for a match, you can have mock force strftime to return a constant value, and look for that instead. You should be able to get rid of ContentCheckfileRegexp completely if you do that. Here's a very similar example, where we use mock to return a constant timestamp: https://github.com/mozilla/balrog/blob/32434c35474470e155fc5de518a26214b2016e7e/auslib/test/test_db.py#L317

Refresh of the PHP product details (conflicting with the new releases).

Comment on attachment 8484200 [details] [diff] [review] templates-update-product-details-2.diff I am going to consider that we, the release team (including Mark for TB and Paul/Josh for www), are the owner of product details. So, I request the review to you guys about my patch. Basically, I did three changes: * move declarations into dedicated files in order to use a template mechanism. * I added "marker" into the history files to be able to easily insert new values. * Add a sanity check (checks if the include can be done, check if the data structure are okay). The variables used in PHP are the same and it won't break any of the tools based on this. The one potential issue could be if third party tools parse directly the PHP source code (but I hope nobody is doing that). I would like to commit that asap because it is a pain to test the Python code without these changes into the SVN.

Comment on attachment 8484200 [details] [diff] [review] templates-update-product-details-2.diff Review of attachment 8484200 [details] [diff] [review]: ----------------------------------------------------------------- If the changes to the history files are intentional, you'll need to run `php export_json.php`, to update the json tables. ::: history/firefoxHistory.class.php @@ +206,5 @@ > '24.7.0' => '2014-07-22', > '24.8.0' => '2014-09-02', > '31.1.0' => '2014-09-02', > + '33.0.2' => '2014-09-04', > + '51.1.0' => '2014-09-04', FF 51 already? ::: history/mobileHistory.class.php @@ +35,5 @@ > '29.0' => '2014-04-29', > '30.0' => '2014-06-10', > '31.0' => '2014-07-22', > + '32.0' => '2014-09-02', > + '32.0' => '2014-09-04', This file also has multiple dates for the same versions. ::: history/thunderbirdHistory.class.php @@ +29,5 @@ > '24.0' => '2013-09-17', > + '31.0' => '2014-07-22', > + '33.0' => '2014-08-28', > + '33.0' => '2014-08-29', > + '33.0' => '2014-09-04', Multiple dates for the same version? I also wouldn't expect 33.0 to be added to the TB major releases, as we aren't going to release it. @@ +127,5 @@ > '24.7.0' => '2014-07-28', > '24.8.0' => '2014-09-02', > + '31.1.0' => '2041-09-02', > + '33.0.2' => '2014-09-04', > + '31.1.2' => '2014-09-04', These don't seem quite right either.... @@ +161,5 @@ > '5.0b1' => '2011-06-02', > '6.0b1' => '2011-07-20', > '6.0b2' => '2011-08-02', > + '6.0b3' => '2011-08-10', > + '32.0b3' => '2014-08-28', We only released 32.0b1 iirc ::: thunderbirdBuildDetails.php @@ +4,4 @@ > */ > require_once dirname(__FILE__).'/productDetails.class.php'; > > + include("LATEST_THUNDERBIRD_VERSION.php"); Looks like it may be better to use the require_once version that's used everywhere else in product-details (I'm assuming this is performance reasons).

The changes as you describe them seem good. I'm no PHP guy (anymore), so I can't do better than the review of the others here, but I do know that the PHP pages that remain for www.mozilla.org do use the PHP data structures, but don't parse them, so I think this should be safe for that use. I'll take a further look, but this seems like good step forward.

Mark, most of the issues that you reported where artifact from tests... sorry :( Anyway, the attached patch removes all of them + change include => require_once If that OK with you, I would like to commit that ASAP. It is a pain to maintain such patch.

Comment on attachment 8490112 [details] [diff] [review] templates-update-product-details-3.diff >Index: history/firefoxHistory.class.php >=================================================================== >--- history/firefoxHistory.class.php (révision 131864) >+++ history/firefoxHistory.class.php (copie de travail) >@@ -42,7 +42,8 @@ > '29.0' => '2014-04-29', > '30.0' => '2014-06-10', > '31.0' => '2014-07-22', >- '32.0' => '2014-09-02' >+ '32.0' => '2014-09-02', >+ // NEXT_MAJOR > ); I think there should probably be a comment in the file that makes it clear that NEXT_MAJOR and the like are markers used to automatically update the history and should be listed at the end of the history list. I would hate for someone to accidentally move/remove one of these markers and break the automation. r=me with this additional comment added to each of the history files. The rest of the patch looks good to me with the caveat that others have listed that I am not a PHP expert. However, I don't know that we need an expert in this case as you have had some good review and the result is pretty straightforward.

Comment on attachment 8490112 [details] [diff] [review] templates-update-product-details-3.diff Review of attachment 8490112 [details] [diff] [review]: ----------------------------------------------------------------- Great, much better.

Comment on attachment 8490112 [details] [diff] [review] templates-update-product-details-3.diff Thanks. Merged: http://viewvc.svn.mozilla.org/vc?view=revision&revision=131910

A few comments: * the 2 tests are using my SVN login. This should be updated to ffxbld once the account is set up. * I haven't been able to test the direct usage of stage-tasks.py. The script is failing. However, the various functions are tested. Besides that, I think I implemented all your comments! Thanks again for the quality of your review. It is now much much better!

Comment on attachment 8495148 [details] [diff] [review] 0001-Update-the-product-details-from-stage-tasks.py.patch Review of attachment 8495148 [details] [diff] [review]: ----------------------------------------------------------------- This patch is looking pretty good now! I glossed over the product_details_update.py part a bit because we talked that to death before and the code looks pretty clean now. As long as the tests are passing, that's good enough for me. Just a couple of small things below to fix: ::: lib/python/util/svn.py @@ +9,5 @@ > + """ > + Checkout the product details SVN > + """ > + if not os.path.isdir(targetSVNDirectory): > + cmd = ["svn", "co", "svn+ssh://" + loginSVN + "@svn.mozilla.org/libs/product-details", targetSVNDirectory] This repo URI should be an argument that's passed in from stage-tasks.py (which probably should read it from the release config - I can help with that part), and this method can then be a generic "checkoutSVN".n all the way from the release config. @@ +22,5 @@ > +def initSVN(targetSVNDirectory, loginSVN="ffxbld"): > + """ > + Checkout the svn repository and check that it has no pending modifications > + """ > + retry(checkoutProductDetailsSVN, args=(targetSVNDirectory, loginSVN), attempts=3, sleeptime=0) This method doesn't look like it needs to exist, given that it's only a single method. Callers can call checkoutSVN directly instead.

Comment on attachment 8495150 [details] [diff] [review] 0002-Add-unitary-tests-for-the-product-details-update.patch Review of attachment 8495150 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Sylvestre Ledru [:sylvestre] from comment #38) > A few comments: > * the 2 tests are using my SVN login. This should be updated to ffxbld once > the account is set up. It looks like the only thing you depend on SVN-wise for the tests is checkout it out. This still needs to be removed, unfortunately, having tests that depend on an external server is a recipe for weird failures. The easiest way to deal with this is to create a minimal svn repo and put it in lib/python/mozilla_buildtools/test. If you don't want to check in that sort of thing, you can also run local svn commands to create the repo you want as part of setUp(). We do that for our Mercurial tests (https://github.com/mozilla/build-tools/blob/master/lib/python/mozilla_buildtools/test/test_util_hg.py#L93), but it's a bit simpler there because the functionality we're testing doesn't depend on any specific files in the repository. ::: lib/python/mozilla_buildtools/test/test_product_detail.py @@ +13,5 @@ > +class TestProductDetails(unittest.TestCase): > + > + @classmethod > + def setup_class(cls): > + initSVN(targetSVNDirectory, "sledru%40mozilla.com") Same thing here about setup_class vs setUp. These tests could fail if the execution order changes, or additinoal tests that touch the same files are added. You almost always want setUp, because it runs before every test, not just when this class is instantiated. You should also move the tmpdir creation into here so that you get a fresh directory for each test. It may seem inefficient, but it's pretty cheap compared to the cost of debugging failures caused by test execution order. @@ +16,5 @@ > + def setup_class(cls): > + initSVN(targetSVNDirectory, "sledru%40mozilla.com") > + > + def test_export_svn(self): > + assert os.path.isdir(targetSVNDirectory) What is this test testing? I don't see it running any code that isn't in this file. @@ +19,5 @@ > + def test_export_svn(self): > + assert os.path.isdir(targetSVNDirectory) > + > + def test_export_json(self): > + exportJSON(targetSVNDirectory) This looks like a dupe of the test in test_svn.py - you only need it in one place. @@ +48,5 @@ > + self.ContentCheckfile(targetSVNDirectory+"/LATEST_FIREFOX_RELEASED_DEVEL_VERSION.php", "'33.0b12'") > + self.ContentCheckfile(targetSVNDirectory+"/history/firefoxHistory.class.php", "'33.0b12' => '2014-09-06',") > + > + > +# Thunderbird is not doing anything with beta Thunderbird ships one beta per cycle, so I'm not sure if this is correct. @@ +110,5 @@ > + run_cmd(["php", os.path.join(targetSVNDirectory, "sanity_check.php")]) > + > + # @classmethod > + # def teardown_class(cls): > + # shutil.rmtree(targetSVNDirectory) This should be tearDown to match the change to setUp, and you _definitely_ want to remove the tmpdir in it. ::: lib/python/mozilla_buildtools/test/test_util_svn.py @@ +12,5 @@ > +class TestProductDetailsSVN(unittest.TestCase): > + > + @classmethod > + def setup_class(cls): > + initSVN(targetSVNDirectory, "sledru%40mozilla.com") I think you probably want setUp instead of setup_class. Otherwise, test_export_json may fail if it runs after test_commitSVN. @@ +15,5 @@ > + def setup_class(cls): > + initSVN(targetSVNDirectory, "sledru%40mozilla.com") > + > + def test_export_json(self): > + exportJSON(targetSVNDirectory) It's good to test that these run without exception, but you should also verify that they did what was expected. In this case, it looks like you should make sure that the JSON was actually exported, and maybe contains something expected. Once you have a known test repo (instead of cloning from production), data validation is much easier.

Changes: 1) the svn URL is given as argument 2) initSVN removed

Much more changes: * Ship the product detail svn into the test/ directory to avoid the potential svn checkout issue. Also greatly improve the speed of the tests * add more checks around exportJSON * "Thunderbird ships one beta per cycle, so I'm not sure if this is correct." => yes but there is no data for the history of tb * implement all changes you asked

Comment on attachment 8498245 [details] [diff] [review] 0001-Update-the-product-details-from-stage-tasks.py.patch Review of attachment 8498245 [details] [diff] [review]: ----------------------------------------------------------------- Pretty darn close now, needs some actual testing though. Do we have a staging svn server that it can be tested against? ::: lib/python/util/svn.py @@ +47,5 @@ > + raise Exception("Could not find the svn directory") > + commitMSG = "'" + product + " version " + version + "'" > + if not fakeCommit: > + retry(doCommitSVN, args=(commitMSG), attempts=3, sleeptime=0) > + os.chdir(retval) You should wrap everything after the first os.chdir() in a try/finally block. The chdir back to retval should go into finally - otherwise you could break the caller if something goes wrong. It would be great to add a test to make sure this happens in case of exception, too, but not a blocker. In an ideal world, svn would have an argument that lets you specify the repo to work on (like hg's -R), but it doesn't look like that exists :(. ::: scripts/release/stage-tasks.py @@ +398,5 @@ > bouncer_aliases=bouncer_aliases) > + > + if 'product-details' in args: > + targetSVNDirectory = "product-details.svn" > + svnURL = "svn+ssh://ffxbld@svn.mozilla.org/libs/product-details" This will need to be factored out to args, but I'm not sure how you can test this part, because it really needs to be run on one of our machines. I just got pulled into something else, so I can't help with this right now :(.

Comment on attachment 8498248 [details] [diff] [review] 0002-Add-unitary-tests-for-the-product-details-update.patch Review of attachment 8498248 [details] [diff] [review]: ----------------------------------------------------------------- This is okay other than the thing one below. But to be honest, you can probably just kill the svn test...it doesn't actually test anything other than svn itself. r=me if you kill the test, or I need a new patch that doesn't depend on the external repo. ::: lib/python/mozilla_buildtools/test/test_util_svn.py @@ +14,5 @@ > + def setUp(self): > + self.tempDir = tempfile.mkdtemp() > + svnURL = "svn+ssh://sledru%40mozilla.com@svn.mozilla.org/libs/product-details" > + self.targetSVNDirectory = self.tempDir + "/product-details.svn" > + retry(checkoutProductDetailsSVN, args=(self.targetSVNDirectory, svnURL), attempts=3, sleeptime=0) This should be using a local repo, too.

> Do we have a staging svn server that it can be tested against? Not that I am aware off :/ > This will need to be factored out to args, but I'm not sure how you can test this part, because it really needs to be run on one of our machines. Any way for me to reproduce the env to test that myself?

test_util_svn.py removed as you asked :) Carrying the r+

Comment on attachment 8498688 [details] [diff] [review] 0001-Update-the-product-details-from-stage-tasks.py.patch >diff --git a/lib/python/util/svn.py b/lib/python/util/svn.py >+def commitSVN(targetSVNDirectory, product, version, fakeCommit=False): >+ """ >+ Commit the change in the svn >+ """ >+ retval = os.getcwd() >+ os.chdir(targetSVNDirectory) >+ if not os.path.isdir(".svn"): >+ raise Exception("Could not find the svn directory") >+ commitMSG = "'" + product + " version " + version + "'" >+ try: >+ if not fakeCommit: >+ retry(doCommitSVN, args=(commitMSG), attempts=3, sleeptime=0) >+ finally: >+ os.chdir(retval) You need to wrap everything past the first chdir(). If you get an exception above (for example, when you raise a few lines up), the caller will end up in a different directory than it started. >diff --git a/scripts/release/stage-tasks.py b/scripts/release/stage-tasks.py >+ >+ if 'product-details' in args: >+ targetSVNDirectory = "product-details.svn" >+ svnURL = "svn+ssh://ffxbld@svn.mozilla.org/libs/product-details" >+ updateProductDetail(targetSVNDirectory, productName, version, svnURL) This part still isn't factored out into the args/options. Without that, our staging releases will break. It also lets us use the tbirdbld account for Thunderbird updates (if we're doing those - I'm not sure).

> This part still isn't factored out into the args/options. Without that, our > staging releases will break. It also lets us use the tbirdbld account for > Thunderbird updates (if we're doing those - I'm not sure). I am going to need your help on this. I cannot test this section. I was hoping you could do that for me ;)

(In reply to Sylvestre Ledru [:sylvestre] from comment #48) > Created attachment 8503974 [details] [diff] [review] > 0001-Update-the-product-details-from-stage-tasks.py.patch > > > This part still isn't factored out into the args/options. Without that, our > > staging releases will break. It also lets us use the tbirdbld account for > > Thunderbird updates (if we're doing those - I'm not sure). > I am going to need your help on this. I cannot test this section. I was > hoping you could do that for me ;) I'm not going to have time to do this for at least a couple of weeks, unfortunately :(.

(In reply to Ben Hearsum [:bhearsum] from comment #49) > (In reply to Sylvestre Ledru [:sylvestre] from comment #48) > > Created attachment 8503974 [details] [diff] [review] > > 0001-Update-the-product-details-from-stage-tasks.py.patch > > > > > This part still isn't factored out into the args/options. Without that, our > > > staging releases will break. It also lets us use the tbirdbld account for > > > Thunderbird updates (if we're doing those - I'm not sure). > > I am going to need your help on this. I cannot test this section. I was > > hoping you could do that for me ;) > > I'm not going to have time to do this for at least a couple of weeks, > unfortunately :(. I'm also not sure how to test this without a staging svn repo, come to think of it...

Comment on attachment 8503974 [details] [diff] [review] 0001-Update-the-product-details-from-stage-tasks.py.patch I'm removing review from this again because there are code changes that are still needed, and I don't have any further comment on the code right now.

Ben, I would like to make some progress on this. How can I help? Setting a staging svn env?

(In reply to Sylvestre Ledru [:sylvestre] from comment #52) > Ben, I would like to make some progress on this. How can I help? Setting a > staging svn env? I really don't know how we can set-up a staging svn server, but that's not the part I'm terribly concerned about. The svn commands are simple and I expect them to work. It's all of the code around that that needs a staging test, ideally through Buildbot. That requires a buildbot master, a slave, and a server that's set-up similarly to stage.mozilla.org. Coincidentally, we have those things in the RelEng network :). I think we can give you enough access so that you can test it out yourself. I'm going to try to get that set up for you today.

I updated my patch. It now updates (svn propset) mozilla.com since it is still necessary... :( FYI, I haven't tested the whole thing (don't want to update mozilla.com in production ;). Ben, as discussed in another bug, I agree we should book some time to work on a build bot instance for me. I proposed that we used this one as a proof of concept ;)

fixed a typo

Comment on attachment 8532743 [details] [diff] [review] attachment.cgi?id=8526889 Review of attachment 8532743 [details] [diff] [review]: ----------------------------------------------------------------- I'm really sorry this is taking so long, but we really need to get it tested in staging. Beyond that, there's still an issue with the patch: ::: scripts/release/stage-tasks.py @@ +419,5 @@ > + if 'product-details' in args: > + targetSVNDirectory = "product-details.svn" > + targetSVNDirectoryMozilla = "mozilla.com.svn" > + svnURLpd = "svn+ssh://ffxbld@svn.mozilla.org/libs/product-details" > + svnURLmozilla = "svn+ssh://ffxbld@svn.mozilla.org/projects/mozilla.com" These URLs need to be moved out to the release configs still. We can't hardcode production ones here.

(In reply to Ben Hearsum [:bhearsum] from comment #56) > These URLs need to be moved out to the release configs still. We can't > hardcode production ones here. Yes, that is on purpose, I need to test that code before moving it!

Query on the timing - how do pushes to product-details & mozilla.com get deployed ? We automatically push to mirrors for beta releases, which may be far too early to update svn; similarly for releases it's still several hours before. Did you mean 'enable updates' instead ?

Bug 1062897 talks about running this code in postrelease, so the summary here needs fixing up ?

(In reply to Nick Thomas [:nthomas] from comment #59) > Bug 1062897 talks about running this code in postrelease, so the summary > here needs fixing up ? Yeah, definitely. The details changed a bit here soon after filing.

(In reply to Nick Thomas [:nthomas] from comment #58) > Did you mean 'enable updates' instead ? How can I test that? Thanks

This is a roll up of the tests + stage-tasks.py patch and includes the fixes I wanted re: putting repos in the config file. Still need to update the config files themselves and do some sort of sanity check.

(In reply to Ben Hearsum [:bhearsum] from comment #62) > Created attachment 8550538 [details] [diff] [review] > roll up + fixes > > This is a roll up of the tests + stage-tasks.py patch and includes the fixes > I wanted re: putting repos in the config file. Still need to update the > config files themselves and do some sort of sanity check. I'm failing at creating staging equivalents of the svn repos. Since this is in postrelease it's fairly low risk, so I'm planning to skip any sort of staging test on this. I won't be landing until after 35.0.1 ships though.

Comment on attachment 8550538 [details] [diff] [review] roll up + fixes I think we're clear to land this any time now. It probably needs review since I tweaked it a fair amount.

Rail, sorry for the lack of summary before. This patch is a combination of work between Sylvestre and I. I think you can safely ignore all the tests (I reviewed them) and svn repo stuff (it's just imports from real repos). The key parts here are the .py files. I'm happy to walk you through if it helps.

Comment on attachment 8550538 [details] [diff] [review] roll up + fixes Review of attachment 8550538 [details] [diff] [review]: ----------------------------------------------------------------- ::: lib/python/util/svn.py @@ +32,5 @@ > +def doCommitSVN(commitMSG): > + """ > + Actually do the commit (called with retry) > + """ > + log.info("svn commit -m " + commitMSG) I think this line can be dropped - run_cmd() is quite verbose.

I was just about to land this when I realized that two things: 1) I still need the buildbot-configs patch 2) Something needs to set SVN_SSH, otherwise the checkouts/commits will fail.

Comment on attachment 8578168 [details] [diff] [review] add svn.mozilla.org to known_hosts stamp

OK, this should get everything we need set in the release configs...I have a new patch for tools incoming to set svnSshKey.

Comment on attachment 8578192 [details] [diff] [review] set SVN_SSH before running svn commands Review of attachment 8578192 [details] [diff] [review]: ----------------------------------------------------------------- The interdiff lgtm

Comment on attachment 8578192 [details] [diff] [review] set SVN_SSH before running svn commands Had to back this out for bustage in the tools repo because "php" isn't available, and trying to install it results in 404. Probably need to play around in my user repo to figure out how to fix it.

Having some trouble getting the tests to pass on Travis...the failure is: test_export_json (mozilla_buildtools.test.test_product_detail.TestProductDetails) ... PHP Parse error: syntax error, unexpected '[' in /tmp/tmplJZC80/product-details.svn/export_json.php on line 64 It passes on my local machine, which has php 5.5. Travis has php 5.3, so maybe that's why?

(In reply to Ben Hearsum [:bhearsum] from comment #74) > Having some trouble getting the tests to pass on Travis...the failure is: > test_export_json > (mozilla_buildtools.test.test_product_detail.TestProductDetails) ... PHP > Parse error: syntax error, unexpected '[' in > /tmp/tmplJZC80/product-details.svn/export_json.php on line 64 > > It passes on my local machine, which has php 5.5. Travis has php 5.3, so > maybe that's why? Catlee spotted what is almost certainly the issue, apparently array definition with "[]" is only supported in 5.4 and up. I pushed a fix to my fork, waiting for Travis to confirm it...

(In reply to Ben Hearsum [:bhearsum] from comment #75) > (In reply to Ben Hearsum [:bhearsum] from comment #74) > > Having some trouble getting the tests to pass on Travis...the failure is: > > test_export_json > > (mozilla_buildtools.test.test_product_detail.TestProductDetails) ... PHP > > Parse error: syntax error, unexpected '[' in > > /tmp/tmplJZC80/product-details.svn/export_json.php on line 64 > > > > It passes on my local machine, which has php 5.5. Travis has php 5.3, so > > maybe that's why? > > Catlee spotted what is almost certainly the issue, apparently array > definition with "[]" is only supported in 5.4 and up. I pushed a fix to my > fork, waiting for Travis to confirm it... I fixed this error, but now there's a new one, presumably because the JSON package doesn't provide the same features in 5.3: PHP Notice: Use of undefined constant JSON_PRETTY_PRINT - assumed 'JSON_PRETTY_PRINT' in /tmp/tmpxm1jyG/product-details.svn/export_json.php on line 24 PHP Warning: json_encode() expects parameter 2 to be long, string given in /tmp/tmpxm1jyG/product-details.svn/export_json.php on line 24 PHP Notice: Use of undefined constant JSON_PRETTY_PRINT - assumed 'JSON_PRETTY_PRINT' in /tmp/tmpxm1jyG/product-details.svn/export_json.php on line 24 I don't think this one test is worth the trouble, especially with a forked version of export_json.php (compared to the real product-details repo) -- I'm just going to disable it.

Here's what I've landed in the build/tools repository - it disables the tests that depend on PHP.

In production: https://hg.mozilla.org/build/buildbot-configs/rev/85e0ddfd7ab0

Fix for Fennec 37.0b7 build1: failed at fennec_push_to_mirrors fennec push to mirrors fails with this error: File "/builds/slave/rel-m-beta-psh_mrrrs-000000000/scripts/scripts/release/stage-tasks.py", line 28, in <module> from product_details_update import updateProductDetailFiles ImportError: No module named product_details_update it should be from release.product_details_update import updateProductDetailFiles

Comment on attachment 8580573 [details] [diff] [review] [tools] Fennec 37.0b7 build1: failed at fennec_push_to_mirrors.patch Review of attachment 8580573 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Massimo!

Comment on attachment 8580573 [details] [diff] [review] [tools] Fennec 37.0b7 build1: failed at fennec_push_to_mirrors.patch landed: https://hg.mozilla.org/build/tools/rev/fc59b2f9965c

last patch fixed the release.product_details_update import. There's still an error on the import (fennec push to mirrors log): File "/builds/slave/rel-m-beta-psh_mrrrs-000000000/scripts/lib/python/release/product_details_update.py", line 1, in <module> from jinja2 import Environment, FileSystemLoader ImportError: No module named jinja2 as a temporary fix, I have added Jinja2-2.7.3 and MarkupSafe-0.23 in lib/python/vendor and updated lib/python/vendorlibs.pth https://hg.mozilla.org/build/tools/rev/5cbbd855ed8c

(In reply to Massimo Gervasini [:mgerva] from comment #82) > last patch fixed the release.product_details_update import. > > There's still an error on the import (fennec push to mirrors log): > > File > "/builds/slave/rel-m-beta-psh_mrrrs-000000000/scripts/lib/python/release/ > product_details_update.py", line 1, in <module> > from jinja2 import Environment, FileSystemLoader > ImportError: No module named jinja2 > > as a temporary fix, I have added Jinja2-2.7.3 and MarkupSafe-0.23 in > lib/python/vendor and updated lib/python/vendorlibs.pth Thanks, and sorry. We're going to need a better fix here, since Jinja2 is a binary library, and not going to function on all of the platforms that this script can run on.

(In reply to Ben Hearsum [:bhearsum] from comment #83) > (In reply to Massimo Gervasini [:mgerva] from comment #82) > > last patch fixed the release.product_details_update import. > > > > There's still an error on the import (fennec push to mirrors log): > > > > File > > "/builds/slave/rel-m-beta-psh_mrrrs-000000000/scripts/lib/python/release/ > > product_details_update.py", line 1, in <module> > > from jinja2 import Environment, FileSystemLoader > > ImportError: No module named jinja2 > > > > as a temporary fix, I have added Jinja2-2.7.3 and MarkupSafe-0.23 in > > lib/python/vendor and updated lib/python/vendorlibs.pth > > Thanks, and sorry. We're going to need a better fix here, since Jinja2 is a > binary library, and not going to function on all of the platforms that this > script can run on. Turns out jinja2 is pure python now, so maybe this isn't an issue.

Jinja2 is pure python, but has an optional speedups compiled component that just makes it faster. It's not required.

Comment on attachment 8578833 [details] [diff] [review] patch as landed Review of attachment 8578833 [details] [diff] [review]: ----------------------------------------------------------------- This didn't work. First of all, there's an issue with the stage-tasks.py patch (we check args instead of actions to see if product details should be updated - which always returns False). I fixed that by hand and then got this: retry: Calling checkoutSVN with args: ('product-details.svn', 'svn+ssh://ffxbld@svn.mozilla.org/libs/product-details'), kwargs: {}, attempt #1 command: START command: svn co svn+ssh://ffxbld@svn.mozilla.org/libs/product-details product-details.svn command: cwd: /builds/slave/rel-m-beta-firefox_pr-00000000 command: output: svn: Error in child process: exec of '/home/cltbld/.ssh/ffxbld_dsa' failed: Permission denied command: ERROR Traceback (most recent call last): File "/builds/slave/rel-m-beta-firefox_pr-00000000/scripts/lib/python/util/commands.py", line 51, in run_cmd return subprocess.check_call(cmd, **kwargs) File "/tools/python27/lib/python2.7/subprocess.py", line 511, in check_call raise CalledProcessError(retcode, cmd) CalledProcessError: Command '['svn', 'co', 'svn+ssh://ffxbld@svn.mozilla.org/libs/product-details', 'product-details.svn']' returned non-zero exit status 1 command: END (0.16s elapsed)

(In reply to Ben Hearsum [:bhearsum] from comment #86) > Comment on attachment 8578833 [details] [diff] [review] > patch as landed > > Review of attachment 8578833 [details] [diff] [review]: > ----------------------------------------------------------------- > > This didn't work. First of all, there's an issue with the stage-tasks.py > patch (we check args instead of actions to see if product details should be > updated - which always returns False). I fixed that by hand and then got > this: > retry: Calling checkoutSVN with args: ('product-details.svn', > 'svn+ssh://ffxbld@svn.mozilla.org/libs/product-details'), kwargs: {}, > attempt #1 > command: START > command: svn co svn+ssh://ffxbld@svn.mozilla.org/libs/product-details > product-details.svn > command: cwd: /builds/slave/rel-m-beta-firefox_pr-00000000 > command: output: > svn: Error in child process: exec of '/home/cltbld/.ssh/ffxbld_dsa' failed: > Permission denied > command: ERROR > Traceback (most recent call last): > File > "/builds/slave/rel-m-beta-firefox_pr-00000000/scripts/lib/python/util/ > commands.py", line 51, in run_cmd > return subprocess.check_call(cmd, **kwargs) > File "/tools/python27/lib/python2.7/subprocess.py", line 511, in check_call > raise CalledProcessError(retcode, cmd) > CalledProcessError: Command '['svn', 'co', > 'svn+ssh://ffxbld@svn.mozilla.org/libs/product-details', > 'product-details.svn']' returned non-zero exit status 1 > command: END (0.16s elapsed) Bah, this is stupid. SVN_SSH needs to be a command, not a key.

This is what I used when I was testing. I'm explicitly _not_ fixing the args vs. actions thing so that I can run this by hand for the next release. Once I know this is working I'll fix that to make it run through automation.

Comment on attachment 8581656 [details] [diff] [review] fix SVN_SSH r+ with no changes to buildfarm/release/release-runner.sh

We worked through a bunch of issues today, including: * Remove svn status from checkout * Change fennec to mobile in data structure * Use full path for svn dirs and pass them down to product details functions There was also a bunch of issues because we use PHP 5.3, but the code is written for PHP 5.5. Sylvestre is still working through those. We might hit more issues on the Python side afterwards, but we definitely need these ones.

We are working on a different approach. Moving the product detail into ship it: bug 1083718