Closed Bug 656155 Opened 9 years ago Closed 8 years ago

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

Categories

(Tree Management Graveyard :: TBPL, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jrmuizel, Assigned: ewong)

References

Details

(Whiteboard: [sheriff-want][mentor=mbrubeck][lang=js][lang=php])

Attachments

(1 file, 7 obsolete files)

This would be handy to have.
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}/
OS: Mac OS X → All
Hardware: x86 → All
The last part isn't the revision actually but an ID I can't found :(
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.
Summary: Add "go to build" link for mozilla-central builds → Add "go to build" link for non-Try builds
Duplicate of this bug: 699624
Any chance the patches noted in #3 are now landed and we can fix this?
Assignee: nobody → 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?
> 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.
(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.
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.
Duplicate of this bug: 764836
Whiteboard: [sheriff-want]
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.
Assignee: mbrubeck → nobody
Whiteboard: [sheriff-want] → [sheriff-want][mentor=mbrubeck][lang=js][lang=php]
Duplicate of this bug: 784876
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attachment #654903 - Flags: review?(mbrubeck)
Attachment #654903 - Attachment is obsolete: true
Attachment #654903 - Flags: review?(mbrubeck)
Attachment #654907 - Flags: review?(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).
Attachment #654907 - Flags: review?(mbrubeck) → review-
Attachment #654907 - Attachment is obsolete: true
Attachment #654927 - Flags: review?(mbrubeck)
attaching the correct patch.
Attachment #654927 - Attachment is obsolete: true
Attachment #654927 - Flags: review?(mbrubeck)
Attachment #654929 - Flags: review?(mbrubeck)
Attachment #654929 - Flags: review?(mbrubeck) → review+
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.
Attachment #654929 - Flags: feedback-
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 '';
}
Attachment #654929 - Attachment is obsolete: true
Attachment #655930 - Flags: review?(mbrubeck)
Attachment #655930 - Flags: review?(bmo)
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 :-)
Attachment #655930 - Flags: review?(bmo) → review-
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
Attachment #655930 - Flags: review?(mbrubeck)
Attachment #655930 - Attachment is obsolete: true
Attachment #656300 - Flags: review?(bmo)
Attachment #656300 - Attachment is obsolete: true
Attachment #656300 - Flags: review?(bmo)
Attachment #656302 - Flags: review?
Attachment #656302 - Attachment is obsolete: true
Attachment #656302 - Flags: review?
Attachment #656314 - Flags: review?(bmo)
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 :-)
Attachment #656314 - Flags: review?(bmo) → review+
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Hasn't been pushed to production yet & there has been some confusion on IRC, so reopening for now.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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).
Depends on: 790559
In production.
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Depends on: 790939
Depends on: 841316
Product: Webtools → Tree Management
Product: Tree Management → Tree Management Graveyard
You need to log in before you can comment on or make changes to this bug.