New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove --without-common_test flag #3

Merged
merged 1 commit into from Feb 2, 2017

Conversation

Projects
None yet
2 participants
@jhchabran
Contributor

jhchabran commented Jan 27, 2017

Hi Paul,

First be aware that I'm not yet very familiar with building Erlang.

We've been facing issues while compiling rabbitmq dependencies when working with Elixir

/app/deps/rabbit_common/src/rabbit_ct_broker_helpers.erl:19: can't find include lib "common_test/include/ct.hrl"

After investigating, we've found that it boils down to common_tests being skipped when Erlang is compiled in alpine-erlang. Re-adding leads to just a 3mb larger image.

I don't know if that makes sense to merge this, given my understanding is:

  • that using releases (with distillery thanks for writing it 馃挭 ) would negates the problem in the problem in the first place as we'd be compiling locally, where we'd have common_test included and then just placing the release in the docker image.
  • still, if we'd be building releases in docker images based on alpine-erlang we'd hit the same problem (it's simpler to build releases in docker images and copying it in another one than having to deal with OTP versions directly on our building machines).
Remove --without-common_test flag
It prevents from compiling `rabbit_common`
@bitwalker

This comment has been minimized.

Owner

bitwalker commented Feb 2, 2017

I'm willing to merge this, but just be aware that if you see this, it's because the library author failed to use -ifdef(TEST). to wrap their test code, leaving the call to -include_lib(...). for common test in their production code. It's better to make sure that gets done than to include common test - but since the impact is relatively small, I don't see any harm.

@bitwalker bitwalker merged commit f1f430e into bitwalker:master Feb 2, 2017

@bitwalker

This comment has been minimized.

Owner

bitwalker commented Feb 2, 2017

I'll get a new version of the image pushed as well

@jhchabran

This comment has been minimized.

Contributor

jhchabran commented Feb 2, 2017

Thanks for the explanation! I'll have a look at the missing -ifdef(TEST).

Btw, I've noticed too late that I forgot to bump the REFRESHED_AT in the Dockerfile in this PR :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment