-
Notifications
You must be signed in to change notification settings - Fork 24
Add option for native exe #182
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
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
f91400d to
0927497
Compare
nex3
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an unresolved tension here between what it means to enable native compilation and the possibility of cross-compiling for other platforms. As this PR is right now, this is broken: if you run pkg-standalone-windows-x64 while you're on Linux with useNative set to true, you'll get a native linux binary with a .exe extension.
I think a better approach is to interpret and document useNative specifically as only affecting the current platform. We should still expose cross-platform build tasks, and those tasks should still work. This provides users the maximum amount of flexibility.
|
I renamed the option useExe to specifically use exe instead of aot for native targets, updated the documentation, and fixed the packaging. |
nex3
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add tests for this as well? Specifically:
- Test that it generates an EXE on a non-default platform when configured to do so
- Test that it doesn't generate an EXE on a default platform when configured to do so
- Make sure there are tests that it does the default thing correctly as well (these probably already exist)
I will gladly add tests, and need some directions as to where and how I can insert these. |
There are existing tests in |
e98ce8d to
e35a4f1
Compare
|
Squashed and rebased on main/2.13.0 |
nex3
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this looks good :)
|
It looks like there's a failure in one of your new tests. |
Fixed |
|
Can you set this PR to allow me to push to your branch? |
It seems you aloready have write access: I don't have an option to allow you, and the PR has "1 change requested by reviewers with write access". |
see #67