bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

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

RESOLVED FIXED in Firefox 45

Status

Firefox Build System
General
RESOLVED FIXED
3 years ago
5 months ago

People

(Reporter: Chris Manchester (mostly offline July 16-20), Assigned: Chris Manchester (mostly offline July 16-20))

Tracking

unspecified
mozilla45

Firefox Tracking Flags

(firefox45 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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
Created 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.
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+

Comment 11

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d419fd3f62ef
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45

Updated

5 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.