[M] Change in dart/sdk[main]: [dart2wasm] Generate relative file paths in source map sources section
Ömer Ağacan (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Martin Kustermann
- Nate Biggs
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Biggs (Gerrit)
Nate Biggs added 1 comment![]( "Open in Gerrit")
final sourceMapJson = serializer.sourceMapSerializerI think dart2js uses the relativizeUri function from the front end instead.
See usages in: https://github.com/dart-lang/sdk/blob/main/pkg/compiler/lib/src/io/source_map_builder.dart
If we use that here we should be consistent.
And then I don't think `wasm_builder` should be modifying what the caller gives it.
Related details
- Martin Kustermann
- Ömer Ağacan
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ömer Ağacan (Gerrit)
Ömer Ağacan added 1 comment![]( "Open in Gerrit")
final sourceMapJson = serializer.sourceMapSerializerI think dart2js uses the relativizeUri function from the front end instead.
See usages in: https://github.com/dart-lang/sdk/blob/main/pkg/compiler/lib/src/io/source_map_builder.dart
If we use that here we should be consistent.
And then I don't think `wasm_builder` should be modifying what the caller gives it.
I've updated the code to use dart2js's relativize function.
Not sure what you mean by modifying what the caller gives? The caller gives wasm builder the locations in the kernel AST nodes, which are absolute paths. Someone needs to do the relativization. dart2js also does it when generating source maps: https://github.com/dart-lang/sdk/blob/3c86e1eb1e9a019bd699d7ada601a0e9daf4af02/pkg/compiler/lib/src/io/source_map_builder.dart#L126
Related details
- Martin Kustermann
- Nate Biggs
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Biggs (Gerrit)
Nate Biggs added 2 comments![]( "Open in Gerrit")
final sourceMapJson = serializer.sourceMapSerializerÖmer AğacanI think dart2js uses the relativizeUri function from the front end instead.
See usages in: https://github.com/dart-lang/sdk/blob/main/pkg/compiler/lib/src/io/source_map_builder.dart
If we use that here we should be consistent.
And then I don't think `wasm_builder` should be modifying what the caller gives it.
I've updated the code to use dart2js's relativize function.
Not sure what you mean by modifying what the caller gives? The caller gives wasm builder the locations in the kernel AST nodes, which are absolute paths. Someone needs to do the relativization. dart2js also does it when generating source maps: https://github.com/dart-lang/sdk/blob/3c86e1eb1e9a019bd699d7ada601a0e9daf4af02/pkg/compiler/lib/src/io/source_map_builder.dart#L126
What I meant is that we shouldn't do the relativizing in `wasm_builder`. We should do it directly in dart2wasm. wasm_builder should just whatever URI the package using it gives it.
import 'package:front_end/src/api_unstable/dart2js.dart' as fe;Can you import relativizeUri from here instead: google3/third_party/dart/_fe_analyzer_shared/lib/src/util/relativize.dart
Related details
- Martin Kustermann
- Ömer Ağacan
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ömer Ağacan (Gerrit)
Ömer Ağacan added 1 comment![]( "Open in Gerrit")
import 'package:front_end/src/api_unstable/dart2js.dart' as fe;Can you import relativizeUri from here instead: google3/third_party/dart/_fe_analyzer_shared/lib/src/util/relativize.dart
- Martin Kustermann
- Nate Biggs
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ömer Ağacan (Gerrit)
Ömer Ağacan added 1 comment![]( "Open in Gerrit")
final sourceMapJson = serializer.sourceMapSerializerÖmer AğacanI think dart2js uses the relativizeUri function from the front end instead.
See usages in: https://github.com/dart-lang/sdk/blob/main/pkg/compiler/lib/src/io/source_map_builder.dart
If we use that here we should be consistent.
And then I don't think `wasm_builder` should be modifying what the caller gives it.
Nate BiggsI've updated the code to use dart2js's relativize function.
Not sure what you mean by modifying what the caller gives? The caller gives wasm builder the locations in the kernel AST nodes, which are absolute paths. Someone needs to do the relativization. dart2js also does it when generating source maps: https://github.com/dart-lang/sdk/blob/3c86e1eb1e9a019bd699d7ada601a0e9daf4af02/pkg/compiler/lib/src/io/source_map_builder.dart#L126
What I meant is that we shouldn't do the relativizing in `wasm_builder`. We should do it directly in dart2wasm. wasm_builder should just whatever URI the package using it gives it.
It's a bit tricky because code generators don't know about their source file map files currently.
They would need to find them from the `Translator`, but `Translator` is also per-program rather than per-module, so it needs to keep track of all source map files for all modules.
So I guess we'll need to map `ModuleBuilder`s to absolute paths to source map files, and then a code generator will do `translator.moduleMetadata(b.moduleBuilder).sourceMapPath` or similar.
Then somehow the io_manager needs to pass the source map absolute paths to `ModuleMetadata`s. (this needs to be done in the io manager as it does IO -- source map paths are relative to the working directory initially)
I'll get back to this CL later.
Related details
- Martin Kustermann
- Nate Biggs
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ömer Ağacan (Gerrit)
Ömer Ağacan added 1 comment![]( "Open in Gerrit")
final sourceMapJson = serializer.sourceMapSerializerÖmer AğacanI think dart2js uses the relativizeUri function from the front end instead.
See usages in: https://github.com/dart-lang/sdk/blob/main/pkg/compiler/lib/src/io/source_map_builder.dart
If we use that here we should be consistent.
And then I don't think `wasm_builder` should be modifying what the caller gives it.
Nate BiggsI've updated the code to use dart2js's relativize function.
Not sure what you mean by modifying what the caller gives? The caller gives wasm builder the locations in the kernel AST nodes, which are absolute paths. Someone needs to do the relativization. dart2js also does it when generating source maps: https://github.com/dart-lang/sdk/blob/3c86e1eb1e9a019bd699d7ada601a0e9daf4af02/pkg/compiler/lib/src/io/source_map_builder.dart#L126
Ömer AğacanWhat I meant is that we shouldn't do the relativizing in `wasm_builder`. We should do it directly in dart2wasm. wasm_builder should just whatever URI the package using it gives it.
It's a bit tricky because code generators don't know about their source file map files currently.
They would need to find them from the `Translator`, but `Translator` is also per-program rather than per-module, so it needs to keep track of all source map files for all modules.
So I guess we'll need to map `ModuleBuilder`s to absolute paths to source map files, and then a code generator will do `translator.moduleMetadata(b.moduleBuilder).sourceMapPath` or similar.
Then somehow the io_manager needs to pass the source map absolute paths to `ModuleMetadata`s. (this needs to be done in the io manager as it does IO -- source map paths are relative to the working directory initially)
I'll get back to this CL later.
As I read the code more I think the current approach is better.
To give access to source file location in code generators we have to pass that information from code gen phase runner to code generators. Code generators currently don't deal with file paths at all, when generating source mappings they use the locations in kernel AST nodes. I don't see why we would want to make it code generator's business to transform source map locations.
We could relativize the paths separately from `wasm_builder`, after generating the mappings the same way as today, but before serializing them as JSON. I don't know what that would buy us though.
@nate...@google.com PTAL at two comments above. If you still think that
We should do it directly in dart2wasm. wasm_builder should just whatever URI the package using it gives it.
Please explain why, in concrete details. E.g. "if we do it the current way it causes X, which we want to avoid."
Related details
- Martin Kustermann
- Nate Biggs
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Biggs (Gerrit)
Nate Biggs added 1 comment![]( "Open in Gerrit")
I want to keep `wasm_builder` as a general purpose WASM AST package without backend specific implementation details. It's possible that in the future we may build a development backend based on WASM that could reuse the same package.
I see "How should I format source map file URIs" as a backend specific decision. Different backends may want to relativize these differently (or not at all). The WASM AST package should just be told by the backend the exact path it needs to put into the WASM module.
It seems like not a huge change to just update `AstCodeGenerator.setSourceMapFileOffset` to call `fe.relativizeUri` on the `fileUri` it passes. To get the base URL argument you can just do `b.moduleBuilder.sourceMapUrl`. The module builder should already have a reference to the relevant source URL.
Related details
- Martin Kustermann
- Ömer Ağacan
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ömer Ağacan (Gerrit)
Ömer Ağacan added 1 comment![]( "Open in Gerrit")
`b.moduleBuilder.sourceMapUrl` can't be used as it's the source map URL encoded in the .wasm file. Currently it's also the relative path to the file (relative to the compiler's working dir), but that doesn't have to be the case, and we need an absolute path to relativize. So it can't be used.
Making it absolute in code generator would introduce a `dart:io` dependency in the compiler.
Making it absolute when creating the `ModuleBuilder` make it break getting the source map file in the browser.
I think we should move source map builder to dart2wasm. The only thing about it that's Wasm specific is that instead of lines and columns, it deals with byte offsets. (in source maps, binary locations are indicated as line 0 column <byte offset>) I don't remember why I added it to wasm_builder.
Relativization could also be made optional by just making the `Uri` argument optional.
WDYT about moving source map builder to dart2wasm?
Related details
- Martin Kustermann
- Nate Biggs
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Biggs (Gerrit)
Nate Biggs added 1 comment![]( "Open in Gerrit")
I don't know, I like the current design of having it be in wasm_builder. If another tool/compiler wants to make use of wasm_builder to produce wasm, it'll likely also want to generate source maps. So it seems like having the logic for both be interleaved makes sense. Like it's easier for `instructions.dart` to know the offsets of the source mapped ranges than for dart2wasm to track that.
b.moduleBuilder.sourceMapUrl can't be used as it's the source map URL encoded in the .wasm file.
AFAICT the argument you pass to `serializeAsJson` and use as the base URL for the fe.relativizeUri is the exact same argument that gets passed to `b.moduleBuilder.sourceMapUrl`:
https://github.com/dart-lang/sdk/blob/main/pkg/dart2wasm/lib/translator.dart#L557
Both are the result of calling `sourceMapUrlGenerator(moduleName)`.
Related details
- Martin Kustermann
- Ömer Ağacan
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |