Skip to content
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

Implement Cross Compilation #162

Merged
merged 21 commits into from
Apr 20, 2024

Conversation

CompeyDev
Copy link
Contributor

See the linked issue for more details.

Closes #161.

This commit addresses various lint issues, mainly:
* clippy flagging once_cell::Lazy variables consts
* other simple fixes in the build command
@CompeyDev CompeyDev marked this pull request as draft March 10, 2024 11:08
@filiptibell
Copy link
Collaborator

A couple questions / notes on this, not reviewing yet since this is a draft:

  • How should native compilation be handled? Right now standalone binaries also include native compilation for luau bytecode. This will not work when cross-compiling for a different target
  • How do we make guarantees that things will still work when using the --base argument? Luau does not make any guarantees that bytecode is compatible between versions (even if they try to not change it too much). They also do not expose any information about this to us that we can check/compare for

@CompeyDev
Copy link
Contributor Author

How should native compilation be handled?

Isn't native compilation handled at runtime? If that is the case, I don't think we'd need to worry about that since the target would be tailored to the platform which would be consuming the natively compiled bytecode anyway.

How do we make guarantees that things will still work when using the --base argument?

I've been troubled by this issue as well; atm, we're just assuming the user knows what they're doing, and are providing a base binary at their own risk. This isn't ideal, but there isn't a very straightforward solution to this problem either. The only other solution I could think of was to parse version increments for luau, but that was quickly shut down as I realized luau does not follow semver. The only other solution we could practically implement would be to statically keep track of breaking changes to bytecode for versions for different lune and luau versions, and alert the user when they are providing a base binary which may cause such a conflict.

@CompeyDev
Copy link
Contributor Author

In order to address bytecode conflicts among varying lune and luau versions, it is currently hardcoded to only support cross compilation among targets for the same release.

@CompeyDev CompeyDev marked this pull request as ready for review March 14, 2024 13:11
Copy link
Collaborator

@filiptibell filiptibell left a comment

Choose a reason for hiding this comment

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

Left some initial comments and changes to make. I have a couple more things that I didn't leave in specific places:

  • We should probably use something like zip_next and a spawn_blocking here instead, async zip is making things more difficult unnecessarily since zip body is not received and decompressed concurrently either way. It's being received in full and then decompressed
  • Lua::compile has JIT enabled, we should probably disable it for standalone binaries on other targets somehow, probably fine to keep enabled for the same os/arch as current though
  • Why has the output formatting changed, and extra prefix spacing been added to many output lines? The output is intentionally plain but maybe we can add a splash of color to things like file paths, following how the rest of Lune does things

src/cli/build.rs Outdated Show resolved Hide resolved
src/cli/build.rs Outdated Show resolved Hide resolved
src/cli/build.rs Outdated Show resolved Hide resolved
src/cli/build.rs Outdated Show resolved Hide resolved
src/cli/build.rs Outdated Show resolved Hide resolved
* Moves caching logic into its own function (`cache_target`)
* `get_base_exe_path` now returns a tuple including the output path
* The build command now checks whether the target specified is the same as the host. If so, it proceeds to use the current executable.
* Instead of returning a `to_cross_compile` boolean, `get_base_exe_path` now directly returns the path to the current exe if required.
defaults

In the previous commit, I had moved `LuaCompiler` construction into the
CLI command, which was not necessary. This reverts that.
@CompeyDev
Copy link
Contributor Author

I've addressed all the specific comments in the review, would you mind addressing the general structural ones yourself after you merge this PR?

Copy link
Collaborator

@filiptibell filiptibell left a comment

Choose a reason for hiding this comment

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

LGTM 👍

I've addressed all the specific comments in the review, would you mind addressing the general structural ones

Sure yeah, I'll take a look

@filiptibell filiptibell merged commit 7fb48df into lune-org:main Apr 20, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The standalone compiler should support cross compilation
2 participants