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

Include erlang/elixir ffi modules in exported erlang app files #3728

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

LostKobrakai
Copy link

@LostKobrakai LostKobrakai commented Oct 21, 2024

This does seem to make the compiler happy, but I'm kinda lost on where and how to test this kind of behaviour.

Related to #3710

@LostKobrakai LostKobrakai changed the title Include erlang/elixir ffi modules in exported erlang app files #3710 Include erlang/elixir ffi modules in exported erlang app files Oct 21, 2024
Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Hey! Thanks for looking into this. 💜

It's a bit tricky to test this bit as it involves code generation, but you could add some checks to this test script: https://github.com/gleam-lang/gleam/blob/main/test/external_only_erlang/test.sh

I think your change here may have stopped the entrypoint module from getting compiled by the way- the Erlang compilation is being run after it is rendered to disc. Perhaps the .app writing could be moved to be the last thing in the module rather than moving the Erlang compilation up?

@LostKobrakai LostKobrakai force-pushed the pr/native-ffi-modules-app-file branch from 81ce392 to f898192 Compare October 30, 2024 17:18
@LostKobrakai LostKobrakai marked this pull request as ready for review October 30, 2024 17:19
@LostKobrakai
Copy link
Author

Somehow this broke the project_erlang test, but I compared the before and after of the resulting folders and they seems to be the same (besides obvious differences in cache files and the compiler script)

@LostKobrakai LostKobrakai force-pushed the pr/native-ffi-modules-app-file branch from f898192 to 0631852 Compare November 6, 2024 10:09
Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Super nice work!!! Really digging this.

I've left some small notes inline

compiler-core/src/build/package_compiler.rs Outdated Show resolved Hide resolved
compiler-core/src/io/memory.rs Outdated Show resolved Hide resolved

// Check if string contains any characters other than alphanumeric, underscore, or @
let contains_special = s
.chars()
Copy link
Member

Choose a reason for hiding this comment

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

Is this going to work with non-ascii text? I think it's iterating chars rather than unicode graphemes.

Would be nice to teuse the same characters iterator as above rather than making a second one, and also to reduce some nesting

let mut chars = s.chars();

let Some(first) = chars.next() else {
    return "".into()
};

if !first.is_ascii_lowercase() || chars.any(needs_escape) {
    ...

Copy link
Author

Choose a reason for hiding this comment

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

I have to be fair, most of that helper was written by claude with me not being a rust dev. You're certainly right that this doesn't check for graphemes, but it likely should. Not sure how one would best deal with utf8 in rust.

Copy link
Member

Choose a reason for hiding this comment

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

There's a grapheme iterator we use elsewhere in the codebase, we could reuse that here

compiler-wasm/src/wasm_filesystem.rs Outdated Show resolved Hide resolved
compiler-cli/src/beam_compiler.rs Outdated Show resolved Hide resolved
compiler-cli/templates/gleam@@compile.erl Outdated Show resolved Hide resolved
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.

2 participants