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

Check presence of each beam listed in .app #7

Closed
wants to merge 1 commit into from

Conversation

avnik
Copy link
Contributor

@avnik avnik commented May 29, 2013

This allow to build riak, because one of riak deps (node_package) have stuff in priv/ and valid node_package.app, but no one .beam files.

This patch not make relx ready to build riak releases (not yet), but I believe that checking beams by .app file better than finding any file in ebin

@ericbmerritt
Copy link
Member

@avnik errors in Relx need to be pretty printable. take a look at include/relx.hrl. You need to wrap your errors in a ?RLX_ERROR macro and then add a format_error clause at the top.

You should do this every place you return an {error, ...}. there are a ton of examples of this in relx.

Btw, sorry this has taken so long to get to. I actually didn't notice the request. I really wish github had a better solution to notifications.

In any case, aside from the minor things I pointed out everything looks really good.

@ericbmerritt
Copy link
Member

@avnik any updates?

@avnik
Copy link
Contributor Author

avnik commented Jun 24, 2013

On Mon, Jun 24, 2013 at 03:14:41PM -0700, Eric Merritt wrote:

@avnik any updates?

Not yet.
Hope to apply recommended fixes on this workdays.

@ericbmerritt
Copy link
Member

@avnik thanks. I am just trying to stay on top of things. I am looking forward to this going in.

@ericbmerritt
Copy link
Member

@avnik another ping

This allow to build riak, because one of riak deps (node_package) have stuff
in priv/ and valid node_package.app, but no one .beam files.
@avnik
Copy link
Contributor Author

avnik commented Jul 19, 2013

On Tue, Jun 18, 2013 at 09:37:27AM -0700, Eric Merritt wrote:

@avnik errors in Relx need to be pretty printable. take a look at
include/relx.hrl. You need to wrap your errors in a ?RLX_ERROR macro
and then add a format_error clause at the top.

I'm not introduce new errors, except {error, {missing_beam_file, Module,
File}} corrected by your suggestion. Should I replace all {error, ...}
with ?RLX_ERROR, even if I not touch them? (may be in separate commit?)

You should do this every place you return an {error, ...}. there are a ton of examples of this in relx.

Btw, sorry this has taken so long to get to. I actually didn't notice the request. I really wish github had a better solution to notifications.

I'm not so fast too.

PS Also I'm rebase my branch to recent master.

@ericbmerritt
Copy link
Member

@avnik only fix the error you made. Not all of them :) sorry for the slow response. Let me know when you have rebased and I will merge.

@tsloughter
Copy link
Member

What is up with this PR now @ericbmerritt ?

@ericbmerritt
Copy link
Member

@tsloughter I was waiting for @avnik to fix the pr. but it looks like he isn't going to do it. If you want to grab his code and respond to that one thing I will merge it.

@jwilberding
Copy link
Member

Is this still relevant or did you merge in his code @tsloughter?

@tsloughter
Copy link
Member

Haven't touched it.

@tsloughter
Copy link
Member

Moved to #42

@tsloughter tsloughter closed this Sep 17, 2013
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.

None yet

4 participants