656155 - Add "go to build" link for non-Try builds

Status: RESOLVED FIXED

(Tree Management Graveyard :: TBPL, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

This would be handy to have.

Comment 1

[Mounir Lamouri (:mounir)]

For non-try servers build, the organization seems to be like this:
https://ftp.mozilla.org/pub/mozilla.org/{firefox,thunderbird}/tinderbox-builds/${repoName}-${OS}/${rev}/

Comment 2

[Mounir Lamouri (:mounir)]

The last part isn't the revision actually but an ID I can't found :(

Comment 3

[Markus Stange [:mstange]]

The buildbot JSON provides the build url, so fixing this will be trivial once we no longer get our data from Tinderbox. My patches for that are almost ready for review.

Comment 5

[JP Rosevear [:jpr]]

Any chance the patches noted in #3 are now landed and we can fix this?

Comment 6

[Matt Brubeck (:mbrubeck)]

(In reply to JP Rosevear [:jpr] from comment #5)
> Any chance the patches noted in #3 are now landed and we can fix this?

Yes, we've now switched from Tinderbox to buildbot.  I still don't see where to find the build directory, though.  Any pointers?

Comment 7

[Justin Lebar (not reading bugmail)]

> Yes, we've now switched from Tinderbox to buildbot.  I still don't see where to find the build 
> directory, though.  Any pointers?

Philor taught me: Click a build.  View brief log.   Copy link location of view full log.  Get the directory from there.

Comment 8

[Matt Brubeck (:mbrubeck)]

(In reply to Justin Lebar [:jlebar] from comment #7)
> Philor taught me: Click a build.  View brief log.   Copy link location of
> view full log.  Get the directory from there.

Right -- I commented the same thing in duplicate bug 699624.  But now I'm wondering where to find this in the buildbot JSON so I can fix this in the TBPL code.

Comment 9

[Phil Ringnalda (:philor)]

That's actually what you want to do in code, too - there's both a properties["packageUrl"] and a properties["log_url"], which afaik are always both the same path with different filenames (I certainly hope we don't ever upload build logs and builds in different directories), and for either one you have to trim off a filename to get a "go to build" directory, so you might as well trim off the filename of the log_url, since we're already storing that in the db.

Comment 11

[Matt Brubeck (:mbrubeck)]

Un-assigning this, though I'll take it again if I have time later to download and set up a vagrant environment for local testing.

If someone else wants to work on this, feel free.  I think it's just a matter of selecting "runs.log" from the database here:
https://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/file/ce4276d1d49c/php/getRevisionBuilds.php#l16

then grabbing it from the JSON result and adding it to the JavaScript object here:
https://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/file/ce4276d1d49c/js/BuildbotDBUser.js#l40

then stripping the filename and adding a link somewhere in the UI generated by JavaScript for the result.

Comment 13

[Edmund Wong (:ewong)]

Attachment: Add 'go to build' link for non-Try builds. (v1)

Comment 14

[Edmund Wong (:ewong)]

Attachment: Add 'go to build' link for non-Try builds. (v2)

Comment 15

[Matt Brubeck (:mbrubeck)]

Comment on attachment 654907 [details] [diff] [review]
Add 'go to build' link for non-Try builds. (v2)

Looks good!  I'd just like to see some minor changes...

>+++ b/js/BuildbotDBUser.js
>+        "runLog": run.log,

Naming nit: I would call this something like "runLogURL" (or maybe "rawLogURL") for consistency with the other property names.

>+++ b/js/UserInterface.js
>+      (function htmlForNonTryBuilds() {
>+        if (result.machine.type != 'Build') {
>+          return '';
>+        }
>+        if (result.tree != 'Try') {

This would be better as "if (this.treeInfo.isTry)" since we have more than one Try tree.

However, instead of a separate function for non-Try, I think it might be better to change htmlForTryBuilds to work for both Try and non-Try builds, by adding your code here as the fallback if no 'ftpDir' is found.  (It would replace the second "return '';" in htmlForTryBuilds.)

Or maybe we should just remove htmlForTryBuilds and use your new code for all trees...

>+          var logList = result.runLog.split('/');
>+          logList[logList.length-1] = "";
>+          var logPath = logList.join('/');
>+          var logPath = logPath.substring(0, logPath.length - 1);

I think you can remove the last line, since the trailing slash will just get added back to the URL by the server anyway.

>+          return '<a href="' + logPath + '/">go to build</a>';

Let's use "go to build directory" for consistency with the Try version (despite the summary of this bug).

Comment 16

[Edmund Wong (:ewong)]

Attachment: Add 'go to build' link for non-Try builds. (v3)

Comment 17

[Edmund Wong (:ewong)]

Attachment: Add 'go to build' link for non-Try builds. (v3)

attaching the correct patch.

Comment 18

[Ed Morley [:emorley]]

Comment on attachment 654929 [details] [diff] [review]
Add 'go to build' link for non-Try builds. (v3)

Unfortunately testing this locally gives paths such as:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-macosx64-debug/1345814849//

(Note the extra trailing '/')

Slice would seem like a better candidate here perhaps?

I made three testcases on jsperf.com using the method in this patch, slice & also regexp for good luck:
http://jsperf.com/slice-vs-split-for-directory-from-filepath

Slice is the fastest (by about a factor of 7) and also the easiest to grok whilst reading the code, IMO.

Comment 19

[Ed Morley [:emorley]]

Oh and forgot to add:
The 'go to build directory' links are missing from Nightlies and some of the B2G builds (eg B2G gb_armv7a_gecko-debug mozilla-central build).

Presume this just needs tweaking:
if (result.machine.type != 'Build') {
           return '';
}

Comment 20

[Edmund Wong (:ewong)]

Attachment: Add 'go to build' link for non-Try builds. (v4)

Comment 21

[Ed Morley [:emorley]]

Comment on attachment 655930 [details] [diff] [review]
Add 'go to build' link for non-Try builds. (v4)

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

::: js/UserInterface.js
@@ +1471,5 @@
>          return ret;
>        })().join(', ') + '</span>' +
>        (function htmlForTryBuilds() {
> +        if (result.machine.type.indexOf('Build') == -1 &&
> +            result.machine.type.indexOf('Nightly') == -1) {

I've just taken a look at the source of the types (Data.js) and it seems we've specified some build types in detail (eg B2G) and not the rest - which is why we can't just use "result.machine.type != 'Build' && result.machine.type != 'Nightly'". I'll file a separate bug for that - so for now the solution you have is the best way of doing this.

@@ +1478,5 @@
> +        var logList = result.rawLogURL.split('/');
> +        logList[logList.length-1] = "";
> +        var logPath = logList.join('/');
> +        logPath = logPath.slice(0, logPath.lastIndexOf("/"));
> +        return '<a href="' + logPath + '/">go to build directory</a>';

We shouldn't need to split/join at all. The code sample at the jsperf link in my previous comment should get the job done in one line :-)

Comment 22

[Ed Morley [:emorley]]

Meant to say before...
Testing client-side-only changes can be done a lot more quickly than using Vagrant, see: http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/file/tip/README#l21

Comment 23

[Edmund Wong (:ewong)]

Attachment: Add 'go to build' link for non-Try builds. (v5)

Comment 24

[Edmund Wong (:ewong)]

Attachment: Add 'go to build' link for non-Try builds. (v5)

Comment 25

[Edmund Wong (:ewong)]

Attachment: Add 'go to build' link for non-Try builds. (v6)

Comment 26

[Ed Morley [:emorley]]

Comment on attachment 656314 [details] [diff] [review]
Add 'go to build' link for non-Try builds. (v6)

Sorry for the delay, was away on Friday.

Looks great! Have landed with a couple of tiny things I had missed before (doesn't change functionality).

http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/138261442b9f

>--- a/js/UserInterface.js
>+++ b/js/UserInterface.js
>@@ -1471,17 +1471,12 @@ var UserInterface = {
>       (function htmlForTryBuilds() {

Have changed this to 'htmlForBuilds'.

>diff --git a/php/getRevisionBuilds.php b/php/getRevisionBuilds.php
>--- a/php/getRevisionBuilds.php
>+++ b/php/getRevisionBuilds.php
>@@ -14,7 +14,7 @@ Headers::send(Headers::ALLOW_CROSS_ORIGI
>-  SELECT runs.id, buildbot_id AS _id, buildername, slave, result,
>+  SELECT runs.id, runs.log, buildbot_id AS _id, buildername, slave, result,

Have s/runs.log/log/ since log is a unique field (unlike 'id'), so doesn't need the table specifying (doesn't hurt to do so, of course :D).

Thank you very much for this - once it's rolled out to production, it is going to be a real timesaver for everyone using TBPL :-)

Comment 27

[Ed Morley [:emorley]]

Hasn't been pushed to production yet & there has been some confusion on IRC, so reopening for now.

Comment 28

[Ed Morley [:emorley]]

(I plan on spending some time this weekend making sure the prefetching landing is working as expected, after which I will request a push to prod which will also include this bug).

Comment 29

[Phil Ringnalda (:philor)]

In production.