-
Notifications
You must be signed in to change notification settings - Fork 997
unix: support command line arguments #1410
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
Conversation
|
This is an exciting new feature, and looks good to me. Do you think that we should have any tests for this functionality? |
|
Agreed with @bgould this is very exciting! Also agree that some test coverage would be a very good idea. |
|
@aykevl can you please both handle the merge conflict, and also consider adding a unit test? Thank you. |
|
Reminder to @aykevl to please resolve and merge conflict, and consider adding unit test. Thanks. |
This simplifies the code somewhat but more importantly makes it possible to put other tests under testdata.
db1f1e9 to
46001c0
Compare
|
I have gotten it working, but somehow this change causes a segmentation fault when I run the stdlib test. The issue seems to be in the interp package where somehow an unaligned memory location is loaded from. I plan to finish this PR after #1430 is finished and merged, as that PR should avoid this issue (hopefully). That said, this PR is now rebased to fix the conflicts and has a test (which is indeed a good idea). I made the test in such a way that it should be easy to add environment variables in the future, which I assume will come up soon enough. |
|
I was looking at this yesterday – as I'm kind of waiting on it – and it segfaults when you try to import the |
|
Yeah I haven't found the root cause yet. It seems to be related to the interp package and/or the way globals are packed into one giant global to make them scannable by the GC, and it might just be a LLVM compiler bug even. Either I can look for that, or maybe just give up on that and work on the new interp package instead (which should sidestep this problem entirely). |
46001c0 to
791a11b
Compare
|
would be great to have os.Args, really fundamental stuff. i want to try on some tools with tinygo. |
|
Looking forward to having this! |
|
Closed in favor of #1812. |
|
Now actually closing. |
This has been a long requested feature.
fixes #456 and #541
example usage: