Closed Bug 1487332 Opened Last year Closed 11 months ago

Re-sort the includes in the Telemetry source files

Categories

(Toolkit :: Telemetry, enhancement, P3)

enhancement
Points:
1

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox63 --- wontfix
firefox64 --- fixed

People

(Reporter: Dexter, Assigned: spiro, Mentored)

References

Details

(Whiteboard: [lang=c++][good-first-bug])

Attachments

(1 file, 1 obsolete file)

Bug 1484611 moved the location of a few Telemetry files within their source directory. As a consequence of that, includes in the implementation files are now unsorted.

This bug is about fixing the style of the includes, making them sorted again.
Depends on: 1484611
In order to fix this:

- go through all the .cpp/.h files in toolkit/components/telemetry (+ subdirectories);
- sort all the #include directives according to the logic at [1];
- build using ./mach build
- make sure all the tests pass with ./mach test toolkit/components/telemetry

[1] - https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#CC_practices
Mentor: alessio.placitelli
Points: --- → 1
Priority: -- → P3
Whiteboard: [lang=c++][good-first-bug]
can I take up this bug. This would be my second!
(In reply to Divyansh Sharma [:spiro] from comment #2)
> can I take up this bug. This would be my second!

Hi, unfortunately the blocker bug has not landed, so this bug is not actionable yet. It will be in a couple of days or so.
(In reply to Alessio Placitelli [:Dexter] from comment #3)
> (In reply to Divyansh Sharma [:spiro] from comment #2)
> > can I take up this bug. This would be my second!
> 
> Hi, unfortunately the blocker bug has not landed, so this bug is not
> actionable yet. It will be in a couple of days or so.

Hi :spiro, the dependency just landed so this is good to work on now :)
Assignee: nobody → sharma.divyansh.501
Do I have to download latest code of firefox for build?
Flags: needinfo?(alessio.placitelli)
(In reply to Divyansh Sharma [:spiro] from comment #5)
> Do I have to download latest code of firefox for build?

Yes, you do.
Flags: needinfo?(alessio.placitelli)
Hi :Dexter, :spiro, There are almost 20 .h and 20.cpp files. Can I do some part (say .h one)?
PS: I am newbie here and looking to fix first bug :)
Flags: needinfo?(alessio.placitelli)
(In reply to Anushi Maheshwari[:Anushi1998] from comment #7)
> Hi :Dexter, :spiro, There are almost 20 .h and 20.cpp files. Can I do some
> part (say .h one)?
> PS: I am newbie here and looking to fix first bug :)

Hi :Anushi1998,

while these might seem a lot of files, not all of them require re-ordering. Moreover, we want all the changes to be in a single changeset, so this doesn't naturally fit into splitting.

Would you consider taking bug 1473603 instead?
Flags: needinfo?(alessio.placitelli)
:Dexter Sure, thanks :)
no activity for 6 days, can I take this bug or is someone working?
:gurungrahul: I have done half work and was waiting for Dexter to unassign. Will it be good if I take this? :)
Flags: needinfo?(gurungrahul2)
Flags: needinfo?(alessio.placitelli)
sure.
Flags: needinfo?(gurungrahul2)
Divyansh, how is this progressing? Are you blocked and need help?
Flags: needinfo?(alessio.placitelli) → needinfo?(sharma.divyansh.501)
I am almost finished. Will submit a patch today or latest by tomorrow.
Flags: needinfo?(sharma.divyansh.501)
Alessio

I am unable to run tests. It is my error log : https://pastebin.com/W3xRwGq6. How to resolve this?
Flags: needinfo?(alessio.placitelli)
:spiro Looks like this is unrelated to your changes. I have the same error report :)
It may be the case that command to run should be different or maybe some bug ;)
Flags: needinfo?(sharma.divyansh.501)
Bug 1487332 -  Re-sorted the header files of .cpp/.h telemetry files.
Flags: needinfo?(sharma.divyansh.501)
(In reply to Divyansh Sharma [:spiro] from comment #15)
> Alessio
> 
> I am unable to run tests. It is my error log :
> https://pastebin.com/W3xRwGq6. How to resolve this?

Hey :spiro, this looks unrelated :)
Flags: needinfo?(alessio.placitelli)
Dexter

Is 'nspr' the only change that is needed now? If not, then how to determine where does 'include' look for files?
Flags: needinfo?(alessio.placitelli)
(In reply to Divyansh Sharma [:spiro] from comment #19)
> Dexter
> 
> Is 'nspr' the only change that is needed now? If not, then how to determine
> where does 'include' look for files?

No, that was an example: we don't need to change any path in this patch. We need to swap the position of include files so that are in order, like described in comment 1.
Flags: needinfo?(alessio.placitelli)
Dexter

You are saying that we need to re-order include files in alphabetical order, right?
Flags: needinfo?(alessio.placitelli)
(In reply to Divyansh Sharma [:spiro] from comment #21)
> Dexter
> 
> You are saying that we need to re-order include files in alphabetical order,
> right?

Almost. I'm saying that it needs to be both alphabetically sorted AND following the criteria from https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#CC_practices , as discussed in comment 1.
Flags: needinfo?(alessio.placitelli)
Dexter

I am unable to run test files. I am encountering some errors.

Error log : https://pastebin.com/vTHGCB6x

Please help!
Flags: needinfo?(alessio.placitelli)
(In reply to Divyansh Sharma [:spiro] from comment #23)
> Dexter
> 
> I am unable to run test files. I am encountering some errors.
> 
> Error log : https://pastebin.com/vTHGCB6x
> 
> Please help!

Hey, this doesn't look related. Unfortunately I can't be sure without taking a look at the patch :) Would you mind uploading the most recent one?
Flags: needinfo?(alessio.placitelli)
Dexter

In some files, I needed to include telemetryCommon.h [or some similar files] above all the include files, otherwise stdint.h would not have been included and uint64_t would not be recognised.

I am uploading the latest diff.
Flags: needinfo?(alessio.placitelli)
Dexter

Please ignore current diff. It has got some extra files included. I will separate them and update the diff.
Flags: needinfo?(alessio.placitelli)
(In reply to Divyansh Sharma [:spiro] from comment #26)
> Dexter
> 
> Please ignore current diff. It has got some extra files included. I will
> separate them and update the diff.

I took a look anyway and left a few comments :)
In some files, I need to place Telemetry.h before main header file, else it throws error about some undeclared variables. Like:

22:39.43 In file included from /home/inspiro/src/mercurial/toolkit/components/telemetry/core/TelemetryEvent.cpp:7:
22:39.43 /home/inspiro/src/mercurial/toolkit/components/telemetry/core/TelemetryEvent.h:33:1: error: unknown type name 'nsresult'
22:39.43 nsresult RecordEvent(const nsACString& aCategory, const nsACString& aMethod,
22:39.43 ^
22:39.43 /home/inspiro/src/mercurial/toolkit/components/telemetry/core/TelemetryEvent.h:33:28: error: unknown type name 'nsACString'
22:39.43 nsresult RecordEvent(const nsACString& aCategory, const nsACString& aMethod,
Flags: needinfo?(alessio.placitelli)
Attached file Bug 1487332 [PATCH] (obsolete) —
Bug 1487332 - Resorted the include files as per coding standards
Attachment #9013266 - Attachment description: bug 1487332 rep → Bug 1487332 [PATCH]
Alessio

I have created a new diff with asked changes. But it has some issues for you to consider:

1) File 'TelemetryEvent.cpp' has include 'Telemetry.h' before 'TelemetryEvent.h', else it throws error described in earlier comment 28.

2) Some tests are failing now too, as per comment 23.

Have a look into this.

Thanks :)
(In reply to Divyansh Sharma [:spiro] from comment #30)
> Alessio
> 
> I have created a new diff with asked changes. But it has some issues for you
> to consider:

Mh, can you update the previous one? Creating a request makes it hard to track down the requested changes. Updating the original patch simplifies things quite a lot.

> 
> 1) File 'TelemetryEvent.cpp' has include 'Telemetry.h' before
> 'TelemetryEvent.h', else it throws error described in earlier comment 28.

Where is this happening? Can you comment on phabricator?

> 2) Some tests are failing now too, as per comment 23.

This still looks unrelated to me.
Flags: needinfo?(alessio.placitelli)
Attachment #9013266 - Attachment is obsolete: true
Keywords: checkin-needed
Hi, We received the following error when trying to land this patch:

With Diff 22167 on Tue, October 9, 2018, 4:06 PM GMT+3, by rvandermeulen@mozilla.com.
Error: hg error in cmd: hg push -r tip upstream: pushing to ssh://hg.mozilla.org/integration/autoland searching for changes remote: adding changesets remote: adding manifests remote: adding file changes remote: added 1 changesets with 26 changes to 26 files remote: remote: remote: ************************** ERROR **************************** remote: Rev 9d3da511671f contains git-format-patch "[PATCH]" cruft. Use git-format-patch -k to avoid this. remote: divyansh <sharma.divyansh.501@iitg.ernet.in> remote: Bug 1487332 [PATCH] r=Dexter remote: remote: Bug 1487332 - Re-sorted the header files of .cpp/.h telemetry files. remote: remote: Differential Revision: https://phabricator.services.mozilla.com/D5840 remote: ************************************************************* remote: remote: remote: transaction abort! remote: rollback completed remote: pretxnchangegroup.c_commitmessage hook failed abort: push failed on remote 

spiro: Can you please have a look at this and update the patch format? Thanks!
Flags: needinfo?(sharma.divyansh.501)
Keywords: checkin-needed
Attachment #9009014 - Attachment description: Bug 1487332 [PATCH] → Bug 1487332 - Re-sorted the header files of .cpp/.h telemetry files.
Flags: needinfo?(sharma.divyansh.501)
Keywords: checkin-needed
Pushed by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fd465ab13d0f
Re-sorted the header files of .cpp/.h telemetry files. r=Dexter
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fd465ab13d0f
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.