Skip to content

Commit

Permalink
Do not check for configured apps when build path is set
Browse files Browse the repository at this point in the history 
If the build path is set, we assume it is being shared
and therefore we cannot check for applications as we
may have both false positives and false negatives.

Therefore we let the application that owns the build path
to ultimately perform the check.
  • Loading branch information
José Valim committed Nov 28, 2015
1 parent 00042b8 commit 7963398
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 2 deletions.
1 change: 0 additions & 1 deletion lib/mix/lib/mix/config.ex
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,6 @@ defmodule Mix.Config do
[log_level: :info, mode: :truncate, threshold: 1024]
This final configuration can be retrieved at run or compile time:
Application.get_all_env(:lager)
Expand Down
8 changes: 7 additions & 1 deletion lib/mix/lib/mix/tasks/app.start.ex
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,13 @@ defmodule Mix.Tasks.App.Start do

type = type(config, opts)
Enum.each apps, &ensure_all_started(&1, type)
check_configured()

# If there is a build path, we will let the application
# that owns the build path do the actual check
unless config[:build_path] do
check_configured()
end

:ok
end

Expand Down
9 changes: 9 additions & 0 deletions lib/mix/lib/mix/tasks/compile.ex
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,15 @@ defmodule Mix.Tasks.Compile do
* `:build_embedded` - when `true`, activates protocol
consolidation and does not generate symlinks in builds
* `:build_path` - the directory where build artifacts
should be written to. This configuration must only be
changed to point to a build path of another application.
For example, umbrella children can change their `:build_path`
configuration to point to the parent application's build
path. It is important for one application to not configure
the build path at all, as many checks are not performed
when the build path is configured (as we assume it is shared)

This comment has been minimized.

Sign in to view

Copy link
@myronmarston

myronmarston Nov 28, 2015

Contributor

It is important for one application to not configure the build path at all, as many checks are not performed when the build path is configured (as we assume it is shared)

I don't understand this sentence -- not even enough to recommend an alternate wording to make it more clear. @josevalim can you try rewording it to make it more clear?

This comment has been minimized.

Sign in to view

Copy link
@josevalim

josevalim Nov 28, 2015

Member

I am not sure how to reword it but I did struggle with it. I will try to explain it to you with other words and, if it makes sense, I would appreciate a PR that attempts to improve the wording. :)

The idea is that at least one app should not set the build_path configuration because some checks are disabled (like the warning emitted when you configure an application that does not exist) every time you set the build_path (as setting the build_path means it is being shared).

Is this any better?

This comment has been minimized.

Sign in to view

Copy link
@myronmarston

myronmarston Nov 28, 2015

Contributor

I would appreciate a PR that attempts to improve the wording. :)

I plan to once I understand it :). Your response helps, but I'm not quite there.

To clarify, what you've written applies only to umbrella apps, correct?

The idea is that at least one app should not set the build_path configuration

On what basis should you choose which app to not set the build_path configuration? For example, does it make sense to recommend you choose the app on which the fewest sibling apps depend?

some checks are disabled (like the warning emitted when you configure an application that does not exist) every time you set the build_path (as setting the build_path means it is being shared).

If you set build_path for all but one, would that mean that you would only see these warnings when directly compiling that one from within its directory (and not when compiling at the umbrella app root)? And would you only see the warnings that apply to that one app and not to any other apps?

This comment has been minimized.

Sign in to view

Copy link
@josevalim

josevalim Nov 28, 2015

Member

It does not only necessary apply to umbrellas. The application you choose need to have a full view of all dependencies in a way that all other applications (and their dependencies) are a subset of the "main" application. That's always true for the umbrella parent. Otherwise, you can have false negatives, i.e. you would warn because an application that does not exist but in fact it does, you just can't "see" it.

Only the application that has not configured the build path is the one doing the checks. So you will only see those checks for those applications.

Honestly, it is probably easier to reword this to say: "this should only be used when umbrella applications inside an umbrella application so each child points to the parent _build directory". I.e. it is probably better to say it is an umbrella only feature than document all side-effects which will require you to have something like an umbrella anyway.

Thoughts?

This comment has been minimized.

Sign in to view

Copy link
@myronmarston

myronmarston Nov 28, 2015

Contributor

Honestly, it is probably easier to reword this to say: "this should only be used when umbrella applications inside an umbrella application so each child points to the parent _build directory". I.e. it is probably better to say it is an umbrella only feature than document all side-effects which will require you to have something like an umbrella anyway.

Document it as an umbrella-only feature makes sense to me. I assumed it was, and am working in an umbrella application, so I was confused about how the statement affected our application (now I understand it doesn't). I'm not even sure why you'd want to configure the build path for a non-umbrella case.

Thanks for taking the time to explain! I'll try to open a PR later tonight (it's about 1 pm here) if you haven't gotten to it first.
## Command line options
* `--list` - list all enabled compilers
Expand Down

0 comments on commit 7963398

Please sign in to comment.