erlware / relx

Sane, simple release creation for Erlang
http://erlware.github.io/relx
Apache License 2.0
697 stars 232 forks source link

application start order has changed #569

Closed massemanet closed 7 years ago

massemanet commented 7 years ago

I have a release that consists of two applications. I need one of the applications ('appenv') to start before all the dependencies of the second ('reporter'). This is how relx used to generate the boot script. E.g. in relx 3.21.1 (rebar3 3.3.2) it would start appenv before the dependencies of reporter, but in relx 3.22.2 (rebar3 3.3.5) it doesn't. Is this a bug or a feature? If it is intended bahaviour, how am I supposed to specify that an app should start first?

{relx, [{release, {reporter, {semver,""}}, [appenv, reporter]},

rebar 3.3.2 on Erlang/OTP 18 Erts 7.3

     {progress,applications_loaded},
     {apply,{application,start_boot,[kernel,permanent]}},
     {apply,{application,start_boot,[stdlib,permanent]}},
     {apply,{application,start_boot,[appenv,permanent]}},
...
     {apply,{application,start_boot,[reporter,permanent]}},
     {apply,{c,erlangrc,[]}},
     {progress,started}]}.

rebar 3.3.5 on Erlang/OTP 18 Erts 7.3

     {progress,applications_loaded},
     {apply,{application,start_boot,[erlang_localtime,permanent]}},
     {apply,{application,start_boot,[kernel,permanent]}},
     {apply,{application,start_boot,[stdlib,permanent]}},
...
     {apply,{application,start_boot,[appenv,permanent]}},
     {apply,{application,start_boot,[reporter,permanent]}},
     {apply,{c,erlangrc,[]}},
     {progress,started}]}.
lrascao commented 7 years ago

is appenv declaring it's dependency on the dependencies of reporter?

massemanet commented 7 years ago

appenv depends on nothing. it should run before reporter's dependencies.

On Feb 14, 2017 12:23, "Luis Rascão" notifications@github.com wrote:

is appenv declaring it's dependency on the dependencies of reporter?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/erlware/relx/issues/569#issuecomment-279681610, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAxhp6rmHJTTVqwCuyzeVOawGVJQFCwks5rcY7BgaJpZM4MAU0u .

lrascao commented 7 years ago

is there a specific need for it to run before since it depends on nothing?

massemanet commented 7 years ago

it changes the environment for all other apps depending on where it (i.e. the container that all this lives in) is deployed.

On Feb 14, 2017 13:09, "Luis Rascão" notifications@github.com wrote:

is there a specific need for it to run before since it depends on nothing?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/erlware/relx/issues/569#issuecomment-279691113, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAxhmGrQs8K6w5jHxKDdSOUbaYx2QJeks5rcZmAgaJpZM4MAU0u .

lrascao commented 7 years ago

so other apps seem to depend on it, are you able to add this dependency?

massemanet commented 7 years ago

no, I don't control the dependencies of the other apps (i.e. reporters deps).

lrascao commented 7 years ago

@massemanet shoot me an email when you get that sample

cjkjellander commented 7 years ago

