Add optional_applications to .app resource files

[josevalim]

Both Mix and Rebar allow some applications to be absent
at runtime - sometimes also known as optional dependencies.

However, given optional applications are not stored in .app
resource files, releases do not consider optional applications
in its boot order, leaving it up to chance if an optional app
will be started before its parent.

Users can try to explicitly list optional applications on their
release definition files, but given the order is not enforced,
this manual specification may be reordered when new apps
are added, leaving developers with broken releases.

This PR introduces the "optional_applications" field to .app
resource files. This field must be a subset of the "applications"
field. The reason why it is a subset rather than its own list
of applications is exactly to keep application start order
on "application:start/2" and during releases.

If application "b" is an optional application for application "a",
and application "b" is missing, "application:start(a)" will still
succeed.

If application "b" is an optional application for application "a",
and application "b" is available, "application:ensure_all_started(a)"
will automatically start application "b" before "a".

[tsloughter]

Big +1 from me.

But to be clear, at least with relx/systools, the order is not subject to be "reordered at any time". A relx release definition passed to systools uses a stable sort even for the top level applications listed for the release. So you can ensure a dependency is started before another by using this. The issue with it is just that it is that it is annoying and pushed on to the end user to get right, meaning every time it is important it must be documented for that particular project, like OpenTelemetry does https://github.com/open-telemetry/opentelemetry-erlang#using

[tsloughter]

So scratch that, I'm a "big +1" only with the change I mention in my review comments -- without it, assuming it at least correctly fails in the case of a release when the included optional dep fails to boot, I'm a small +1 :)

I believe in the case of a release the boot script does a start on each application so it won't be the case that it would silently skip an optional dep that fails to boot. Which I think is correct. So the issue is also the different result to the user whether they are booting up locally for testing with like rebar3 shell or running a release -- in one case it silently ignores a failed boot and in the other it errors out the whole boot process.

[josevalim]

Thanks @tsloughter for the review. "at any time" was poorly phrased on my end, I should have said "at any time new dependencies are introduced", so I have edited it accordingly. Your other feedback is also great, I will do those changes and push them soon.

[josevalim]

All feedback from @tsloughter has been addressed. Docs have been amended and tests have been improved.

[josevalim]

Pushed support for optional applications on systools too.

[garazdawi]

We'll take a look at this once vacation times are over, i.e. sometime in august.

[tsloughter]

Bumping this :)

[sverker]

I think reltool needs updating too. For example reltool_target:sort_apps/4 contains the same algorithm as systools_make:sort_appls/4.

[sverker]

What do you think about allowing applications in optional_applications that are not part of applications. That would be optional library applications that do not need to be started. By allowing that, the optional_applications list can also serve the runtime_dependencies list.

[tsloughter]

@sverker do you have examples of how runtime_dependencies are used? You say "do not need to be started", is this referring to an application that may or may not be used? Like they are only used if a user triggers a specific action and you don't want the application to start unless such an action is taken?

That is the only thing I could think of as being a reason to include but not start an application.

[ferd]

One way we use applications in a lot of places for rebar3 is to use it as the path of "what will end up in the release". xref analysis, for example, has shifted to only using these. More importantly, releases also build using these for transitive dependencies.

The gotcha is that using optional_applications as a disjoint set from applications is that you risk losing backwards compatibility with libraries relying on it as you move forwards with support for things. Having the sets overlap is trickier for declaration, but allows easy backwards compatibility with people on older toolchains using newer apps.

[josevalim]

@sverker I have added reltool support. Note that in sort_apps we sort the applications field of each individual .app file. I chose to have the missing optional applications come last. While the order of these missing optional applications should not matter, I could have taken the other approach of removing all missing optional applications from the applications entry. I decided it is probably best to keep all applications in the list though, instead of silently removing them.

Regarding the adding support for optional applications not in the applications list, is it ok if we have this discussion later on? I am hoping this patch is functionally complete and if we add more scenarios, we can do so in a separate discussion.

[tsloughter]

I could have taken the other approach of removing all missing optional applications from the applications entry.

They should be removed. Unless I'm misunderstanding. Removal from the .app file is definitely what relx will do with this feature.

[josevalim]

I briefly talked to @tsloughter on IRC and, to make sure we are all on the same page, keeping the missing optional applications is not an issue because we are keeping both applications and optional_applications in the release app files and the application_controller knows that these missing apps are optional and the system should still boot just fine.

[rickard-green]

@tsloughter

@sverker do you have examples of how runtime_dependencies are used?

runtime_dependencies are used by the otp_patch_apply tool to determine that runtime dependencies for the applications to patch are satisfied in the installed system being patched. The user can also verify that all runtime dependencies are satisfied using system_information:sanity_check()

runtime_dependencies list all direct dependencies to other applications that may be needed. These dependencies also include minimum version requirements. That is, optional as well as required dependencies. A way to inform about which dependencies are optional (apart from writing it in the documentation) has been requested multiple times.

You say "do not need to be started", is this referring to an application that may or may not be used? Like they are only used if a user triggers a specific action and you don't want the application to start unless such an action is taken?

Yes this, and library applications which have no requirement to be started.

We also have the scenario with mutual dependency. In that case it would be problematic for both applications to list each other under applications in their .app files. The documentation say "All applications that must be started before this application is allowed to be started" which also solves this otherwise problematic scenario. That is, we potentially have cases where an application should not be listed in applications even though it must be started.

ssl and inets is an example of applications with mutual dependency. Both include each other in their runtime_dependencies. None of them, however, include each other in applications since this is optional functionality.

@josevalim

Regarding the adding support for optional applications not in the applications list, is it ok if we have this discussion later on? I am hoping this patch is functionally complete and if we add more scenarios, we can do so in a separate discussion.

I'm not too fond of merging things without ironing out the details. I also think it would be really unfortunate to unnecessarily have to introduce yet another parameter if we can use this new one to solve both problems. I'm not sure using optional_applications for both is the best way, but I think we should have the discussion.

[tsloughter]

opentelemetry is also a good example.

If only the opentelemetry_api applications is included then nothing is recorded, it is all noops. But if opentelemetry is included then it needs to be booted before everything else so that it is ready to handle starting and ending spans before any application that uses it.

At this time we simply document that you need to include it in the release list of apps before your applications that use opentelemetry https://github.com/open-telemetry/opentelemetry-erlang#including-in-release

If any app that used it could put it in their applications list but also in the optional_applications list they could ensure it is started without configuring it in the release's list of applications (though optionally setting it to temporary would still be documented for users who want to keep it from ever taking down their node, but that would be separate from boot order, so easier to explain and document.)

[IngelaAndin]

OK, I understand that it could be important to start an optional application if it exists. And as I understand it
that is what will happen, but it will not fail if the optional application does not exist which is good for the persons that does not want that application at all.

We also need to consider the case of circular optional dependencies, which I can agree on should be avoided, but they do exist and might not be so easy to resolve (as inets (optional HTTPS) and ssl (optional CRL handling with HTTP)), a required start will not work in this case. There are probably other such cases as this is something that was not previously disallowed, even if it is in rebar case.

[garazdawi]

There are a bunch of testcase failures in sasl related to this change, mostly in systools_SUITE and release_handler_SUITE.

[josevalim]

Thanks @garazdawi, I will investigate.

[garazdawi]

I also suspect system:upgrade_SUITE may fail as a result of this change, though it does not fail on all our machines, so I'm not 100% sure.

*** CT Error Notification 2020-10-27 04:11:29.734 ***
upgrade_SUITE:upgrade_system failed on line 235
Reason: {badmatch,error}

Full error description and stacktrace




=== Ended at 2020-10-27 04:11:29
=== Location: [{upgrade_SUITE,upgrade_system,235},
              {upgrade_SUITE,upgrade_test1,144},
              {test_server,ts_tc,1754},
              {test_server,run_test_case_eval1,1263},
              {test_server,run_test_case_eval,1195}]
=== === Reason: no match of right hand side value error
  in function  upgrade_SUITE:upgrade_system/6 (upgrade_SUITE.erl, line 235)
  in call from upgrade_SUITE:upgrade_test1/3 (upgrade_SUITE.erl, line 144)
  in call from test_server:ts_tc/3 (test_server.erl, line 1754)
  in call from test_server:run_test_case_eval1/6 (test_server.erl, line 1263)
  in call from test_server:run_test_case_eval/9 (test_server.erl, line 1195)

[garazdawi]

Removed from testing while waiting for testcases to be fixed.

[josevalim]

@garazdawi just a quick update. I am unable to reproduce the sasl failures locally, I am investigating why that is the case.

[garazdawi]

I don't know if it makes any difference in this case, but it is always good to test sasl using a released Erlang/OTP and not the source tree. i.e. make release and use release/x86....*/bin/ as your erl when testing.

[josevalim]

This is where the bug was! The reason it passed locally is because apparently I had fixed it 10 days ago but I forgot to push. :D

All tests pass for me now (edit: all tests in sasl) except for 5 tests in systools because they can't find "lib/kernel-7.1". I assume this is exactly because I was running the tests from $ERL_ROOT/bin/erl. I have tried the approach described by @garazdawi by doing this:

$ make release
$ cd releases/x...
$ ./Install `pwd`
$ cd ../test/test_server
$ ../../x.../bin/erl
erl> ts:install().
erl> ts:run(sasl, [batch]).

But unfortunately the suite never started. I assume I messed up the installation somehow.

[garazdawi]

You may need to add it to the PATH for the test to catch the correct erl.

I'll re-add this to the tests tomorrow in order to get one night without this patch to more easily see which testcases fail because of it.

[josevalim]

You may need to add it to the PATH for the test to catch the correct erl.

It has ran now! We are down to one failure in release_handler_SUITE:client1 with reason {badmatch,{badrpc,{'EXIT',{disallowed_function_call,{trace,<...}. It seems to be unrelated to this PR.

[josevalim]

Hi! 👋 I have rebased the PR to latest master. Please let me know if there is anything I can do to move this forward. Thank you!

[sverker]

@josevalim I have rebased your single commit on top of a fresh master and force pushed it to jv-opt-apps in your repo. (You had done a merge of master into topic branch which we usually try to avoid).

I will put it into test again and if no more issues merge it.