Closed Bug 1731594 Opened 4 years ago Closed 4 years ago

Update pdf.js through updatebot

Categories

(Firefox :: PDF Viewer, task)

Product:
Firefox
Component:
PDF Viewer
Type:
task

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: marco, Assigned: tjr)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1731594: Update the updatebot dockerfile to prepare for pdf.js r?jewilde
48 bytes, text/x-phabricator-request
Details | Review
Bug 1731594: Add an update task for pdf.js r?calixte
48 bytes, text/x-phabricator-request
Details | Review

Currently pdf.js update is a tedious manual process. It would be nice to instead automate it with updatebot.

We might want to run try pushes periodically to ensure things are working well, but then only open a bug on a tag or on a less frequent cadence

From Brendan:

The current process is pretty hacky, but here are my steps:

In pdf.js github repo

  • git reset --hard && git remote update && git checkout upstream/master && npm install && gulp mozcentral && node ~/projects/utils/update_pdfjs.js
  • generate release notes for bugzilla: python ~/projects/utils/release.py -n 8 XXX (where XXX is the last github pull request number that went into the previous release)
  • for testing I usually just run ./mach build && ./mach mochitest toolkit/components/pdfjs/test/ && ./mach mochitest browser_parsable_css.js

The two scripts can be found in https://drive.google.com/drive/folders/1Tv82Ubbcb2TBI3qWdgiqUdeJU_OaWxwm?usp=sharing

Tom, June, is this something you could help with?

Flags: needinfo?(tom)
Flags: needinfo?(jewilde)

I've started looking at this. The amount of things that happen as part of node install and gulp mozcentral seem pretty substantial. (Including downloading the latest Firefox nightly for some reason?) I'm wondering if there's anyway we can minimize the amount of things that happen there. For example pre-loading all the npm dependencies in the docker image would help....

Assignee: nobody → tom
Flags: needinfo?(tom)
Flags: needinfo?(jewilde)

Alright, I've gotten it to work successfully, so I think we're in good shape here. Just have to rebase and refine and then figure out what we want to do in terms of frequency.

https://bugzilla-dev.allizom.org/show_bug.cgi?id=1631749
https://treeherder.mozilla.org/jobs?repo=try&revision=c2dfb4c95b52065e8731dd35a4ffbba05c6fe1db

Depends on D142366

(In reply to Tom Ritter [:tjr] (ni? for response to CVE/sec-approval/advisories/etc) from comment #2)

I've started looking at this. The amount of things that happen as part of node install and gulp mozcentral seem pretty substantial. (Including downloading the latest Firefox nightly for some reason?) I'm wondering if there's anyway we can minimize the amount of things that happen there. For example pre-loading all the npm dependencies in the docker image would help....

Firefox is downloaded in order to run some tests.

(In reply to Tom Ritter [:tjr] (ni? for response to CVE/sec-approval/advisories/etc) from comment #3)

Alright, I've gotten it to work successfully, so I think we're in good shape here. Just have to rebase and refine and then figure out what we want to do in terms of frequency.

https://bugzilla-dev.allizom.org/show_bug.cgi?id=1631749
https://treeherder.mozilla.org/jobs?repo=try&revision=c2dfb4c95b52065e8731dd35a4ffbba05c6fe1db

I think the first comment in the bug that updatebot opens looks good. We mainly need the PR title, and it is there (we mention the bug ID linked to the PR in its title, so the autonag bot can mark the update bug as blocking the bugs that are fixed by the PRs part of the update).

For the frequency, does it have to be defined in terms of days or can it be defined in terms of commits/PRs?

(In reply to Calixte Denizet (:calixte) from comment #6)

(In reply to Tom Ritter [:tjr] (ni? for response to CVE/sec-approval/advisories/etc) from comment #2)

I've started looking at this. The amount of things that happen as part of node install and gulp mozcentral seem pretty substantial. (Including downloading the latest Firefox nightly for some reason?) I'm wondering if there's anyway we can minimize the amount of things that happen there. For example pre-loading all the npm dependencies in the docker image would help....

Firefox is downloaded in order to run some tests.

Do those tests need to be run at vendoring time? Are they/can they be run on try?

(In reply to Marco Castelluccio [:marco] from comment #7)

(In reply to Tom Ritter [:tjr] (ni? for response to CVE/sec-approval/advisories/etc) from comment #3)

Alright, I've gotten it to work successfully, so I think we're in good shape here. Just have to rebase and refine and then figure out what we want to do in terms of frequency.

https://bugzilla-dev.allizom.org/show_bug.cgi?id=1631749
https://treeherder.mozilla.org/jobs?repo=try&revision=c2dfb4c95b52065e8731dd35a4ffbba05c6fe1db

I think the first comment in the bug that updatebot opens looks good. We mainly need the PR title, and it is there (we mention the bug ID linked to the PR in its title, so the autonag bot can mark the update bug as blocking the bugs that are fixed by the PRs part of the update).

This reminds me that I did not integrate the release.py script to generate a nice commit message. But if what is happening now looks good that is great and less work.

For the frequency, does it have to be defined in terms of days or can it be defined in terms of commits/PRs?

I can fairly easily make it every N new commits. How many commits before an update would you like?

Flags: needinfo?(mcastelluccio)
Flags: needinfo?(cdenizet)

Looking at:
https://bugzilla-dev.allizom.org/show_bug.cgi?id=1631749
I think we must filter out merge PRs: they're useless and they add some noise.

About the frequency: every week if more than 3 commits in the week (except merge).
About running tests: tests are ran on every PR so I think useless to do them one more time.

About the phab commit message, is it possible to keep something like Bug XXX - Update pdf.js to version ${version} (like we've at the end of update_pdfjs.js script) ?

Flags: needinfo?(cdenizet)
Flags: needinfo?(mcastelluccio)
Depends on: 1764976
  • I've added support for waiting until N new commits have occured and then further support for waiting to satisfy both at least N weeks since the last job and M new commits
  • I've excluded merge commits from the count of commits and the bug description
  • Changing the commit message (to add the word 'version') is actually more trouble than it seems because the verification script has t be able to parse it; and landing an updatebot bump and the verification script is a flag day type thing - I figured not having 'version' isn't that big a deal?
  • Instead of npm install / gulp mozcentral is there an argument to omit the download of FF Nightly and tests run on it during the vendoring process, so they can just run during PR merging?

But I think we're pretty close.

Flags: needinfo?(cdenizet)
Keywords: leave-open
Pushed by tritter@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5158c730e64f Update the updatebot dockerfile to prepare for pdf.js r=jewilde

Firefox download is done thanks to:
https://github.com/mozilla/pdf.js/blob/2be19e828fcf9ce7740a2136087da028dff56e00/package.json#L69

so in order to avoid it, passing --ignore-scripts to npm should be enough.

Flags: needinfo?(cdenizet)
Attachment #9269908 - Attachment description: WIP: Bug 1731594: Add an update task for pdf.js → Bug 1731594: Add an update task for pdf.js r?calixte

(In reply to Tom Ritter [:tjr] (sec-approvals paused as we're past last uplift) from comment #14)

Okay here is the latest run:

https://phabricator-dev.allizom.org/D3289

Looking at that patch, it doesn't appear that the update scripts are able to correctly deal with files being deleted.

Note that as part of the changes in that patch the toolkit/components/pdfjs/content/web/images/toolbarButton-openFile.svg file should have been removed, since it's no longer being included when the gulp mozcentral command is run.

You're right. I fixed that by being a lot more explicit about what we copy, and for the two directories using rsync's --delete option. Here's a new run:

https://bugzilla-dev.allizom.org/show_bug.cgi?id=1632010
https://phabricator-dev.allizom.org/D3305
https://treeherder.mozilla.org/jobs?repo=try&revision=f2fb80fcf37b2b87816eeb24844082980fdb070d

(In reply to Calixte Denizet (:calixte) from comment #17)

Is it a bot error: https://bugzilla-dev.allizom.org/show_bug.cgi?id=1632010#c1 ?

No; but it can be improved.

  1. Updatebot will attempt to edit moz.build files when it add/removes files from the repository. It is somewhat successful in doing that, depending on the complexity of the moz.build file. If it cannot figure it out, it will report a warning and put the entire contents of the command into a comment. The relevant line is Could not remove toolkit/components/pdfjs/content/web/images/toolbarButton-openFile.svg from the appropriate moz.build file
  2. The entire output of the update script is included in that comment.

I can improve this by making the rsync commands quiet (so we don't see the files it copies) or since it looks like pdf.js moz.build file isn't really going to change based on pdfjs updates, I can just tell it to skip the moz.build editing step. (I edited the patch to do the latter)

Would it be possible to use this set of tests for the try:
https://hg.mozilla.org/try/rev/07f31fe1578e0cb1bd266209d43b9a202714e5f6

And is there a way to trigger a release ourselves ?

without this test:
https://hg.mozilla.org/try/rev/07f31fe1578e0cb1bd266209d43b9a202714e5f6#l1.39
there is no need to have a pgo build.

Depends on: 1769307

This try run https://treeherder.mozilla.org/jobs?repo=try&revision=753229c88319359e7755ae0da965cf294ea8106a encompasses those tests and some additional platforms, does that work?

There is not a way to poke Updatebot to trigger a release currently.

Flags: needinfo?(cdenizet)

Is it possible to only run tests with opt and debug builds ?
pdf.js by itself is well-tested upstream and we've only few tests in m-c, so running on every possible builds is almost useless.

Flags: needinfo?(cdenizet)

Yes; I've updated the patch to do so.

Pushed by tritter@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b31bacc0d075 Add an update task for pdf.js r=calixte
Depends on: 1770895

I think we can call this fixed now. Thanks Tom for taking care of this!

Status: NEW → RESOLVED
Closed: 4 years ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: