Closed Bug 1283930 Opened 4 years ago Closed 4 years ago

Add Makefile.PL & local/lib/perl5 support to bmo/master


( :: General, defect)

Not set





(Reporter: dylan, Assigned: dylan)


(Blocks 1 open bug)



(1 file, 1 obsolete file)

44 bytes, text/x-github-pull-request
: review+
Details | Review
(starting out unconfirmed as maybe this isn't the approach we should take)

As it stands good progress has been made with the upstream merge.
Assuming we do some more testing, we'll be on track for rolling it out to scl3. 

I am a deeply paranoid man, and I worry that we'll miss something. There are something like 64k lines added, and a third of those removed between bmo/master and bmo/upstream-merge. Lots of room for things to be hiding.

A nicer approach would be an incremental migration of changes from upstream-merge into master until they are identical. That sounds nice on paper -- but the difficulty arises because of new library dependencies introduced.

Perhaps we can solve that? In upstream-merge (and upstream bugzilla) I added code to support a Makefile.PL, MYMETA* files and support for modules being installed in $BUGZILLA_DIR/local. We've been using this code in test and it is really quite nice. If *that* feature is backported to master, we could then start pulling in changes incrementally, as normal pushes.

A question arises about how to *install* things to local/.
The carton vendor tarballs we create are quite hairy to generate
and rely on subtle behavior (recompiling a perl interpreter that is the same as the system one). So discounting carton, can we solve this?

We can. We can commit a symlink 'local' pointing to data/local.
The data directory is shared between the nodes, including the admin node. Provided the nodes have the same version of *perl* and compatible versions of C libraries.

What are the tangible benefits of this aside from the incremental merging of upstream merge?

1) make use of Cache::Memcached::Fast. This is a *lot* faster than Cache::Memcached, and doubly so as we don't have to detaint data coming out of it.
2) Search::Elasticsearch (needed for the elasticsearch bug) could be installed without having to package an rpm
3) File::Spec could be upgraded to a faster version (the current version suffers a big performance bug)
4) dkl and I could solve more problems if we don't have to worry about "if we can use Module X".

I do also want new servers and software eventually. A non-threaded perl 5.24 would provide a great performance boost. But we shouldn't block on that. We should make the most of what is possible now :-)
Attached file the pull request in question (obsolete) —
Attachment #8767341 - Flags: review?(dkl)
Comment on attachment 8767341 [details] [review]
the pull request in question

Comments in
Attachment #8767341 - Flags: review?(dkl) → review-
Ever confirmed: true
Blocks: 1291051
Added some more comment to the github pull request conversation. r-

Attachment #8767341 - Flags: review- → review?(dkl)
Comment on attachment 8767341 [details] [review]
the pull request in question

In order to build a vendor tarball with this patch applied, I had to add to the Dockerfile I use:

RUN cpanm -l local XSLoader && vendor/bin/carton install --cached --deployment

but as discussed in IRC, this seems to be an issue with carton and not the BMO code itself.

All other issues seem to be addressed.

Attachment #8767341 - Flags: review?(dkl) → review+
Blocks: 1293689
Blocks: 1289886
Comment on attachment 8767341 [details] [review]
the pull request in question

Some scripts are missing the use lib "local" stuff.
Attachment #8767341 - Flags: review+ → review-
Depends on: 1293682
I should have known this, but under taint mode $FinBin::Bin is tainted and it cannot be used in "use lib" unless we also detaint it.
Fixed that problem, now it seems "use_for_bugs" crept into qa/config/
Summary: Add Makefile.PL & local/lib/perl5 support to bmo/master + local symlink to data/ directory → Add Makefile.PL & local/lib/perl5 support to bmo/master
Attachment #8767341 - Attachment is obsolete: true
Blocks: 1290580
Attached file revised patch
Attachment #8782099 - Flags: review?(dkl)
Comment on attachment 8782099 [details] [review]
revised patch


We can address the qa/t/*.t compile test issues in another bug.
Attachment #8782099 - Flags: review?(dkl) → review+
No longer blocks: 1290580
   591e2da..e6bf4ca  master -> master
Closed: 4 years ago
Resolution: --- → FIXED
   e6bf4ca..14bcdce  master -> master

Backed out for our mozreview friends, whom we keep pooping on.
Resolution: FIXED → ---
Blocks: 1304160
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.