Skip to content

dart2js: support main with an argument #14200

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
iposva-google opened this issue Oct 17, 2013 · 12 comments
Closed

dart2js: support main with an argument #14200

iposva-google opened this issue Oct 17, 2013 · 12 comments
Labels
P0 A serious issue requiring immediate resolution web-dart2js
Milestone

Comments

@iposva-google
Copy link
Contributor

Both main() and main(args) are valid signatures for the main entry point into an isolate. Florian can expand on this if needed.

@kasperl
Copy link

kasperl commented Oct 18, 2013

Do we have any tests for this yet?


Added this to the M8 milestone.
Removed Priority-Unassigned label.
Added Priority-Critical label.
Changed the title to: "dart2js: support main with an argument".

@iposva-google
Copy link
Contributor Author

corelib/main_test.dart does minimal checking that you do not fall over when you write a main with arguments.

It is unclear to me how I would write a test using our harness which would pass some arguments to the test execution.

@kasperl
Copy link

kasperl commented Oct 18, 2013

Yeah, my worry was that our harness doesn't allow testing this. Maybe that's something we can fix though. Allowing a test like this:

   // Options: foo bar
   import 'package:expect/expect.dart';
   main(args) {
     Expect.listEquals(['foo', 'bar'], args);
   }

would allow us to make progress. We'll have to figure out what it means in a browser context.


cc @ricowind.

@peter-ahe-google
Copy link
Contributor

Why is this critical?

What is "args"? Might that argument contain useful information, or will it simply be null? Is there a type that would be appropriate?

@whesse
Copy link
Contributor

whesse commented Oct 22, 2013

I have written a test for this, using the DartOptions feature of the test harness.
It is just changing standalone/io/options_test.dart to standalone/io/arguments_test.dart. The CL has a supression for dart2js, with this issue noted.
CL = https://codereview.chromium.org/34233003/

The old version also had the supression, because dart2js compilation does not accept the arguments on the DartOptions line anyway:
// DartOptions=tests/standalone/io/arguments_test.dart 10 arguments_test 20

@peter-ahe-google
Copy link
Contributor

Where is this behavior specified?

@DartBot
Copy link

DartBot commented Oct 22, 2013

This comment was originally written by @seaneagan


What's wrong with new Options().arguments ? That can be accessed from anywhere, rather than needing to be passed around as a parameter to anything that needs to access it. I might prefer a plain top-level property though, so I can omit the new Options(). part.

For command line apps, issue #12272 would provide a more convenient way to access args. For browser apps, the URI pretty much acts as the command line, so a similar approach could be taken with the "route" package.

@floitschG
Copy link
Contributor

If you want to leak the main arguments, just create a global variable and store it there. So nothing is lost by just giving it to main.

We don't like the new Options().arguments for several reasons: it is only in dart:io which means only server, and it shouldn't be necessary to allocate a new object (which is due to historic limitations).
Since we clean up the API we might as well create one that is familiar to most developers and that doesn't leak the arguments to everyone.
Another reason for this change is the refactorings of the isolate library which should use a similar pattern to send the initial message.

@peter-ahe-google
Copy link
Contributor

FWIW, this would simplify use of command-line arguments in the implementation of dart2js. I'm all for this change, it just needs to be specified somewhere. "Florian can expand on this if needed." is a bit too vague.

@floitschG
Copy link
Contributor

This is now blocking the dart:isolate refactoring.

We work around it with the following patch, but it breaks dart2js when isolate support is not enabled: it passes in null instead of the empty list, and even then, null is not enqueued and therefore a print(args) will fail.

https://codereview.chromium.org/39263007/

@larsbak
Copy link

larsbak commented Oct 25, 2013

Please coordinate with floitsch and fix this asap.


Set owner to [email protected].

@DartBot
Copy link

DartBot commented Oct 25, 2013

This comment was originally written by [email protected]


Fixed in https://codereview.chromium.org/43723002/.


Added Fixed label.

@iposva-google iposva-google added Type-Defect P0 A serious issue requiring immediate resolution web-dart2js labels Oct 25, 2013
@iposva-google iposva-google added this to the M8 milestone Oct 25, 2013
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 A serious issue requiring immediate resolution web-dart2js
Projects
None yet
Development

No branches or pull requests

7 participants