1150312 - Check if MOZ_SHARK is still used and if not remove it

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Robert Strong (they/them - no direct email)]

I asked a couple of people that are in the know and no one knew whether MOZ_SHARK is still needed. If it isn't it then it can be removed.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Probably worth asking dev.platform first, but if no one jumps up and down we should just remove it and see who complains :)

Comment 2

[Robert Strong (they/them - no direct email)]

done

Comment 3

[Mike Hoye [:mhoye]]

Still seeing email going by about this: can I get a go/no-go call on this so I can surface it to the community?

Comment 4

[(not currently active) Ted Mielczarek]

I haven't heard anything but speculation that someone somewhere might possibly be using it. In the absence of evidence I say we remove it.

Comment 5

[Robert Strong (they/them - no direct email)]

As I understand it there are also alternatives. Go for it

Comment 6

[Robert Strong (they/them - no direct email)]

If you or someone else would prefer to mentor this feel free to take over

Comment 7

[Mike Hommey [:glandium]]

Sheppy asked in the dev-platform thread whether the tenfourfox people should be contacted for this, and there is evidence that the tenfourfox people have seen the thread (http://tenfourfox.blogspot.jp/2015/04/beware-undead-snow-leopard.html), so I guess they would have complained if they were actively using shark. That said, better safe than sorry, so let's get an explicit statement. Cameron?

Comment 8

[Cameron Kaiser [:spectre]]

Yes, I'm aware. The MOZ_SHARK code appears to be specific to actually controlling Shark, whereas I just use Shark to attach to a running application and take samples, so I don't think removing it should affect TenFourFox work. However, I'd prefer this was not backported to 38ESR just in case, since I'm actively working on that port.

Comment 9

[Giovanny Gongora [:gioyik]]

Hi,

I am seeing many MOZ_SHARK on source code [1]. I want to make a patch for this, so could you tell me which one should be removed?

Thanks

[1]: http://mxr.mozilla.org/mozilla-central/search?string=MOZ_SHARK

Comment 10

[Robert Strong (they/them - no direct email)]

MOZ_SHARK is no longer needed so all.

Comment 11

[Giovanny Gongora [:gioyik]]

Attachment: 1150312.patch

Comment 12

[Giovanny Gongora [:gioyik]]

Hi Robert,

I submitted a patch. Could you review it and tell me if is ok?

Thanks

Comment 13

[Robert Strong (they/them - no direct email)]

Comment on attachment 8638094 [details] [diff] [review]
1150312.patch

Nice... looks good to me.

Let's also have a build peer review this.

Comment 14

[Giovanny Gongora [:gioyik]]

Thanks, I will assign this bug to myself to keep it following.

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 8638094 [details] [diff] [review]
1150312.patch

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

Thank you for your patch.

"Check if MOZ_SHARK is still used and if not remove it" does not describe what the change is about. Please change this to e.g. "Remove MOZ_SHARK".

::: js/src/configure.in
@@ -3231,5 @@
> -    MOZ_PROFILING=1
> -    AC_DEFINE(MOZ_SHARK)
> -fi
> -
> -dnl ========================================================

There's an AC_SUSBT(MOZ_SHARK) leftover at line 3702.

Comment 16

[Giovanny Gongora [:gioyik]]

Attachment: 1150312.patch

Updated patch

Comment 17

[Giovanny Gongora [:gioyik]]

Hi Mike,

I added the change you mentioned in the comment and changed the description in the patch. I usually put as description bug title, but I have not problem to change with a more descriptive text.

Let me know if this one is ok and good to land.

Thanks

Comment 18

[Giovanny Gongora [:gioyik]]

So, should I add checkin-needed keyword now? or there's something else to do?

Comment 19

[Robert Strong (they/them - no direct email)]

That works and thanks again!

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/926651591417

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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