The default bug view has changed. See this FAQ.

Add basic source map support to the preprocessor

NEW
Unassigned

Status

()

Core
Build Config
4 years ago
2 years ago

People

(Reporter: Gijs, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
Created attachment 814432 [details] [diff] [review]
preprocessor-source-maps

This is the basic patch I have right now. It definitely needs a bit of cleanup before going anywhere near hg.m.o. At a minimum, we should probably have some way to configure this, and configure the output location for the source maps.

Greg, could you help me out here with some minimal pointers in terms of how/where to do that?
Attachment #814432 - Flags: feedback?(gps)
Just checked out the source maps being output, and they look good to me.

See http://fitzgeraldnick.com/weblog/51/ for advice on how to test generated source maps.

Comment 2

4 years ago
Comment on attachment 814432 [details] [diff] [review]
preprocessor-source-maps

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

Nice!

Is there any way the core API for writing source maps can be extracted into its own Python module? Perhaps you could add the functionality to https://github.com/mattrobenolt/python-sourcemap! We'll need to write source maps in other parts of the build system (e.g. when we start minifying JS). So having a reusable API is a huge win. I'd prefer an external package (which we can import easily enough). But a .py file under python/mozbuild/mozbuild will be good for now.

The general approach of passing an additional argument to the preprocessor to have it write source maps is fine. We'll likely need to retool a bunch of call sites in the build system. But those can be followup bugs.

I know file consistency normally wins out, but can you please write 4 space indent for all new Python where possible? I plan to run all these 2 space indended files through a reformater one of these days. Feel free to submit a part 0 that does that. Hint: there might be a reindent.py lingering on your machine. If not, see http://hg.python.org/cpython/file/1cbd3d9f7d61/Tools/scripts/reindent.py.

::: config/Preprocessor.py
@@ +238,5 @@
>      if includes:
>        for f in includes:
>          self.do_include(f, False)
>        self.warnUnused(f)
> +      if (self.writeSourceMap):

Nit: Don't need () in Python unless you are grouping for precedence reasons or spanning multiple lines.
Attachment #814432 - Flags: feedback?(gps) → feedback+
(Reporter)

Comment 3

4 years ago
(In reply to Gregory Szorc [:gps] from comment #2)
> Comment on attachment 814432 [details] [diff] [review]
> preprocessor-source-maps
> 
> Review of attachment 814432 [details] [diff] [review]:

Thanks for taking a look!

> Is there any way the core API for writing source maps can be extracted into
> its own Python module? Perhaps you could add the functionality to
> https://github.com/mattrobenolt/python-sourcemap! We'll need to write source
> maps in other parts of the build system (e.g. when we start minifying JS).
> So having a reusable API is a huge win. I'd prefer an external package
> (which we can import easily enough). But a .py file under
> python/mozbuild/mozbuild will be good for now.

Hm. If we minify JS, chances are we'd need to use the minifier's own source map support. The API for writing the file, probably. I suppose I can extract code in a fashion where there'd be an API of the form:

addMapping([column, line, newColumn, newLine, symbols])

and the current code could just call that (it's pretty close to the method it's using right now, IIRC). That's pretty low level, but it'd work. I will give that a shot.

> The general approach of passing an additional argument to the preprocessor
> to have it write source maps is fine. We'll likely need to retool a bunch of
> call sites in the build system. But those can be followup bugs.

Yeah, I have to admit I'm still not clear on the best integration story here. It seems we all agree we don't want to ship the original files because of size concerns.

Here's a proposal based on the initial feedback in the fx-dev list:
- ship the processed files with an //#sourceMappingURL = chrome://browser-source/content/browser.js.map style final comment. That'll fail to load in builds that don't include the package, which shouldn't have any adverse effects (if it does, that's a bug and we should fix it).
- put the maps in dist/sourceMaps/$(PackageName)-source/$(PackageType)/$(Filename).map

Then I see two options:
- We could make the build process query the hg parent cset and default URL to generate an hg-web source URL like https://hg.mozilla.org/projects/ux/raw-file/a99a67ddfb41/browser/base/content/browser.js . We could include those in the source maps, and hopefully the source map code would be fine with fetching them...
- We could also store/symlink the original files in the same dist/sourceMaps/$(PackageName)-source/$(PackageType)/$(Filename)

Then, regardless of which of these two options we pick, we could build a dumb add-on as part of the build process that simply zips up everything in dist/sourceMaps/ and uses chrome.manifest to map the files to the chrome URLs. Presumably, if you had the add-on installed and opened your debugger, the source maps would Just Work. Of course, the problem would be having some version of the add-on installed and then updating Firefox would mean the maps would be out of date. We could probably have a bootstrap.js script that disabled the add-on if the build rev didn't match the source maps' rev.

Greg and Nick, I'd like both of your feedback on the above, if possible. :-)

(the patch on this bug just writes the map to the same place as the preprocessed file; that'd need to be fixed before landing)

> I know file consistency normally wins out, but can you please write 4 space
> indent for all new Python where possible? I plan to run all these 2 space
> indended files through a reformater one of these days. Feel free to submit a
> part 0 that does that. Hint: there might be a reindent.py lingering on your
> machine. If not, see
> http://hg.python.org/cpython/file/1cbd3d9f7d61/Tools/scripts/reindent.py.

I will have a look at this.


Note also that the current code doesn't do filter substitutions, only includes/if/ifdef/else conditional inclusion/exclusion. I used MXR to look for cases where we do substantial filter substitutions in 'real' JS files (ie not firefox.js/all.js pref collection files) and didn't find any, but it's totally possible I missed stuff, in which case I'd need to add support for that. We'd certainly want that if we were to do this for CSS files, where we use it extensively. Less sure how useful it is there (not supported by devtools right now, so less of a priority, too).
(Reporter)

Comment 4

4 years ago
(In reply to :Gijs Kruitbosch from comment #3)
> Greg and Nick, I'd like both of your feedback on the above, if possible. :-)

Sigh. Setting needinfo this time, sorry for spam.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(nfitzgerald)
Flags: needinfo?(gps)
I think for the first pass, we should have this be a developer only feature that isn't enabled for releases. Then we can inline the source maps into the files via data URI and the sources into the source maps via sourcesContent. If we do all this, then we don't worry about versions of sources lining up, we don't worry about where to store the source map and how to link to it properly, etc.
Flags: needinfo?(nfitzgerald)
(Reporter)

Comment 6

4 years ago
(In reply to Nick Fitzgerald [:fitzgen] from comment #5)
> I think for the first pass, we should have this be a developer only feature
> that isn't enabled for releases. Then we can inline the source maps into the
> files via data URI and the sources into the source maps via sourcesContent.
> If we do all this, then we don't worry about versions of sources lining up,
> we don't worry about where to store the source map and how to link to it
> properly, etc.

That'd be pretty huge, though... browser.js is 500k. I forget exactly how big the source map is, but the base64 overhead of the data URI would pretty much guarantee a 700k+ data URI because it'll contain all the source info. That doesn't sound like a workable solution.
(In reply to :Gijs Kruitbosch from comment #6)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #5)
> > I think for the first pass, we should have this be a developer only feature
> > that isn't enabled for releases. Then we can inline the source maps into the
> > files via data URI and the sources into the source maps via sourcesContent.
> > If we do all this, then we don't worry about versions of sources lining up,
> > we don't worry about where to store the source map and how to link to it
> > properly, etc.
> 
> That'd be pretty huge, though... browser.js is 500k. I forget exactly how
> big the source map is, but the base64 overhead of the data URI would pretty
> much guarantee a 700k+ data URI because it'll contain all the source info.
> That doesn't sound like a workable solution.

We are going to load all that as soon as you open the debugger anyways.

Personally, I think this is totally fine for builds that (1) firefox devs build themselves (don't enable this for Nightly or any release channel), and (2) devs manually flip a flag to enable it (off by default).

This way, we can have a first iteration and gather feedback from brave souls, without worrying about all the little problems that come when we start talking about how to do this on a larger scale.

Comment 8

4 years ago
Comment on attachment 814432 [details] [diff] [review]
preprocessor-source-maps

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

I'd still prefer the source map writing to be in its own reusable function: we will need to produce source maps elsewhere. You can punt on this if you want. But it will get rewritten, likely in the near future.

::: config/Preprocessor.py
@@ +165,5 @@
> +
> +      encoded = ''
> +      # Poor man's do...while loop:
> +      shouldContinue = True
> +      while (shouldContinue):

Nit: No needs for parens in Python.

@@ +205,5 @@
> +        b64node += 'A'
> +        mappings += b64node
> +        lastFileIndex = fileIndex
> +        lastLine = oldLine - 1
> +      lineCounter += 1

Since strings are immutable in Python, += in loops can be bad for perf. Since you are only doing boolean testing on mappings, I'd convert it to a list and just append new elements. After the loop, just ''.join(mappings) to produce the string.

@@ +210,5 @@
> +    info['mappings'] = mappings
> +    if hasattr(self, "sourceMapFile"):
> +      outf = self.sourceMapFile
> +    else:
> +      outf = open(outputFileName + '.map', 'wb')

You should just pass this into the function. Having arguments that may not do something depending on object state leads to hard to understand code and APIs.

Comment 9

4 years ago
I'm not sure what you need my info about. I think we should punt hard decisions about what to do about source map bundling and lookup into another bug. Let's just get the preprocessor to play nice here. (That's not to say I don't care - I do, very much.)
Flags: needinfo?(gps)
(Reporter)

Comment 10

2 years ago
I've changed my mind about what I think we should do here, and think we should just strive to eliminate ifdefs/includes from our tree instead, so I'm unassigning.
Assignee: gijskruitbosch+bugs → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.