[tool][web] Pass invoker flag to dart2js. #122344
Conversation
Dart2js currently produces a warning message if it is invoked from its snapshot and not from the 'dart compile js' command. In Dart 3.0 we plan to make it an error to invoke the snapshot directly, unless we have previously allowed it (which is the case in our internal build system and the flutter tool). By passing the invoker flag we can recognize flutter's use and prevent breaking it when we enable this new error.
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
@christopherfujino - I have a limited setup for this repo, so I don't have a full validation that this is working as expected. Will the tests cover this codepath directly? If not, would you be available to help me validate that this works as expected? The only observable difference today should be that you no longer see a warning message. In particular, today you see this when doing a flutter web build like |
Looks like your failing presubmits are unit tests. I will manually verify the change tomorrow morning. |
It makes me happy when a test catches changes, even if minor 😄 I just uploaded test expectation changes. The updates don't validate what's the stdout of the underlying dart2js command, only that the command and test expectations are the same. |
yep, they're not perfect, but better than nothing. so I'm actually trying to reproduce the warning message at HEAD (i.e. not on this branch) but I can't. Investigating futher... |
|
So on the Flutter master channel, I executed:
And I see this in the verbose logs: @sigmundch Shouldn't I have seen the warning about dart2js being deprecated? I know I've seen this warning before... |
|
I see it often in bug reports. I wonder if it is only shown on stderr and is potentially hidden unless the tool crashes or returns an error? Do you see it by chance if you try to compile a program with a syntax error? See for example: dart-lang/sdk#51658 |
|
never mind - the error is printed to stdout when I run dart2js locally. Do you see the error if you simply copy/paste the command that is shown and run it directly outside the flutter tool? |
Hmm, yeah, when I run this compile command directly in my terminal I see the warning, let me dig more into where it goes... |
Ahh, ok, we only show that output if there is a compiler error. I can verify this change works, let me add an integration test that fails compilation and verifies we don't see the warning. |
|
Awesome, thank you! |
|
Pushed an update to an existing integration test and added the |
|
Fantastic, thank you so much @christopherfujino! |
Dart2js currently produces a warning message if it is invoked from its snapshot and not from the
dart compile jscommand.We plan to make it an error to invoke the snapshot directly, potentially in the Dart 3 timeframe.
There are two mechanisms available to avoid this error:
dart compile jsto invoke dart2js instead of the dart2js snapshot--invokerflag to indicate to dart2js that the invocation is from a supported context.This CL takes the latter approach, which is the same approach used for the dart-cli and our internal build systems using bazel. Long term Flutter may prefer to start using
dart compile js, but that would require a more careful validation.For context see: dart-lang/sdk#51695 and dart-lang/sdk#46100