1237983 - Investigate and remove the Bagheera Client implementation

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect, P3)

Description

[Alessio Placitelli [:Dexter]]

With FHR going away, we should figure out if it makes sense to keep the Bagheera client [0] (and server) or if it's ok to remove it.

[0] - https://dxr.mozilla.org/mozilla-central/rev/d4213241bb796fdfa7a5ad4f1989e97b44474364/services/common/bagheeraclient.js

Comment 1

[Georg Fritzsche [:gfritzsche]]

Per dxr i don't see any users besides FHR:
https://dxr.mozilla.org/mozilla-central/search?q=bagheeraclient.js&redirect=false&case=true

Are there any other concerns here?

Comment 2

[Alessio Placitelli [:Dexter]]

No. We should probably remove the bagheera server as well.

Comment 3

[Alessio Placitelli [:Dexter]]

Brendant, would you be interested in taking this bug? (Details will follow in the next comment)

Comment 4

[Alessio Placitelli [:Dexter]]

We should be removing:

- The bagheeraclient.js implementation, along with all of its references and test coverage, making sure that nothing breaks.
- The bagheeraserver implementation, along with all of is references and test coverage.

Everything seems to live in services/common, so this looks like a good place to start looking.

To make sure that everything works as expected:

./mach build services
./mach test services

Comment 5

[Brendan Good]

