Add gleam support to Mix - #14262
Conversation
421dc7c to
0afa895
Compare
|
@josevalim when working on this I was always thinking of |
|
It should be possible because we use heroicons, a regular JS package, as a dependency on Phoenix applications. So we could probably support a |
|
Hello! You shouldn't need to do anything special target-wise as Gleam performs dead code elimination for code that is for other targets.
I would not expect Mix to become a front-end build tool by adding Gleam support. I think the workflow should be the same as it is today etc, with the desired build tool (esbuild, webpack, Adding frontend dependencies to the BEAM application dependency tree would either be wasted work compiling and managing them, or the programmer would need to go through every dependency in the lock file and add configuration to their
There is no such config. A Gleam package doesn't have an explicit target, each function in the public API is available on Erlang, JavaScript, or both, and they get either compiled or eliminated as needed. Whether a package is for Erlang or JavaScript is typically a matter of how the dependant is using them, not a matter of the configuration of the dependency. There is a |
b91ae71 to
c181ecd
Compare
a18bf89 to
26c8aff
Compare
8a7d9fa to
360db23
Compare
|
I am kind of copypasting this from the Gleam discord in case someone wants to try this out before merging into main: What is this?These changes add first-class support for Gleam in Elixir's Mix, thus allowing gleam path dependencies in What's missing?Probably nothing. Work on the gleam compiler is already merged and work on the Mix side is completed, but there might be rough edges, that's what we need to test. InstructionsMix integration will expect you to have gleam >= 1.10.0 installed and that hasn't been released yet. So:
If you were using UsageAfter running What needs to be tested?
ThanksJust report back with your findings and thanks 🙇🏽 Note: I hope these instructions are enough and correct. Let me know if that's not the case. |
|
@Papipo thanks for all the work so far! One question: I thought that |
No, gleam is a single binary written in Rust. I don't know if we can provide a hex archive maybe? I don't know how these work. How is rebar installation handled? I know that mix is able to install it locally, but I am not sure what that means exactly. It's still a binary as well, right? |
|
For Rebar, we have some binaries which we upload to hex.pm, and we version control them every year or so. Although I don't think this will work for Gleam, because people want to upgrade it more frequently, and Rebar is only a build tool, not really a compiler. So I guess we need to be clear we are using the system one and it is up to them to enforce the team uses the same version across the board (which is how we deal with |
In fact the binary is also the LS, so you definitely want to have full control of the version you are using, etc. |
|
Quoting @lpil on the forums:
Apologies but I am a bit confused. We are using The .app file has things like app name, version, description, registered processes, app environment, and module names. We would need to lift all of this information from the Gleam .toml and make assumptions on how Gleam wants those be used. Worst case scenario, couldn't Gleam support an |
|
Yes, From @lpil comment over elixirforum I assume the problem are colocated Elixir files within a Gleam project. The Gleam compiler knows nothing about them so any modules they define can't be written in the .app file, I guess. What I have done here is use the Mix task to generate the .app file (and inject the two options gleam.toml supports for the erlang target: Let me know if this looks good or if I should revert it. Thanks. |
|
Yes, for collocated projects that’s the responsibility of Mix, but for packages I would say that’s Gleam responsibility. Otherwise there is even more we need to understand from Gleam TOML’s and we may need to keep track of it as it evolves. So is there a chance for it to emit an .app file either by default or via a flag? On the plus side, if Rebar wants to integrate Gleam as well, they can reuse it to emit apps too. :) |
b70f8fd to
b64f23a
Compare
|
Apparently this seems to be working fine except that people sometimes need to compile twice . Could this be code path related maybe? It's probably something stupid I missed. |
|
Is there anything blocking this or something I can help with? |
|
Hi @inoas! It is a 100% blocking on us. Both this and #14352 requires us to familiarize with the code and test it under different scenarios, which takes time. Bugs always take higher priority and the remaining of our development time goes almost fully into the type system, which delays other features. We will move them forward once we are able to. |
| with: | ||
| otp-version: ${{ matrix.otp_version }} | ||
|
|
||
| gleam-version: "1.11.1" |
There was a problem hiding this comment.
Testing all versions seems to much. I guess it would make sense to just keep it up to date.
|
I usually don't do this, but I really need this one to be merged soon. Is there really nothing that us outsiders can do to help speed this one up? |
|
@spicychickensauce I took 2h from my weekend to take a quick look at the PR. Please see below. Apologies for the length of the text but technical discussions like these require a lot of considerations. I have added a TL;DR on top for convenience. TL;DR
Feedback
|
|
And before I forget, thank you @Papipo, @eksperimental, @dvic for implementing, reviewing, debugging, etc! ❤️ |
|
Oh my, now I feel bad for making you miss part of your weekend 🥺. |
|
@josevalim I have made the requested changes. For now the minimal required version of gleam is 1.19.0, but I don't know if my changes will make it. I will update the version if that's not the case. I ran a test with a locally compiled gleam with my fork and all tests still pass. With gleam 1.18 and the latest changes from the last commit in this PR they don't. |
| vsn: to_charlist(version) | ||
| ] | ||
| |> merge_project_application(project) | ||
| |> merge_project_application(project, config[:application]) |
There was a problem hiding this comment.
Do we still need the changes to this file?
There was a problem hiding this comment.
No, but that added support for this. Let me know if we should remove this altogether or if we want to keep it. I can extract the feature into its own PR.
There was a problem hiding this comment.
Right, we should remove it from here indeed. Please open up a discussion around it on the mailing list with potential use cases!
| "--out", | ||
| out, | ||
| "--lib", | ||
| lib |
There was a problem hiding this comment.
I know it is WIP but we likely want the --prod flag here too.
There was a problem hiding this comment.
I have added it and made the needed adjustments. Support for --prod in gleam is a separate PR but here locally I have everything working fine and tests passing.
| def load_config(dir) do | ||
| File.cd!(dir, fn -> | ||
| with {:ok, output} <- | ||
| gleam(~W(export package-information --out /dev/stdout)), |
There was a problem hiding this comment.
Will --out /dev/stdout work on Windows? 🤔
There was a problem hiding this comment.
We need a tmp file I am afraid. So I guess it's plain better to use a tmp file in all platforms, right?
There was a problem hiding this comment.
Some Unix tools allow "-" to mean stdin/stdout. Perhaps the same could be added to gleam? There should be a way to export package information to stdout!
There was a problem hiding this comment.
yeah, we were talking about that on the gleam discord. I'll see what I can do.
| case Map.has_key?(json, "dev_dependencies") do | ||
| true -> | ||
| Map.get(json, "dev_dependencies", %{}) | ||
|
|
||
| # Old format, for compatibility purposes | ||
| false -> | ||
| Map.get(json, "dev-dependencies", %{}) | ||
| end |
There was a problem hiding this comment.
| case Map.has_key?(json, "dev_dependencies") do | |
| true -> | |
| Map.get(json, "dev_dependencies", %{}) | |
| # Old format, for compatibility purposes | |
| false -> | |
| Map.get(json, "dev-dependencies", %{}) | |
| end | |
| json["dev_dependencies"] || json["dev-dependencies"] || %{} |
However, note a package runs in production by default, and for this reason we don't include its dev dependencies. It is likely that we can remove this code altogether.
| dest = MixTest.Case.tmp_path(fixture) | ||
| File.mkdir_p!(dest) | ||
| File.cp_r!(source, dest) | ||
| end) |
There was a problem hiding this comment.
Why do we need these changes? This is setting up fixtures but the fixtures already exist. Copying to tmp happens during the test.
There was a problem hiding this comment.
I can't even recall why this was added
There was a problem hiding this comment.
We should probably get rid of this fixture, as we are venturing into testing Gleam behaviour.
|
@Papipo thank you, I did one initial pass! |
|
Btw, this looks much better, as we depend way less on Gleam internals! |
|
@josevalim I am also removing the check for the per-package required gleam version. When mix tries to compile the package, the gleam command will already make this check so I think it's redundant. |
|
@josevalim everything has been dealt with except the windows thingy. I'll come back once that is settled. |
0a1b51f to
33380fd
Compare
This PR adds support for the gleam language.
gleam.tomlinpathdepsgleamin deps loader.deps.compilegleambinary version (hardcoded to 1.9.0 for now, see below)gleambinarymainalready but not yet released):applicationoption inMix.ProjectStack.push(thus anapplicationfunction is not needed)Notes:
gleambinary version requirements are handled automatically when executinggleam compile-package.gleam export package-infobut I can't know the reason for a bad exit status (maybe the dep path was wrong and the command was run in a dir without agleam.toml).is not yet released but has already been merged. Should land on v1.10.0.Was released on v1.10.0