Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add missing application tuple value for elli dependency #3

Merged
merged 2 commits into from
Aug 24, 2018
Merged

Add missing application tuple value for elli dependency #3

merged 2 commits into from
Aug 24, 2018

Conversation

oldgalileo
Copy link
Contributor

It is possible that rebar incorrectly assumes the order of the dependencies necessary to compile. This will manifest itself in an error that resembels:

===> Compiling _build/default/lib/elli_cache/src/elli_middleware_cache.erl failed
_build/default/lib/elli_cache/src/elli_middleware_cache.erl:19: can't find include lib "elli/include/elli.hrl"
...

This is recognized in this post, where Fred Hobert explains that if a dependency isn't listed as a dependency in the application tuple, rebar3 does not guarantee the build order.

This change fixes this issue for rebar3.

@oldgalileo
Copy link
Contributor Author

Also, Travis has been build-failing since Travis#20. It's not an issue with this PR specifically.

@fvasquez
Copy link

I know elli_cache is still pre-release but it sure would be nice if rebar3 could build it into our OTP applications.

@yurrriq
Copy link
Member

yurrriq commented Aug 23, 2018

Elli was deliberately not included in deps so users could/would have to provide their own version. Otherwise we might have mismatches and such. I wasn't aware of the rebar3 issue though. Thoughts, @elli-lib/team?

@tsloughter
Copy link
Member

I think Fred is wrong about it needing to be in the application tuple to get the build order right. It has to be in either the app tuple or the deps list in order to get the build order right.

It may work with putting it only in the application's list of the .app.src. But not really any reason to not include it in the dpes list too I guess.

Rebar3 does dep resolving like maven, the first version it encounters during a sorted level walk of the dep tree is used, there can be no conflicts by the build system. So having it in the deps for the cache doesn't matter since the users version will override it.

@yurrriq
Copy link
Member

yurrriq commented Aug 23, 2018

👍 to adding elli to applications in elli_cache.app.src but not deps then, I think. Hopefully I get free time soon and can finish this..

@oldgalileo
Copy link
Contributor Author

@yurrriq I've made the change and confirmed that keeping elli solely within the applications of the elli_cache.app.src works and resolves the issue.

@yurrriq
Copy link
Member

yurrriq commented Aug 24, 2018

LGTM, thanks!

@yurrriq
Copy link
Member

yurrriq commented Aug 24, 2018

PS I'd be happy to help review other PRs, as my time to actively contribute to this is nearly nonexitent for now.

@yurrriq yurrriq merged commit 3e6af62 into elli-lib:develop Aug 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants