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

Already on GitHub? Sign in to your account

Add other deps to library path when running xref #300

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

dLuna commented Aug 28, 2012

For those of us who do their sub_dirs as

{sub_dirs, ["apps/app1",
                "apps/app2"...]}.

the other apps are not available in xrefs library path so calls between applications will always be seen as calls to undefined functions.

This fixes that problem.

(The proper and correct solution to how to run xref is of course to use a filtered code path which only contains applications which are (recursively) dependencies of the app that is currently checked. This is a bit bigger, and not part of the current pull request.)

This pull request passes (merged 1c05910c into ff8337f).

This pull request passes (merged 5c14f80a into ff8337f).

@ghost ghost assigned tuncer Aug 29, 2012

Contributor

dLuna commented Sep 4, 2012

Looking into whether this is the correct solution or not. Potentially $SUBDIR/ebin should be in the global code path instead.

Contributor

dLuna commented Oct 12, 2012

After looking at this again I think that this solution is not optimal, but good enough for now.

The proper way to do this is to merge code path handling with the way it's handled in compilation. I believe that all the directories should have already been part of the global code path. It has to be for compilation to work properly, so this already happens somewhere else in rebar.

The current patch is still a huge improvement from doing nothing, so I'm inclined to say merge for now and I'll do the proper fix at a later date.

Contributor

dLuna commented Oct 12, 2012

Btw. I rebased from current master so it should merge cleanly.

Contributor

dizzyd commented Nov 1, 2012

I'm fine with this fix, but can you please add a comment in function that pulls in sub_dirs which explains why this is a short term fix? That way when we read the code in X months, we have a why. :)

Once you do that, I'll get it merged.

Contributor

dizzyd commented Nov 5, 2012

Got tired of waiting -- added comment myself and merged. :)

@dizzyd dizzyd closed this Nov 5, 2012

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