Our problems started when #530 was merged, and rlx_topo was reintroduced. This is the application start order in relx 3.21.1:

 {progress,applications_loaded},
 {apply,{application,start_boot,[kernel,permanent]}},
 {apply,{application,start_boot,[stdlib,permanent]}},
 {apply,{application,start_boot,[sasl,permanent]}},
 {apply,{application,start_boot,[appenv,permanent]}},
 {apply,{application,start_boot,[erlang_localtime,permanent]}},
 {apply,{application,start_boot,[eqw,permanent]}},
 {apply,{application,start_boot,[crypto,permanent]}},
 {apply,{application,start_boot,[asn1,permanent]}},
 {apply,{application,start_boot,[public_key,permanent]}},
 {apply,{application,start_boot,[ssl,permanent]}},
 {apply,{application,start_boot,[lhttpc,permanent]}},
 {apply,{application,start_boot,[base16,permanent]}},
 {apply,{application,start_boot,[eini,permanent]}},
 {apply,{application,start_boot,[jsx,permanent]}},
 {apply,{application,start_boot,[inets,permanent]}},
 {apply,{application,start_boot,[xmerl,permanent]}},
 {apply,{application,start_boot,[erlcloud,permanent]}},
 {apply,{application,start_boot,[eqw_sqs,permanent]}},
 {apply,{application,start_boot,[prometheus,permanent]}},
 {apply,{application,start_boot,[cowlib,permanent]}},
 {apply,{application,start_boot,[ranch,permanent]}},
 {apply,{application,start_boot,[cowboy,permanent]}},
 {apply,{application,start_boot,[promqueen,permanent]}},
 {apply,{application,start_boot,[emo,permanent]}},
 {apply,{application,start_boot,[lejson,permanent]}},
 {apply,{application,start_boot,[leg,permanent]}},
 {apply,{application,start_boot,[mnesia,permanent]}},
 {apply,{application,start_boot,[reporter,permanent]}},
 {apply,{application,start_boot,[eper,permanent]}},
 {apply,{c,erlangrc,[]}},
 {progress,started}]}.

And this is the start order after.

 {progress,applications_loaded},
 {apply,{application,start_boot,[kernel,permanent]}},
 {apply,{application,start_boot,[stdlib,permanent]}},
 {apply,{application,start_boot,[asn1,permanent]}},
 {apply,{application,start_boot,[xmerl,permanent]}},
 {apply,{application,start_boot,[inets,permanent]}},
 {apply,{application,start_boot,[jsx,permanent]}},
 {apply,{application,start_boot,[eini,permanent]}},
 {apply,{application,start_boot,[base16,permanent]}},
 {apply,{application,start_boot,[eqw,permanent]}},
 {apply,{application,start_boot,[prometheus,permanent]}},
 {apply,{application,start_boot,[crypto,permanent]}},
 {apply,{application,start_boot,[mnesia,permanent]}},
 {apply,{application,start_boot,[leg,permanent]}},
 {apply,{application,start_boot,[lejson,permanent]}},
 {apply,{application,start_boot,[emo,permanent]}},
 {apply,{application,start_boot,[erlang_localtime,permanent]}},
 {apply,{application,start_boot,[public_key,permanent]}},
 {apply,{application,start_boot,[cowlib,permanent]}},
 {apply,{application,start_boot,[ssl,permanent]}},
 {apply,{application,start_boot,[lhttpc,permanent]}},
 {apply,{application,start_boot,[ranch,permanent]}},
 {apply,{application,start_boot,[erlcloud,permanent]}},
 {apply,{application,start_boot,[cowboy,permanent]}},
 {apply,{application,start_boot,[promqueen,permanent]}},
 {apply,{application,start_boot,[eqw_sqs,permanent]}},
 {apply,{application,start_boot,[sasl,permanent]}},
 {apply,{application,start_boot,[appenv,permanent]}},
 {apply,{application,start_boot,[reporter,permanent]}},
 {apply,{application,start_boot,[eper,permanent]}},
 {apply,{c,erlangrc,[]}},
 {progress,started}]}.

And this is with a relx config like so:

    {relx, [{release, {reporter, {semver,""}},
     [sasl, appenv, reporter, eper]},
    {dev_mode, false},
    {include_erts, true},
    {sys_config, "rel/sys.config"},
    {vm_args, "rel/vm.args"},
    {extended_start_script, true}]}.
cjkjellander commented 7 years ago

What we really would like is for sasl and appenv to start before any deps of reporter.

lrascao commented 7 years ago

are you able to provide a simple test project that exhibits this behaviour so i can try it out?

cjkjellander commented 7 years ago

https://github.com/campanja-forks/relx-systool#relx-systool

Just type make and tail the relxsystool.script.

 {progress,applications_loaded},
 {apply,{application,start_boot,[kernel,permanent]}},
 {apply,{application,start_boot,[stdlib,permanent]}},
 {apply,{application,start_boot,[asn1,permanent]}},
 {apply,{application,start_boot,[crypto,permanent]}},
 {apply,{application,start_boot,[public_key,permanent]}},
 {apply,{application,start_boot,[cowlib,permanent]}},
 {apply,{application,start_boot,[ssl,permanent]}},
 {apply,{application,start_boot,[ranch,permanent]}},
 {apply,{application,start_boot,[sasl,permanent]}},
 {apply,{application,start_boot,[cowboy,permanent]}},
 {apply,{c,erlangrc,[]}},
 {progress,started}]}.

Here I'm trying to get sasl to start before all the deps of cowboy.

lrascao commented 7 years ago

I made some changes in PR #1 (https://github.com/campanja-forks/relx-systool/pull/1), basically it amounts to create an OTP app and declare it as the goal of the release in rebar.config, does that translate well to your use-case?

cjkjellander commented 7 years ago

Nope, it still doesn't satisfy that sasl should start before all cowboy dependencies.

 {progress,applications_loaded},
 {apply,{application,start_boot,[kernel,permanent]}},
 {apply,{application,start_boot,[stdlib,permanent]}},
 {apply,{application,start_boot,[asn1,permanent]}},
 {apply,{application,start_boot,[crypto,permanent]}},
 {apply,{application,start_boot,[sasl,permanent]}},
 {apply,{application,start_boot,[public_key,permanent]}},
 {apply,{application,start_boot,[cowlib,permanent]}},
 {apply,{application,start_boot,[ssl,permanent]}},
 {apply,{application,start_boot,[ranch,permanent]}},
 {apply,{application,start_boot,[cowboy,permanent]}},
 {apply,{application,start_boot,[relxsystool,permanent]}},
 {apply,{c,erlangrc,[]}},
 {progress,started}]}.

So there is still no way of guaranteeing that sasl starts first. (after kernel and stdlib of course)

lrascao commented 7 years ago

Both asn1, crypto and sasl only depend on [kernel, stdlib] as declared in their project's .app.src files so it really shouldn't matter the order in which they are started. The original issue here, i think, was that appenv doesn't declare it's dependency on any other application (in .app.src), however it needs to run before all others which is sort of an implicit dependency, right? If all deps of reporter declare their dependency on sasl, they should start after it

f355 commented 7 years ago

It is not a dependency per se, because nothing in reporter uses anything that appenv provides, more than that, reporter is unaware of appenv completely.

The reason why we want appenv to start before everything else is because all it does is setting other applications' environment variables conditionally (like setting the log level in sasl based on production/dev environment), and it needs to do so after the applications are loaded, but before they are started.

Most of the time .app file dependencies are just enough if one wants to include several apps in the release, there are very few use cases when using relx configuration instead is actually helpful, so it's disappointing to see one of these rare cases being broken.

ferd commented 7 years ago

The proper way to do that may be to start the apps as included applications, though I'm not sure what the implications of that are for relups

f355 commented 7 years ago

Included applications? I am aware of the feature, but it doesn't seem to be widely used and carries additional overhead.

There are many ways to pre-configure applications, but we have used this one, for better or for worse. It used to work before, it does not work now. Neither of the two startup orders is inherently better than the other. This issue is a regression, albeit spotted pretty late.

I will gladly take a look at rlx_topo.erl and send a PR that restores the previous order, but I don't want to spend my time on something that will get rejected right away or put on the back-burner because no one wants to merge it, so I want to make sure you guys understand the issue first and agree that it needs a fix.

lrascao commented 7 years ago

The topo sort was brought back to fix #511 and it's been (so far) working as expected (the issue has some context, basically the generated start order was inverted), i propose we come up with a special solution for this special case. What about a new relx directive that specifies a list of applications that override the generated start order, so in your case you would set appenv that would end up at the top of the start list, does that sound reasonable?

f355 commented 7 years ago

Thanks for the suggestion. I think I'll try to fix the current topo sort to behave like I think it should, and if it comes out too ugly or something - let's discuss the new directive or other options.

Expect a PR in a few days :)

f355 commented 7 years ago

I have made an attempt at it in #606, please take a look.

lrascao commented 7 years ago

closing via #606