Use target_dir as source of new version in generate-appups #273

Merged
merged 1 commit into from May 12, 2015

Projects

None yet

6 participants

@cybergrind
Contributor

Not sure what exact reason of setting NewVerPath as rel + first rel name from reltool.config so trying to make this command more correct.

@cybergrind cybergrind changed the title from Trait target_dir as source of new version in generate-appups to Use target_dir as source of new version in generate-appups May 6, 2014
Contributor
tuncer commented May 7, 2014
Contributor

This issue is still actual.

Contributor

This seems okay but I haven't looked at any of this in a very long time so I have no idea.

Contributor

Just to clean how it works now and how it will work after fix.

Example reltool.config (without redundant parts)

{sys, [
       .....
       {rel, "test1", "1", ... },
       {rel, "test2", "1", ... }
....
]}.
{target_dir, "release_1"}.

Current:

  1. rebar generate will generate release in directory: release_1
  2. rebar generate-appups previous_release=release_0 will generate appups from release_0 => test1 (unsuccessfully)
    Note: it will set NewVerPath to first tuple with rel in sys

After fix:

  1. same as before
  2. rebar generate-appups previous_release=release_0 will generate appups from release_0 => release_1 (target_dir)
Member

I've never used rebar generate or rebar generate-appups and I take it we don't have good appup testing? So I'm fine merging this but also would like to hear from someone else who actually uses this feature first.

@ferd ferd added the enhancement label Jun 16, 2014
Contributor
tuncer commented Apr 2, 2015

/cc @lrascao

Contributor
lrascao commented Apr 2, 2015

My reltool.config only has one rel entry in sys, so it doesn't really affect my use case. The proposed fix works fine.
From what i could understand from the code (rebar_appups.erl) as it stands on master right now:

  • through rebar_rel_utils:get_target_parent_dir, target_dir is read from reltool.config, and obtains parent dir (eg. _rel/release_1 returns _rel)
  • rebar_rel_utils:get_reltool_release_info expects a single {rel, ...} tuple, it extracts the release name from it and appends it to the target parent dir

@cybergrind probably caught this because he's using multiple rel entries in the sys tuple, looking at the doc at http://www.erlang.org/doc/man/reltool.html i see this is allowed. It appears that the root of the issue is actually the fact that rebar is not allowing more than one release, the issue now is if this was an intentional design decision.
@cybergrind, do you have an actual use case that we could look at with a reltool.config and multiple rel entries?

Contributor

Sure

{sys, [
       {lib_dirs, ["../deps"]},
       {erts, [{mod_cond, derived}, {app_file, strip}]},
       {app_file, strip},
       {rel, "igr", "0.9",
        [
         kernel,
         stdlib,
         sasl,
         lager,
         folsom,
         folsom_unix,
         igr_worker,
         http_api
        ]},
       {rel, "start_clean", "",
        [
         kernel,
         stdlib
        ]},
       {boot_rel, "igr"},
       {profile, embedded},
       {incl_cond, derived},
       {excl_archive_filters, [".*"]}, %% Do not archive built libs                                                                                                           
       {excl_sys_filters, ["^bin/.*", "^erts.*/bin/(dialyzer|typer)",
                           "^erts.*/(doc|info|include|lib|man|src)"]},
       {excl_app_filters, ["\.gitignore"]},
       {app, igr_worker, [{mod_cond, app}, {incl_cond, include},
                          {lib_dir, "../apps/igr_worker"}]},
       {app, http_api, [{mod_cond, app}, {incl_cond, include},
                          {lib_dir, "../apps/http_api"}]}
      ]}.

{target_dir, "igr"}.

My main concern that if we have target_dir for setup our output directory - we have to use it for all operations that have no additional settings without implicit cases (like generate-appups).

Contributor
lrascao commented Apr 4, 2015

So i looked at the original commit and really can't understand the reason for the parent target dir + first rel name from reltool.config approach for appups, @cybergrind's change looks sane. Would be nice to get more input from people who actually use appups but appears there aren't much around.

Contributor
tuncer commented May 12, 2015

ping?

Contributor
lrascao commented May 12, 2015

i don't really have a lot to add other that what i've already said, so 👍 by me

@ferd ferd merged commit 791db71 into rebar:master May 12, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment