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

Dev dependencies are written into the app list in .app.src #3035

Open
Nicd opened this issue Apr 23, 2024 · 10 comments
Open

Dev dependencies are written into the app list in .app.src #3035

Nicd opened this issue Apr 23, 2024 · 10 comments
Labels
bug Something isn't working help wanted Contributions encouraged priority:high

Comments

@Nicd
Copy link

Nicd commented Apr 23, 2024

See the gleam.toml in https://preview.hex.pm/preview/bigi/3.0.0/show/gleam.toml, gleeunit is only a development dependency. But in the created .app.src, gleeunit is included as a dependency application: https://preview.hex.pm/preview/bigi/3.0.0/show/src/bigi.app.src.

Context: I managed to get bigi compiling in Elixir by putting {:bigi, "~> 3.0", manager: :rebar3} in mix.exs, but trying to run fails due to not having gleeunit. If I edit _build/dev/lib/bigi/ebin/bigi.app and remove gleeunit manually, I can open IEx and run bigi code successfully! :)

zsh/2 486 % iex -S mix
Erlang/OTP 26 [erts-14.1] [source] [64-bit] [smp:20:20] [ds:20:20:10] [async-threads:1] [jit:ns]

Interactive Elixir (1.16.2) - press Ctrl+C to exit (type h() ENTER for help)
iex(1)> :bigi.zero()
0
iex(2)> :bigi.add(1, 2)
3
@Nicd Nicd added the bug Something isn't working label Apr 23, 2024
@Nicd
Copy link
Author

Nicd commented Apr 23, 2024

The compiler (or rebar3?) also warns

===> "/home/nicd/test/gleam_test/_build/dev/lib/gleam_json/ebin/gleam_json.app" is missing kernel from applications list
===> "/home/nicd/test/gleam_test/_build/dev/lib/gleam_json/ebin/gleam_json.app" is missing stdlib from applications list

and indeed kernel and stdlib are not in the app files. I'm not sure if that's an issue or not.

@Nicd
Copy link
Author

Nicd commented Apr 23, 2024

I guess this is the place responsible (suitable TODO here 😁):

// TODO: When precompiling for production (i.e. as a precompiled hex
// package) we will need to exclude the dev deps.
let applications = config
.dependencies
.keys()
.chain(
config
.dev_dependencies
.keys()
.take_while(|_| self.config.include_dev_deps),
)
// TODO: test this!
.map(|name| self.config.package_name_overrides.get(name).unwrap_or(name))
.chain(config.erlang.extra_applications.iter())
.sorted()
.join(",\n ");
let text = format!(
r#"{{application, {package}, [
{start_module} {{vsn, "{version}"}},
{{applications, [{applications}]}},
{{description, "{description}"}},
{{modules, [{modules}]}},
{{registered, []}}
]}}.

@Nicd
Copy link
Author

Nicd commented Apr 23, 2024

Whether dev deps are included is controlled by this line:

include_dev_deps: is_root,

We could turn it to false there, but I suppose that would break functionality if the user wants some app to exist in development but not in production (like phoenix_live_reload type things)? So do we need a dev/prod distinction to proceed with this issue?

@Acepie
Copy link
Contributor

Acepie commented Apr 23, 2024

Would it ever make sense to include dev deps in prod? Cause the project compiler already has the mode for dev vs prod so unless there is a reason we would want to keep dev deps in prod we could always update that line to check mode != Mode::Prod instead since dependencies are all compiled in prod mode already

@Nicd
Copy link
Author

Nicd commented Apr 24, 2024

But, do we ever use the prod mode? I was under the impression that we don't have prod builds. That's what I meant by having a dev/prod distinction, i.e. do we need to start using prod builds?

A lighter alternative could be to detect when building for Hex upload and not including dev deps then?

@Acepie
Copy link
Contributor

Acepie commented Apr 24, 2024

Yeah there are a couple places where the compiler looks at the mode to determine what to do though i know for javascript generation specifically it ignores the mode. For example https://github.com/gleam-lang/gleam/blob/main/compiler-core/src/build/package_loader.rs#L214 the package loader doesn't load test modules in prod. This seems like it would fall under the same category?

@Nicd
Copy link
Author

Nicd commented Apr 24, 2024

Hmm, I guess I'll try it out and see. 👍

@Nicd
Copy link
Author

Nicd commented Apr 24, 2024

I made this change:

diff --git a/compiler-core/src/build/project_compiler.rs b/compiler-core/src/build/project_compiler.rs
index de2f2006..79eea3d6 100644
--- a/compiler-core/src/build/project_compiler.rs
+++ b/compiler-core/src/build/project_compiler.rs
@@ -517,7 +517,7 @@ where
                     .collect();
                 super::TargetCodegenConfiguration::Erlang {
                     app_file: Some(ErlangAppCodegenConfiguration {
-                        include_dev_deps: is_root,
+                        include_dev_deps: self.mode() != Mode::Prod,
                         package_name_overrides,
                     }),
                 }

After running gleam publish and canceling it, this file is created in build/prod/erlang/bigi/ebin/bigi.app:

{application, bigi, [
    {vsn, "3.0.0"},
    {applications, [gleam_stdlib]},
    {description, "Arbitrary precision integer arithmetic for Gleam"},
    {modules, [bigi]},
    {registered, []}
]}.

After running just gleam build, this file is created in build/dev/erlang/bigi/ebin/bigi.app:

{application, bigi, [
    {vsn, "3.0.0"},
    {applications, [gleam_stdlib,
                    gleeunit]},
    {description, "Arbitrary precision integer arithmetic for Gleam"},
    {modules, [bigi,
               bigi_test]},
    {registered, []}
]}.

So it seems to work. Is there something specific I should test? This does not seem to be tested directly as the only mention of include_dev_deps has it hardcoded in the tests and all the tests pass with the change.

@lpil
Copy link
Member

lpil commented Apr 25, 2024

It'd be good to have a test for the contents of the Hex tarball if that's something we don't already have.

@lpil lpil added help wanted Contributions encouraged priority:high labels Apr 25, 2024
@Nicd
Copy link
Author

Nicd commented Apr 25, 2024

There is a test-package-compiler, but it forces include_dev_deps: true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Contributions encouraged priority:high
Projects
None yet
Development

No branches or pull requests

3 participants