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

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Chris Manchester (limited bugmail, email directly)]

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).

Comment 1

[Chris Manchester (limited bugmail, email directly)]

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

Comment 2

[Chris Manchester (limited bugmail, email directly)]

Attachment: 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.

Comment 3

[Mike Hommey [:glandium]]

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.

Comment 4

[Chris Manchester (limited bugmail, email directly)]

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 5

[Chris Manchester (limited bugmail, email directly)]

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

Comment 6

[Mike Hommey [:glandium]]

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.

Comment 7

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 8

[Chris Manchester (limited bugmail, email directly)]

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 9

[Mike Hommey [:glandium]]

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.

Comment 10

[Pulsebot]

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

Comment 11

[Carsten Book [:Tomcat]]

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