update product details when shipping releases

RESOLVED WONTFIX

Status

Release Engineering
Release Automation
RESOLVED WONTFIX
3 years ago
2 years ago

People

(Reporter: sylvestre, Assigned: sylvestre)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 21 obsolete attachments)

15.37 KB, patch
standard8
: review+
lmandel
: review+
sylvestre
: checked-in+
Details | Diff | Splinter Review
2.78 KB, patch
rail
: review+
bhearsum
: checked-in+
Details | Diff | Splinter Review
16.69 KB, patch
rail
: review+
bhearsum
: checked-in+
Details | Diff | Splinter Review
1.79 MB, patch
bhearsum
: checked-in+
Details | Diff | Splinter Review
853 bytes, patch
pmoore
: review+
pmoore
: review+
massimo
: checked-in+
Details | Diff | Splinter Review
979 bytes, patch
rail
: review+
bhearsum
: checked-in+
Details | Diff | Splinter Review
4.27 KB, patch
rail
: review+
bhearsum
: checked-in+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
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"
(Assignee)

Comment 2

3 years ago
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.
(Assignee)

Comment 4

3 years ago
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
(Assignee)

Comment 5

3 years ago
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.
(Assignee)

Comment 6

3 years ago
Created attachment 8477351 [details] [diff] [review]
templates-update-product-details.diff
Attachment #8477351 - Flags: review?(rail)
Attachment #8477351 - Flags: review?(bhearsum)
(Assignee)

Comment 7

3 years ago
Created attachment 8477352 [details] [diff] [review]
0001-Update-the-product-details-from-stage-tasks.py.patch
Attachment #8477352 - Flags: review?(rail)
Attachment #8477352 - Flags: review?(bhearsum)
(Assignee)

Comment 8

3 years ago
Created attachment 8477353 [details] [diff] [review]
0002-Add-unitary-tests-for-the-product-details-update.patch
Attachment #8477353 - Flags: review?(rail)
Attachment #8477353 - Flags: review?(bhearsum)
(Assignee)

Updated

3 years ago
Assignee: nobody → sledru
(Assignee)

Comment 9

3 years ago
I can also implement a sanity check in PHP if you think that it is relevant.
(Assignee)

Comment 10

3 years ago
Created attachment 8477441 [details] [diff] [review]
0001-Manage-the-errors-with-exception.patch
Attachment #8477441 - Flags: review?
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)
Attachment #8477351 - Flags: review?(rail)
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.
Attachment #8477352 - Flags: review?(rail) → review-
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+
Attachment #8477351 - Flags: review?(bhearsum) → 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.
Attachment #8477352 - Flags: review?(bhearsum) → review-
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.
Attachment #8477353 - Flags: review?(bhearsum) → review-
Attachment #8477441 - Flags: 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.
Attachment #8477353 - Flags: review?(rail)
(Assignee)

Comment 17

3 years ago
Created attachment 8480728 [details] [diff] [review]
0001-Update-the-product-details-from-stage-tasks.py.patch
Attachment #8477352 - Attachment is obsolete: true
Attachment #8477441 - Attachment is obsolete: true
Attachment #8480728 - Flags: review?(bhearsum)
(Assignee)

Comment 18

3 years ago
Created attachment 8480730 [details] [diff] [review]
0002-Add-unitary-tests-for-the-product-details-update.patch
Attachment #8477353 - Attachment is obsolete: true
Attachment #8480730 - Flags: review?(bhearsum)
(Assignee)

Comment 19

3 years ago
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
Attachment #8480728 - Flags: review?(bhearsum) → feedback?(bhearsum)
(Assignee)

Comment 20

3 years ago
Created attachment 8481384 [details] [diff] [review]
0001-Update-the-product-details-from-stage-tasks.py.patch

That should be closer to your expectations.
Attachment #8480728 - Attachment is obsolete: true
Attachment #8480728 - Flags: feedback?(bhearsum)
Attachment #8481384 - Flags: review?(bhearsum)
(Assignee)

Comment 21

3 years ago
Created attachment 8481387 [details] [diff] [review]
0002-Add-unitary-tests-for-the-product-details-update-A-s.patch

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)
Attachment #8480730 - Attachment is obsolete: true
Attachment #8480730 - Flags: review?(bhearsum)
Attachment #8481387 - Flags: review?(bhearsum)
(Assignee)

Comment 22

3 years ago
Created attachment 8481388 [details] [diff] [review]
templates-update-product-details.diff

