[M] Change in dart/sdk[main]: [dart2wasm] Fix handling of --define/-D
Ömer Ağacan (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Martin Kustermann
- Nate Biggs
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")
Something about ddc needs to be fixed as well, but I can't figure out what as I can't figure out how to run the test in ddc. I'll fix it next week.
Related details
- Martin Kustermann
- Nate Biggs
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kustermann (Gerrit)
Martin Kustermann voted and added 2 comments![]( "Open in Gerrit")
Votes added by Martin Kustermann
| Code-Review | +1 |
2 comments
var dart2jsOptions1 = _parseStringOption(Consider having a helper function to avoid the redundancy between dart2js/dart2wasm/ddc
// ddcOption="-DFOO=a, b, c=123"I think the failing DDC test may be due to this quoting as it runs
```
out/ReleaseX64/dart-sdk/bin/snapshots/dartdevc_aot.dart.snapshot ""-DFOO=a, b, c=123"" -Dtest_runner.conf...
```
Even for dart2js it seems the quotes shouldn't be needed as it's parsed as one argument. Maybe it's the batch mode compiler that needs to auto-quote arguments instead of us doing this in this test here?
Related details
- Nate Biggs
- Ömer Ağacan
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kustermann (Gerrit)
Martin Kustermann added 1 comment![]( "Open in Gerrit")
Fixes Flutter issue 164873.Please use https://... urls so one can just click on them.
Related details
- Nate Biggs
- Ömer Ağacan
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")
Please use https://... urls so one can just click on them.
Done
Related details
- Nate Biggs
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")
I think the failing DDC test may be due to this quoting as it runs
```
out/ReleaseX64/dart-sdk/bin/snapshots/dartdevc_aot.dart.snapshot ""-DFOO=a, b, c=123"" -Dtest_runner.conf...
```Even for dart2js it seems the quotes shouldn't be needed as it's parsed as one argument. Maybe it's the batch mode compiler that needs to auto-quote arguments instead of us doing this in this test here?
I think the failing DDC test may be due to this quoting as it runs
It shows as if it's adding quotes, but it's not. The code that prints the reproduction instructions is different from the actual code that runs, it prints different commands than what it really runs.
The code is a huge mess really..
Even for dart2js it seems the quotes shouldn't be needed as it's parsed as one argument
IIRC dart2js treats the args differently based on whether you're running in batch mode (where the args are written to the batch compiler's stdin) vs. non-batch mode (where args are passed to the process as args). It's a huge mess...
I'll figure this out.
Related details
- Nate Biggs
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 2 comments![]( "Open in Gerrit")
Something about ddc needs to be fixed as well, but I can't figure out what as I can't figure out how to run the test in ddc. I'll fix it next week.
Done
Consider having a helper function to avoid the redundancy between dart2js/dart2wasm/ddc
- Martin Kustermann
- Nate Biggs
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")
// TODO: Use proper shell escaping here.Give this TODO an owner or attach an issue.
Expect.equals("a, b, c=123", const String.fromEnvironment("FOO"));Can you add a few more tests ensuring the split line is happening?
I know `command_line_split_test` has a bunch of tests of the actual functionality. But I'm not sure we have any other tests guaranteeing that all the web backends continue to use that package. So tests here would help make sure all the backends stay aligned.
Related details
- Martin Kustermann
- Ömer Ağacan
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kustermann (Gerrit)
Martin Kustermann voted Code-Review+1![]( "Open in Gerrit")
- Ömer Ağacan
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")
Give this TODO an owner or attach an issue.
- Nate Biggs
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")
Expect.equals("a, b, c=123", const String.fromEnvironment("FOO"));Can you add a few more tests ensuring the split line is happening?
I know `command_line_split_test` has a bunch of tests of the actual functionality. But I'm not sure we have any other tests guaranteeing that all the web backends continue to use that package. So tests here would help make sure all the backends stay aligned.
I improved the test a little bit, if you tell me what you have in mind that you want to see tested we can add those as well.
Related details
- Nate Biggs
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 voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Nate Biggs
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Commit Queue (Gerrit)
Commit Queue submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
19 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: pkg/shell_arg_splitter/test/command_line_split_test.dart
Insertions: 1, Deletions: 1.
@@ -5,7 +5,7 @@
import "package:expect/expect.dart";
import "package:shell_arg_splitter/shell_arg_splitter.dart";
-main() {
+void main() {
Expect.listEquals(["foo", "bar"], splitLine("foo bar"));
Expect.listEquals(["foo", "bar"], splitLine("foo bar", windows: true));
```
```
The name of the file: tests/web/wasm_js_shared/env_defines_test.dart
Insertions: 4, Deletions: 3.
@@ -2,12 +2,13 @@
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
-// dart2jsOptions="-DFOO=a, b, c=123"
-// ddcOptions="-DFOO=a, b, c=123"
-// dart2wasmOptions="--extra-compiler-option=-DFOO=a, b, c=123"
+// dart2jsOptions="-DFOO=a, b, c=123" --define=BAR=hi
+// ddcOptions="-DFOO=a, b, c=123" --define=BAR=hi
+// dart2wasmOptions="--extra-compiler-option=-DFOO=a, b, c=123" --define=BAR=hi
import 'package:expect/expect.dart';
void main() {
Expect.equals("a, b, c=123", const String.fromEnvironment("FOO"));
+ Expect.equals("hi", const String.fromEnvironment("BAR"));
}
```
```
The name of the file: pkg/test_runner/lib/src/process_queue.dart
Insertions: 1, Deletions: 9.
@@ -1058,15 +1058,7 @@
}
String _createArgumentsLine(List<String> arguments, int timeout) {
- arguments = arguments.map((arg) {
- // TODO: Use proper shell escaping here.
- if (arg.contains(' ')) {
- return "\"$arg\"";
- } else {
- return arg;
- }
- }).toList();
-
+ arguments = arguments.map(escapeCommandLineArgument).toList();
if (_useJson) {
return "${jsonEncode(arguments)}\n";
} else {
```
Change information
[dart2wasm] Fix handling of --define/-D
When parsing `--define` or `-D` arguments don't split the the value by
commas.
This is consistent with how dart2js handles `-D`, but inconsistent with
how VM handles it.
Example:
void main() {
print(const String.fromEnvironment("FOO"));
}
When compiled with `dart compile js -DFOO="a, b"` and run, dart2js
prints
a, b
VM prints (when compiled to exe)
a
Between these two, I think dart2js' behavior is more common, so we
follow dart2js.
Also update compile_benchmark to avoid splitting a single argument "a b"
into "a" and "b" when parsing the arguments and then splicing them back
before calling `dart2wasm`.
Also update the test runner and ddc batch mode argument parser to handle
splitting quoted arguments in `// dart2jsOption = ...` and the same
options for ddc and dart2wasm, by moving dart2js's `splitLine` to a new
library and reusing it in the test runner and ddc.
Fixes https://github.com/flutter/flutter/issues/164873.
See also https://github.com/dart-lang/sdk/issues/60341 for relevant
future work.
- M pkg/compiler/lib/src/dart2js.dart
- M pkg/compiler/pubspec.yaml
- M pkg/dart2wasm/lib/dart2wasm.dart
- M pkg/dart2wasm/lib/option.dart
- M pkg/dart2wasm/tool/compile_benchmark
- M pkg/dartdev/lib/src/commands/compile.dart
- M pkg/dev_compiler/lib/ddc.dart
- M pkg/dev_compiler/pubspec.yaml
- A pkg/shell_arg_splitter/analysis_options.yaml
- R pkg/shell_arg_splitter/lib/shell_arg_splitter.dart
- A pkg/shell_arg_splitter/pubspec.yaml
- R pkg/shell_arg_splitter/test/command_line_split_test.dart
- M pkg/test_runner/lib/src/process_queue.dart
- M pkg/test_runner/lib/src/test_file.dart
- M pkg/test_runner/pubspec.yaml
- A tests/web/wasm_js_shared/env_defines_test.dart
Code-Review: +1 by Martin Kustermann
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |