rabbitmq / rabbitmq-erlang-client

Erlang client for RabbitMQ
https://www.rabbitmq.com/
Other
184 stars 127 forks source link

Using amqp_client as dependency - no debug_info in built modules #32

Closed binarin closed 8 years ago

binarin commented 8 years ago

When starting erlang.mk project from scratch with amqp_client in dependencies, every module from this library is built without debug_info. Only amqp_client modules are affected, e.g. modules from rabbit_common has this option.

Minimal steps to reproduce:

mkdir amqp_test
cd amqp_test
wget https://raw.githubusercontent.com/ninenines/erlang.mk/master/erlang.mk
make -f erlang.mk bootstrap
perl -pi -e 's/^(?=include)/DEPS += amqp_client\n/' Makefile
make
make shell
% no debug_info for every module from amqp_client
proplists:get_value(options, amqp_channel:module_info(compile)). 

% but modules from other repos, e.g. from rabbit_common has debug_info
proplists:get_value(options, rabbit_channel:module_info(compile)).
binarin commented 8 years ago

@essen ping. Exactly as you were asking on twitter :smile:

I've looked into it, deps/amqp_client/erlang.mk contains only single line:

include ../../erlang.mk

I've added

ERLC_OPTS = +debug_info

there and everything started to work. It looks like that line should be added automatically like at https://github.com/ninenines/erlang.mk/blob/06e9fbf4a93d182328c52503b187c4e83c9dc085/core/deps.mk#L136 - but it didn't.

essen commented 8 years ago

I'll take a look on Monday.

essen commented 8 years ago

There is no issue with the patching because this is what is run: https://github.com/ninenines/erlang.mk/blob/06e9fbf4a93d182328c52503b187c4e83c9dc085/core/deps.mk#L124

Looking into why ERLC_OPTS is empty now.

essen commented 8 years ago

The problem comes from the most recent rabbit-common commit, this file in particular: https://github.com/rabbitmq/rabbitmq-common/commit/150fbe197512b5b3ecfffed55afbbc3811c06c83#diff-ab44287f26c99529cf030b1469c0f69dR1

Doing this unfortunately overrides ERLC_OPTS because the defaults are defined only after, using ?=. The defaults are not applied because += takes precedence. This file probably should set the entire variable including +debug_info and so on rather than try to += to what is so far an empty variable.

essen commented 8 years ago

@dumbbell If you're OK with my assessment I can send a patch that defines ERLC_OPTS fully in that file.

michaelklishin commented 8 years ago

@essen @dumbbell is on holiday atm. We definitely trust your judgement when it comes to erlang.mk ;) I personally think fixing the issue and using the exact value is fine; we can correct it later as the change is in master only and won't be in a production release for some time.

essen commented 8 years ago

Alright then.

essen commented 8 years ago

To be honest I think it would be better if Erlang.mk defined this variable earlier (before plugins). I opened a ticket at https://github.com/ninenines/erlang.mk/issues/502 to track this issue.

In the meantime I will simply temporarily define ERLC_OPTS in the amqp_client's Makefile on master, with a note saying to remove it once Erlang.mk gets fixed and updated (no problems if it's not removed though, it'll just become redundant).