Skip to content

Run the executable if no error #1302

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

Merged
merged 1 commit into from
Nov 21, 2022

Conversation

Thirumalai-Shaktivel
Copy link
Collaborator

No description provided.

Comment on lines +1537 to +1540
if (backend == Backend::wasm) {
err = system(("js " + outfile +".js").c_str());
} else {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Shaikh-Ubaid, the Wasm Backend executable can be executed using js filename.out.js, right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be executed using node a.out.js (where a.out and a.out.js are the output files generated).

I am unsure if we need auto-execution for wasm backend (or other backends excluding llvm). It seems that when a user specifies a particular backend (apart from llvm, for example, let's say --backend=wasm), the (primary) intention of the user is to generate target backend code from the source code. Thus, if a user executes lpython examples/expr2.py --backend wasm, he's trying to generate wasm code from the source code. He could later choose to use the generated wasm code for his own purposes or choose to execute it.

I think it would be better if we leave the choice of what should be done with the output code (wasm code in our example) onto the user. One added advantage of leaving the choice with the user is that the user would get to know what all files are generated using a particular backend (wasm backend generates two files a.out and a.out.js). The user also gets the opportunity to explore how the generated target (wasm) code could be executed.

I am just sharing my thoughts here. Anything is-fine/works for me.

Copy link
Collaborator Author

@Thirumalai-Shaktivel Thirumalai-Shaktivel Nov 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@czgdp1807 @certik Can you please share your thoughts as well?
I made these changes because I felt like the behaviour should be consistent on all the backends.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that when a user specifies a particular backend (apart from llvm, for example, let's say --backend=wasm), the (primary) intention of the user is to generate target backend code from the source code.

Not sure. I guess --show-wasm should generate the code?

Overall I guess at least the executable shouldn't be deleted after automatic execution. For now for LLVM backend it gets deleted.

Copy link
Collaborator Author

@Thirumalai-Shaktivel Thirumalai-Shaktivel Nov 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this PR changes got merged. LPython doesn't generate a.out anymore. Instead, it generates filename.out. These changes were made because of some problems in testing the runtime outputs, see here for more details. Also, to stop the automatic execution one can use the -o option to specify the executable to be written into.
I apologize for not sharing this updated information.

Copy link
Contributor

@certik certik Nov 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion either way on this. I feel calling "node" from LPython seems fragile.

I think for now, why don't we do the following:

  • lpython a.py compiles a.py with the default backend (currently LLVM, perhaps in the future it will be WASM + x86/arm) and runs it
  • If any other option is passed to lpython, then it will not run it by default.

Although I can see how this is not super consistent. But it gets us started. I think the most common use case is python a.py, so we now already have lpython a.py. Everything else seems lower priority.

We could also consider lpython run a.py or lpython --run a.py, and then you can supply other options as needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean lpython --run a.py --backend x86 to run by default?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think lpython a.py --backend x86 && ./a.out is much familiar than using --run. So, Let's only support lpython a.py to run by default.
Also, I have a doubt: Currently, we write the executable to filename.out. Is this fine for other backend as well?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean lpython --run a.py --backend x86 to run by default?

Makes sense to me.

Comment on lines +1546 to +1549
if (err != 0) {
if (0 < err && err < 256) {
return err;
} else {
return 1;
}
}
Copy link
Collaborator Author

@Thirumalai-Shaktivel Thirumalai-Shaktivel Nov 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a separate topic:
I recently came across something like this:

def test():
	quit(10)

test()
$ lpython a.py
err = 2560
STOP
$ echo $?
1

So, I thought to replace this with the following change:

Suggested change
if (err != 0) {
if (0 < err && err < 256) {
return err;
} else {
return 1;
}
}
if (err != 0) {
if (0 < err && err < 256) {
return err;
} if else (err % 256 == 0) {
return err / 256;
} else {
return 1;
}
}

@certik

Copy link
Collaborator

@czgdp1807 czgdp1807 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Will ask @certik as well before merging. Conflicts should be resolved as well.

Provide this support for all other backends.
The user can pass `-o` option to stop the binary execution.
Copy link
Contributor

@certik certik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

Open up a new issue for the error return logic.

@Thirumalai-Shaktivel
Copy link
Collaborator Author

Thanks for the approvals. Let's merge this.

@Thirumalai-Shaktivel Thirumalai-Shaktivel merged commit d9525b6 into lcompilers:main Nov 21, 2022
@Thirumalai-Shaktivel Thirumalai-Shaktivel deleted the auto_run branch November 21, 2022 05:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review PRs that are ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants