Closed Bug 1215238 Opened 6 years ago Closed 6 years ago

Mention the included file path in pre-processed js sources with #includes

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox45 fixed)

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: chmanchester, Assigned: chmanchester)

References

Details

Attachments

(1 file)

For instance, https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-unknown-linux-gnu/_tests/reftest/b2g_start_script.js doesn't mention reftest-preferences.js (which is where all those prefs came from).
Summary: Mention the included file path in pre-processed sources with #includes → Mention the included file path in pre-processed js sources with #includes
Bug 1215238 - Mention the included filepath in pre-processed js sources with #includes.
Attachment #8678397 - Flags: review?(gps)
https://reviewboard.mozilla.org/r/23199/#review20977

::: python/mozbuild/mozbuild/preprocessor.py:769
(Diff revision 1)
> +            self.writtenLines = -1

It seems to me this could, in some cases, add a //@line comment at the beginning of the original file.

Also, it looks to me like the whole thing with "writtenLines" is broken, and that you can find a corner case where a //@line comment wouldn't be printed out when "returning" from an include if the line numbers match somehow.

I think we should instead track the tuple (file, line), and output //@line when that doesn't match what was for the last printed out line.
Attachment #8678397 - Flags: review?(gps)
https://reviewboard.mozilla.org/r/23199/#review20977

> It seems to me this could, in some cases, add a //@line comment at the beginning of the original file.
> 
> Also, it looks to me like the whole thing with "writtenLines" is broken, and that you can find a corner case where a //@line comment wouldn't be printed out when "returning" from an include if the line numbers match somehow.
> 
> I think we should instead track the tuple (file, line), and output //@line when that doesn't match what was for the last printed out line.

A //@ line would be added at the beginning of a file if a file name were passed as a string to do_include, but this doesn't happen in the build system.

I couldn't replicate the bug proposed because I guess the "#include" line itself is what causes the mismatch when "returning" from the included file. Tracking a tuple still seems a lot clearer, I'll give it a go.
Comment on attachment 8678397 [details]
MozReview Request: Bug 1215238 - Mention the included filepath in pre-processed js sources with #includes. r=glandium

Bug 1215238 - Mention the included filepath in pre-processed js sources with #includes. r=glandium
Attachment #8678397 - Attachment description: MozReview Request: Bug 1215238 - Mention the included filepath in pre-processed js sources with #includes. → MozReview Request: Bug 1215238 - Mention the included filepath in pre-processed js sources with #includes. r=glandium
Attachment #8678397 - Flags: review?(mh+mozilla)
Attachment #8678397 - Flags: review?(mh+mozilla)
Comment on attachment 8678397 [details]
MozReview Request: Bug 1215238 - Mention the included filepath in pre-processed js sources with #includes. r=glandium

https://reviewboard.mozilla.org/r/23199/#review21821

::: python/mozbuild/mozbuild/preprocessor.py:410
(Diff revision 2)
> +    def updatedLineInfo(self, line, filename):

updatedLineInfo is a weird name for this function, but I don't have any better suggestion to give. OTOH, the function is only used once, so how about inlining it?

    if self.checkLineNumbers:
        expected_file, expected_line = self.line_info
        if expected_line + 1 != next_line or expected_file and expected_file != next_file:
             ...

::: python/mozbuild/mozbuild/preprocessor.py:767
(Diff revision 2)
> +        self.noteLineInfo()

This is too early for this one. It seems to me this should happen where the writtenLines used to be set.

::: python/mozbuild/mozbuild/preprocessor.py:786
(Diff revision 2)
> +        self.noteLineInfo()

This doesn't seem necessary.

::: python/mozbuild/mozbuild/test/test_preprocessor.py:590
(Diff revision 2)
> +                             ('//@line 1 "CWDfoo.js"\n'
> +                              'bazfoobar\n'
> +                              '//@line 2 "CWDbar.js"\n'
> +                              '@foo@\n'
> +                              '//@line 3 "CWDfoo.js"\n'
> +                              'bazbarfoo\n'
> +                              '//@line 2 "CWDbar.js"\n'
> +                              'foobarbaz\n'
> +                              '//@line 3 "CWDtest.js"\n'
> +                              'barfoobaz\n').replace('CWD',

replace CWD with CWD/, that will make things easier to get when just glancing over the code.
https://reviewboard.mozilla.org/r/23199/#review21821

> This is too early for this one. It seems to me this should happen where the writtenLines used to be set.

Putting this where writtenLines used to be set means "entering" a file isn't enough to cause a "//@ line" comment, because we get this sequence:

1. self.context['FILE'] is updated to the new line, self.context['LINE'] is set to 0.
2. self.noteLineInfo() <-- sets self.last_line to the _new_ file and line to 0.
3. self.handleLine(l) <-- gets the new file and line 1, which it expects.

I added to the test to show this a little more clearly.
Attachment #8678397 - Flags: review?(mh+mozilla)
Comment on attachment 8678397 [details]
MozReview Request: Bug 1215238 - Mention the included filepath in pre-processed js sources with #includes. r=glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23199/diff/2-3/
Comment on attachment 8678397 [details]
MozReview Request: Bug 1215238 - Mention the included filepath in pre-processed js sources with #includes. r=glandium

https://reviewboard.mozilla.org/r/23199/#review21989

::: python/mozbuild/mozbuild/test/test_preprocessor.py:606
(Diff revision 3)
> +                              'fin\n').replace('CWD', os.getcwd()))

I think you still need to replace CWD/ with os.getcwd() + os.pathsep, because Windows.
Attachment #8678397 - Flags: review?(mh+mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/d419fd3f62ef
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.