PHP update (various minor fixes) + sanity check added (sanity_check.php)
Attachment #8477351 - Attachment is obsolete: true
(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.
Attachment #8481384 - Flags: review?(bhearsum) → review-
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
Attachment #8481387 - Flags: review?(bhearsum) → review-
(Assignee)

Updated

3 years ago
Depends on: 1062897
(Assignee)

Comment 28

3 years ago
Created attachment 8484200 [details] [diff] [review]
templates-update-product-details-2.diff

Refresh of the PHP product details (conflicting with the new releases).
Attachment #8481388 - Attachment is obsolete: true
(Assignee)

Comment 29

3 years ago
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.
Attachment #8484200 - Flags: review?(standard8)
Attachment #8484200 - Flags: review?(pmac)
Attachment #8484200 - Flags: review?(lsblakk)
Attachment #8484200 - Flags: review?(lmandel)
Attachment #8484200 - Flags: review?(jmize)
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).
Attachment #8484200 - Flags: review?(standard8) → review-
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.
(Assignee)

Updated

3 years ago
Depends on: 1068049
(Assignee)

Comment 32

3 years ago
Created attachment 8490112 [details] [diff] [review]
templates-update-product-details-3.diff

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.
Attachment #8484200 - Attachment is obsolete: true
Attachment #8484200 - Flags: review?(pmac)
Attachment #8484200 - Flags: review?(lsblakk)
Attachment #8484200 - Flags: review?(lmandel)
Attachment #8484200 - Flags: review?(jmize)
Attachment #8490112 - Flags: review?(standard8)
Attachment #8490112 - Flags: review?(lmandel)
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.
Attachment #8490112 - Flags: review?(lmandel) → review+
Comment on attachment 8490112 [details] [diff] [review]
templates-update-product-details-3.diff

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

Great, much better.
Attachment #8490112 - Flags: review?(standard8) → review+
(Assignee)

Comment 35

3 years ago
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
Attachment #8490112 - Flags: checked-in+
(Assignee)

Updated

3 years ago
Depends on: 1069418
(Assignee)

Comment 36

3 years ago
Created attachment 8495148 [details] [diff] [review]
0001-Update-the-product-details-from-stage-tasks.py.patch
Attachment #8481384 - Attachment is obsolete: true
Attachment #8495148 - Flags: review?(bhearsum)
(Assignee)

Comment 37

3 years ago
Created attachment 8495150 [details] [diff] [review]
0002-Add-unitary-tests-for-the-product-details-update.patch
Attachment #8481387 - Attachment is obsolete: true
Attachment #8495150 - Flags: review?(bhearsum)
(Assignee)

Comment 38

3 years ago
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.
Attachment #8495148 - Flags: review?(bhearsum) → review-
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.
Attachment #8495150 - Flags: review?(bhearsum) → review-
(Assignee)

Comment 41

3 years ago
Created attachment 8498245 [details] [diff] [review]
0001-Update-the-product-details-from-stage-tasks.py.patch

Changes:
1) the svn URL is given as argument
2) initSVN removed
Attachment #8495148 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8498245 - Flags: review?(bhearsum)
(Assignee)

Comment 42

3 years ago
Created attachment 8498248 [details] [diff] [review]
0002-Add-unitary-tests-for-the-product-details-update.patch

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
Attachment #8495150 - Attachment is obsolete: true
Attachment #8498248 - Flags: review?(bhearsum)
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 :(.
Attachment #8498245 - Flags: review?(bhearsum) → review-
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.
Attachment #8498248 - Flags: review?(bhearsum) → review+
(Assignee)

Comment 45

3 years ago
Created attachment 8498688 [details] [diff] [review]
0001-Update-the-product-details-from-stage-tasks.py.patch

> 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?
Attachment #8498245 - Attachment is obsolete: true
Attachment #8498688 - Flags: review?(bhearsum)
(Assignee)

Comment 46

3 years ago
Created attachment 8498689 [details] [diff] [review]
0002-Add-unitary-tests-for-the-product-details-update.patch

test_util_svn.py removed as you asked :) 
Carrying the r+
Attachment #8498248 - Attachment is obsolete: true
Attachment #8498689 - Flags: review+
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).
Attachment #8498688 - Flags: review?(bhearsum) → review-
(Assignee)

Comment 48

3 years ago
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 ;)
Attachment #8498688 - Attachment is obsolete: true
Attachment #8503974 - Flags: review?(bhearsum)
(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.
Attachment #8503974 - Flags: review?(bhearsum)
(Assignee)

Comment 52

3 years ago
Ben, I would like to make some progress on this. How can I help? Setting a staging svn env?
Flags: needinfo?(bhearsum)
(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.
Flags: needinfo?(bhearsum)
(Assignee)

Comment 54

3 years ago
Created attachment 8526889 [details] [diff] [review]
0001-Bug-1053814-Update-the-product-details-and-mozilla.c.patch

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 ;)
Attachment #8503974 - Attachment is obsolete: true
Attachment #8526889 - Flags: review?(bhearsum)
(Assignee)

Comment 55

2 years ago
Created attachment 8532743 [details] [diff] [review]
attachment.cgi?id=8526889

fixed a typo
Attachment #8526889 - Attachment is obsolete: true
Attachment #8526889 - Flags: review?(bhearsum)
Attachment #8532743 - Flags: review?(bhearsum)
Attachment #8532743 - Attachment is patch: true
Attachment #8532743 - Attachment mime type: application/mbox → text/plain
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.
Attachment #8532743 - Flags: review?(bhearsum) → review-
Depends on: 1108614
(Assignee)

Comment 57

2 years ago
(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.
Summary: Update transparently the product details when pushing to the mirror → update product details when shipping releases
(Assignee)

Comment 61

2 years ago
(In reply to Nick Thomas [:nthomas] from comment #58)
> Did you mean 'enable updates' instead ?
How can I test that? Thanks
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.
Attachment #8498689 - Attachment is obsolete: true
Attachment #8532743 - Attachment is obsolete: true
(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.
Attachment #8550538 - Flags: review?(rail)
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.
Attachment #8550538 - Flags: review?(rail) → review+
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.
Created attachment 8578168 [details] [diff] [review]
add svn.mozilla.org to known_hosts
Attachment #8578168 - Flags: review?(rail)
Comment on attachment 8578168 [details] [diff] [review]
add svn.mozilla.org to known_hosts

stamp
Attachment #8578168 - Flags: review?(rail) → review+
Created attachment 8578191 [details] [diff] [review]
update release configs

OK, this should get everything we need set in the release configs...I have a new patch for tools incoming to set svnSshKey.
Attachment #8578191 - Flags: review?(rail)
Created attachment 8578192 [details] [diff] [review]
set SVN_SSH before running svn commands
Attachment #8550538 - Attachment is obsolete: true
Attachment #8578192 - Flags: review?(rail)
Attachment #8578191 - Flags: review?(rail) → review+
Comment on attachment 8578192 [details] [diff] [review]
set SVN_SSH before running svn commands

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

The interdiff lgtm
Attachment #8578192 - Flags: review?(rail) → review+
Attachment #8578168 - Flags: checked-in+
Attachment #8578191 - Flags: checked-in+
Attachment #8578192 - Flags: checked-in+
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.
Attachment #8578192 - Flags: checked-in+ → checked-in-
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.
Created attachment 8578833 [details] [diff] [review]
patch as landed

Here's what I've landed in the build/tools repository - it disables the tests that depend on PHP.
Attachment #8578192 - Attachment is obsolete: true
Attachment #8578833 - Flags: checked-in+
In production: https://hg.mozilla.org/build/buildbot-configs/rev/85e0ddfd7ab0
Created attachment 8580573 [details] [diff] [review]
[tools] Fennec 37.0b7 build1: failed at fennec_push_to_mirrors.patch

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
Attachment #8580573 - Flags: review?(pmoore)
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!
Attachment #8580573 - Flags: review+
Attachment #8580573 - Flags: review?(pmoore) → review+
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
Attachment #8580573 - Flags: checked-in+
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.
Created attachment 8581656 [details] [diff] [review]
fix SVN_SSH

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.
Attachment #8581656 - Flags: review?(rail)
Comment on attachment 8581656 [details] [diff] [review]
fix SVN_SSH

r+ with no changes to buildfarm/release/release-runner.sh
Attachment #8581656 - Flags: review?(rail) → review+
Attachment #8581656 - Flags: checked-in+
Created attachment 8583845 [details] [diff] [review]
bunch of fixes

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.
Attachment #8583845 - Flags: review?(rail)
Attachment #8583845 - Flags: review?(rail) → review+
Attachment #8583845 - Flags: checked-in+
(Assignee)

Comment 91

2 years ago
We are working on a different approach. Moving the product detail into ship it: bug 1083718
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.