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 source of applications from current project #685

Merged
merged 1 commit into from Jul 1, 2019

Conversation

hrubi
Copy link
Contributor

@hrubi hrubi commented Jun 20, 2019

The option include_src: true now includes the source code of
applications which are built from this project. That means:

  • the top-level app
  • umbrella apps
  • dependencies

Fix #683

# Returns `nil` otherwise.
defp project_app_src_root(app_name) do
if not Mix.Project.umbrella?() && Mix.Project.get().project[:app] == app_name do
File.cwd!()
Copy link
Owner

Choose a reason for hiding this comment

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

This will result in including not only source files, but all of the generated build artifacts, including the contents of the release being copied by the current in-progress assembly. It is really only intended that source code be included (so that one can use that source to modify modules and hot load them in production if needed, or at the very least, be able to read the code of the current deployed version if you don't have another means of finding that information).

If you want to bundle the entire project with the release, then I'd recommend doing that as a plugin, rather than as part of Distillery itself (since Distillery can't anticipate all the possible variations of things that may or may not want to be included).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The source code subdirectories get filtered in project_app_scr_dirs: https://github.com/bitwalker/distillery/pull/685/files#diff-f54ed4fbf4049c971597b8a1e8d04ca1R235.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, that's fine then. The other problem though is in the other branch: It misses the case where a project is an umbrella. We should find the app directory in the umbrella (using the Mix.Project config option as the base path and joining the app name) and use that as the source path. Unless I'm missing why that case was skipped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, this is not obvious from the code, I should've added a comment. Mix.Project.deps_paths/0 returns a map with all the deps and also all the umbrella apps. I've tried this (showing only the relevant output):

$ mix new myubrella --umbrella
$ cd myubrella/apps
$ mix new myapp1
$ mix new myapp2
$ cd ..
$ mix run --eval 'IO.inspect(Mix.Project.deps_paths())'
%{                                                                                                                    
  myapp1: "/home/hrubi/projects/elixir/distillery-include_src/tmp2/myubrella/apps/myapp1",
  myapp2: "/home/hrubi/projects/elixir/distillery-include_src/tmp2/myubrella/apps/myapp2"
}

Next I've verified that with distillery with this patch and include_src: true, the release indeed includes sources of the umbrella apps configured for the release:

$ tar tf _build/prod/rel/myubrella/releases/0.1.0/myubrella.tar.gz | grep 'myapp.*/lib'
lib/myapp2-0.1.0/lib/myapp2.ex
lib/myapp1-0.1.0/lib/myapp1.ex

Is this what you thought? Should I add the comment at least? Or do you suggest some other approach?

Copy link
Owner

Choose a reason for hiding this comment

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

I would add a comment to the change that explains that - I recall the reasoning behind why that occurs, but it is definitely something I or others may miss again later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added, please check if it's fine.

The option `include_src: true` now includes the source code of
applications which are built from this project. That means:

  - the top-level app
  - umbrella apps
  - dependencies

Fix bitwalker#683
@bitwalker bitwalker merged commit aff7e6e into bitwalker:master Jul 1, 2019
@bitwalker
Copy link
Owner

Thanks!

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.

include_src: true does not include source of Elixir apps
2 participants