(In reply to Alessio Placitelli [:Dexter] from comment #3)
> Brendant, would you be interested in taking this bug? (Details will follow
> in the next comment)

Hi Alessio, I will definitely take a look into this bug. It sounds like a good second step!

Comment 6

[Alessio Placitelli [:Dexter]]

Brendan, are you still interested in working on this bug?

Comment 7

[Brendan Good]

(In reply to Alessio Placitelli [:Dexter] from comment #6)
> Brendan, are you still interested in working on this bug?

Hi Alessio:

Sorry for the interval without updates; I tried doing this without any assistance for a while and then other things started to take over. 

I'm still interested in working on this bug and I have a one main question for now:

What does removing the bagheeraclient.js implementation and all of its references entail exactly? Is it a matter of 
removing lines 58 to the end or 58 to 80 on this file?
https://dxr.mozilla.org/mozilla-central/rev/d4213241bb796fdfa7a5ad4f1989e97b44474364/services/common/bagheeraclient.js#58 (I'm unsure whether the prototype whose definition starts on line 81 counts as an implementation) and then check for errors in the build/tests?

Sorry again for the delay and, now that the other events have sorted themselves out, this shall have my full attention until it's resolved!

Comment 8

[Alessio Placitelli [:Dexter]]

(In reply to Brendan Good from comment #7)
> What does removing the bagheeraclient.js implementation and all of its
> references entail exactly? Is it a matter of 
> removing lines 58 to the end or 58 to 80 on this file?
> https://dxr.mozilla.org/mozilla-central/rev/
> d4213241bb796fdfa7a5ad4f1989e97b44474364/services/common/bagheeraclient.
> js#58 (I'm unsure whether the prototype whose definition starts on line 81
> counts as an implementation) and then check for errors in the build/tests?

No problem for the delay! I just wanted to be sure you were still interested :)

Removing bagheeraclient.js means physically removing the file from the repository. This must be done using the "hg remove" command followed by the path to the file. 

The file itself is referenced in a couple of other locations, which need to be taken care of (e.g. removing the test file and removing the references from the other files).

Once this is done, build and check that it's correctly working by issuing ./mach test services/

[1] - https://dxr.mozilla.org/mozilla-central/search?q=bagheeraclient.js+-path%3Aobj&redirect=false&case=false

Comment 9

[Brendan Good]

Attachment: Investigate and remove the Bagheera Client Implementation

I had to perform a hg commit --amend (since I forgot that you explicitly said
to remove the bagheeraserver implementation as well when I did my first commit).
One test did fail, but the reason for the test failing seemed identical for the
tests failing on the last bug I worked on (trying to connect to an invalid address)
but everything else seems to work fine.

Comment 10

[Brendan Good]

Attachment: Investigate and remove the Bagheera Client Implementation

I resubmitted this patch to remove lines which were previously just commented out.

Comment 11

[Alessio Placitelli [:Dexter]]

Comment on attachment 8711536 [details] [diff] [review]
Investigate and remove the Bagheera Client Implementation

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

Thank you Brendan, this looks almost there! There's only one thing: since we're removing bagheeraserver.js, we should make sure to remove its uses as well [1].

To make sure you got all the references, look for bagheeraclient or bagheeraserver on DXR (dxr.mozilla.org). Ignore the results in "obj-*", or simply add "-path:obj" to the search query so they don't show up.

[1] - https://dxr.mozilla.org/mozilla-central/rev/c2256ee8ae9a8ee0bf7ab49a8b1924720d846cc7/services/common/tests/mach_commands.py#37,114-117

Comment 12

[Brendan Good]

Attachment: Investigate and remove the Bagheera Client Implementation

I did a grep search through the whole project for "bagheera" locally as well as
the dxr search (I, surprisingly, got slightly different results even after
accounting for the files I've already deleted) and found one more file which
just tests running the bagheera server so I deleted it as well.

Comment 13

[Alessio Placitelli [:Dexter]]

Comment on attachment 8712013 [details] [diff] [review]
Investigate and remove the Bagheera Client Implementation

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

This looks good to me now, thanks. Georg Fritzsche should be reviewing this, so bouncing the review to him!

Comment 14

[Richard Newman [:rnewman]]

Removing this means we don't need to make it eslintable: Bug 1242965.

Comment 15

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8712013 [details] [diff] [review]
Investigate and remove the Bagheera Client Implementation

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

Thanks!

Comment 16

[Georg Fritzsche [:gfritzsche]]

Alessio, can you push this to try and follow up on the results?

Comment 17

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe33208f940b

Comment 18

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/fx-team/rev/1686b901daa4eb42e6bbae2e9ba23c9754c584da
Bug 1237983 - Investigate and remove the Bagheera Client Implementation. r=gfritzsche

Comment 19

[Carsten Book [:Tomcat]]

backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=6886551&repo=fx-team

error is :

Error: /builds/slave/fx-team-m64-000000000000000000/build/src/browser/installer/package-manifest.in:619: File missing in ../../dist: Nightly.app/Contents/Resources/modules/services-common/bagheeraclient.js
 00:26:56     INFO -  Traceback (most recent call last):
 00:26:56     INFO -    File "/builds/slave/fx-team-m64-000000000000000000/build/src/toolkit/mozapps/installer/packager.py", line 410, in <module>
 00:26:56     INFO -      main()
 00:26:56     INFO -    File "/builds/slave/fx-team-m64-000000000000000000/build/src/toolkit/mozapps/installer/packager.py", line 362, in main
 00:26:56     INFO -      copier.add(mozpath.join(respath, 'removed-files'), removals)
 00:26:56     INFO -    File "/tools/python/lib/python2.7/contextlib.py", line 24, in __exit__
 00:26:56     INFO -      self.gen.next()
 00:26:56     INFO -    File "/builds/slave/fx-team-m64-000000000000000000/build/src/python/mozbuild/mozpack/errors.py", line 131, in accumulate
 00:26:56     INFO -      raise AccumulatedErrors()
 00:26:56     INFO -  mozpack.errors.AccumulatedErrors

Comment 20

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/7581aa0f335f

Comment 21

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/fx-team/rev/a24f3a35d1c88c199af1693d82d9dafc70b3c4a2
Bug 1237983 - Investigate and remove the Bagheera Client Implementation. r=gfritzsche

Comment 22

[Alessio Placitelli [:Dexter]]

Ad discussed over IRC, this could be due to the patch needing a clobber. Trying that.

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a24f3a35d1c8