841316 - Make "go to build directory" link show up for Try builds before they complete (to reduce delay caused by make check)

Status: RESOLVED FIXED

(Tree Management Graveyard :: TBPL, defect)

Description

[Ed Morley [:emorley]]

From glandium on IRC...

Due to make check taking ages + the fact that running builds disappear for a while before showing up as completed, there is often a significant delay before one can use the "go to build directory" link (since it only shows up for completed builds, since only then do we have the log URL).

However, for Try we can guess the URL from the pusher email address, even before we have the log URLs from buildbot - and so display the link even when the job is running.

In fact, before adding "go to build directory" for non-try trees (bug ), guessing the URL was the way it was implemented.

The only downside is that until the build completes, the guessed link will 404, but I think that doesn't matter (the trychooser email that thanks people for their push & links to the directory, has the same issue).

Comment 1

[Edmund Wong (:ewong)]

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

This patch only goes to the root build directory of the pusher's rev, 
i.e. 
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-<rev>/

I have not yet figured out how to get the machine names i.e. try-linux from
Linux x86-64 try build.

Comment 2

[Ed Morley [:emorley]]

Comment on attachment 717720 [details] [diff] [review]
Add 'go to build' link for Try builds (v1)

Thank you for taking a look at this :-)

Whilst the approach in this patch may work with a bit of tweaking, we're kind of abusing the rawLogURL field, by filling it with a directory, rather than a log, until the Try job completes.

My preferred approach would be to re-add part of the logic that bug 656155 removed (second to last paragraph in comment 0), for no rawLogURL && is Try.

ie in UserInterface.js:

> (function htmlForBuilds() {
>   if (result.machine.type.indexOf('Build') == -1 &&
>       result.machine.type.indexOf('Nightly') == -1) {
>     return '';
>   }
>   if (!result.rawLogURL)
>     return '';
>   var logPath = result.rawLogURL.slice(0, result.rawLogURL.lastIndexOf('/'));
>   return '<a href="' + logPath + '/" target="_blank">go to build directory</a>';
> })()

if rawLogURL
  -> use raw log url like we do at present
else if a Try push
  -> use logic removed by bug 656155 
else
  -> give up, and return nothing

Comment 3

[Edmund Wong (:ewong)]

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

Comment 4

[Ed Morley [:emorley]]

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

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

Almost there :-)

::: js/UserInterface.js
@@ +1473,5 @@
>          }
> +        if (result.rawLogURL) {
> +          var logPath = result.rawLogURL.slice(0, result.rawLogURL.lastIndexOf('/'));
> +          return '<a href="' + logPath + '/" target="_blank">go to build directory</a>';
> +        }

Style nit: The else needs to be on the same line as the brace.

@@ +1474,5 @@
> +        if (result.rawLogURL) {
> +          var logPath = result.rawLogURL.slice(0, result.rawLogURL.lastIndexOf('/'));
> +          return '<a href="' + logPath + '/" target="_blank">go to build directory</a>';
> +        }
> +        else if (result.tree == "Try") {

This will only catch normal try, rather than comm-central try; instead perhaps add "&& 'isTry' in treeInfo" after the existing "'ftpDir' in treeInfo", a couple of lines down.

@@ +1479,5 @@
> +          for (var repo in result.revs) {
> +            var treeInfo = UserInterface.getTreeInfoForRepo(repo);
> +            if (treeInfo && 'ftpDir' in treeInfo) {
> +              return '<a href="' + treeInfo.ftpDir + '/' + result.push.pusher + '-' +
> +                     result.revs[repo] + '/">go to build directory</a>';

I'd like to avoid repeating the markup if possible - for each of the two cases above (have rawLogURL and the Try fallback) capture the result & then at the end check to see if we have "directoryURL" (or similar) and return that, else ''.

@@ +1486,1 @@
>            return '';

Unnecessary return (with the following else removed).

@@ +1486,3 @@
>            return '';
> +        }
> +        else 

Unnecessary else

Comment 5

[Edmund Wong (:ewong)]

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

Comment 6

[Ed Morley [:emorley]]

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

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

Getting closer, but still a few review comments left from the last review. Please can you test it locally before attaching the next patch (this version doesn't work). Thank you for working on this bug :-)

::: js/UserInterface.js
@@ +1470,5 @@
>          if (result.machine.type.indexOf('Build') == -1 &&
>              result.machine.type.indexOf('Nightly') == -1) {
>            return '';
>          }
> +        var logResult = '';

Perhaps use 'directoryURL' instead for clarity? (see comment 4)

@@ +1473,5 @@
>          }
> +        var logResult = '';
> +        if (result.rawLogURL) {
> +          var logPath = result.rawLogURL.slice(0, result.rawLogURL.lastIndexOf('/'));
> +          logResult = '<a href="' + logPath + '/" target="_blank">go to build directory</a>';

We're still duplicating the markup here, all we need to do is:
directoryURL = ...

@@ +1474,5 @@
> +        var logResult = '';
> +        if (result.rawLogURL) {
> +          var logPath = result.rawLogURL.slice(0, result.rawLogURL.lastIndexOf('/'));
> +          logResult = '<a href="' + logPath + '/" target="_blank">go to build directory</a>';
> +        } else if (result.tree == "Try") {

You need to remove this, the "&& 'isTry' in treeInfo" replaces it.

@@ +1479,5 @@
> +          for (var repo in result.revs) {
> +            var treeInfo = UserInterface.getTreeInfoForRepo(repo);
> +            if (treeInfo && 'ftpDir' in treeInfo && 'isTry' in treeInfo) {
> +              logResult = '<a href="' + treeInfo.ftpDir + '/' + result.push.pusher + '-' +
> +                     result.revs[repo] + '/">go to build directory</a>';

We don't want the markup here, just set directoryURL to the correct URL.

@@ +1485,5 @@
> +          }
> +        }
> +        if (!logResult)
> +          logResult = '';
> +        return logResult;

Add the markup here instead.

ie: 
if (directoryURL)
  return '<a href......'
return ''

Comment 7

[Ed Morley [:emorley]]

(To test locally, just open index.html from the filesystem, no need to use the Vagrant project etc)

Comment 8

[Edmund Wong (:ewong)]

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

Comment 9

[Ed Morley [:emorley]]

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

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

Looks good & works well when I tested :-)

Can land with the one nit fixed (no need to re-attach a patch).

Thank you for doing this!

::: js/UserInterface.js
@@ +1471,5 @@
>          if (result.machine.type.indexOf('Build') == -1 &&
>              result.machine.type.indexOf('Nightly') == -1) {
>            return '';
>          }
> +        var directoryURL = '';

This can be |var directoryURL;|

Comment 10

[Edmund Wong (:ewong)]

Pushed to tbpl:
http://hg.mozilla.org/webtools/tbpl/rev/e843ce657f7d

Comment 11

[Ed Morley [:emorley]]

In production :-)