Closed Bug 1237983 Opened 4 years ago Closed 4 years ago

Investigate and remove the Bagheera Client implementation

Categories

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

defect

Tracking

(firefox46 affected, firefox47 fixed)

RESOLVED FIXED
Firefox 47
Tracking Status
firefox46 --- affected
firefox47 --- fixed

People

(Reporter: Dexter, Assigned: brendantgood, Mentored)

References

Details

(Whiteboard: [measurement:client])

Attachments

(1 file, 2 obsolete files)

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
Blocks: 1209088
Whiteboard: [measurement:client]
Priority: -- → P3
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?
Flags: needinfo?(alessio.placitelli)
No. We should probably remove the bagheera server as well.
Flags: needinfo?(alessio.placitelli)
Brendant, would you be interested in taking this bug? (Details will follow in the next comment)
Flags: needinfo?(brendantgood)
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
(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!
Flags: needinfo?(brendantgood)
Mentor: alessio.placitelli
Brendan, are you still interested in working on this bug?
Flags: needinfo?(brendantgood)
(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!
Flags: needinfo?(brendantgood)
(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
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.
Attachment #8711535 - Flags: review?(alessio.placitelli)
I resubmitted this patch to remove lines which were previously just commented out.
Attachment #8711536 - Flags: review?(alessio.placitelli)
Attachment #8711535 - Attachment is obsolete: true
Attachment #8711535 - Flags: review?(alessio.placitelli)
Assignee: nobody → brendantgood
Status: NEW → ASSIGNED
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
Attachment #8711536 - Flags: review?(alessio.placitelli) → feedback+
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.
Attachment #8712013 - Flags: review?(alessio.placitelli)
Attachment #8711536 - Attachment is obsolete: true
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!
Attachment #8712013 - Flags: review?(gfritzsche)
Attachment #8712013 - Flags: review?(alessio.placitelli)
Attachment #8712013 - Flags: feedback+
Removing this means we don't need to make it eslintable: Bug 1242965.
Blocks: 1242965
Comment on attachment 8712013 [details] [diff] [review]
Investigate and remove the Bagheera Client Implementation

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

Thanks!
Attachment #8712013 - Flags: review?(gfritzsche) → review+
Alessio, can you push this to try and follow up on the results?
Flags: needinfo?(alessio.placitelli)
Flags: needinfo?(alessio.placitelli)
https://hg.mozilla.org/integration/fx-team/rev/1686b901daa4eb42e6bbae2e9ba23c9754c584da
Bug 1237983 - Investigate and remove the Bagheera Client Implementation. r=gfritzsche
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
Flags: needinfo?(brendantgood)
https://hg.mozilla.org/integration/fx-team/rev/a24f3a35d1c88c199af1693d82d9dafc70b3c4a2
Bug 1237983 - Investigate and remove the Bagheera Client Implementation. r=gfritzsche
Ad discussed over IRC, this could be due to the patch needing a clobber. Trying that.
Flags: needinfo?(brendantgood)
https://hg.mozilla.org/mozilla-central/rev/a24f3a35d1c8
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.