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

Removed exometer as a dependency #20

Merged
merged 1 commit into from
Jan 8, 2016

Conversation

scohen
Copy link
Collaborator

@scohen scohen commented Jan 7, 2016

The community doesn't like having exometer as a dependency since most of
the popular reporters are inside of exometer_core anyway. I removed it
and cleaned up some of the other dependencies as well.

Addresses #19

@jparise

{:edown, github: "uwiger/edown", tag: "0.7", override: true},
{:lager, github: "basho/lager", tag: "2.1.0", override: true},
{:exometer, github: "pspdfkit-labs/exometer"},
{:netlink, github: "Feuerlabs/netlink", ref: "d6e7188e", override: true},
{:exometer_core, git: "git://github.com/PSPDFKit-labs/exometer_core.git", branch: "master"},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use {:exometer_core, github: "pspdfkit-labs/exometer_core"} here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did, but this caused a conflict when I used this in our internal project; The conflict stems from exometer's rebar file using git://, while github in mix.exs causes it to use http as the protocol.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems like a bug of some sort that should be addressed. But I'm glad there's a workaround.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll ask in the elixir chat room if this is expected; I agree, they're the same repo (though the branch differed --exometer explicity required master... odd.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there are several deps along the dependency chain (parse_trans, setup, etc.) that is calling edown all for a edoc generation. so far does not look like their code is calling anything in edown.

Also, there is a remove_deps.script in exometer/priv that looks like to prune the deps. But can't get that to work by rebar -C ...

For a otp release,you can exclude those out in reltool.config. Unsure how you can do it in mix (though there is a mix rebar plugin).

Also look at the network graph of the git repo. Lots just forked their own and remove the deps.
Lots of Erlang shops forked their own internally.

At this point, it does not seem to worth it. May be this is "an exercise left to the reader" to their post processing when they do otp release...unless you are willing to own the upstream dependencies.

@jparise
Copy link
Collaborator

jparise commented Jan 7, 2016

👍

The community doesn't like having exometer as a dependency since most of
the popular reporters are inside of exometer_core anyway. I removed it
and cleaned up some of the other dependencies as well.

Addresses pinterest#19
scohen added a commit that referenced this pull request Jan 8, 2016
Removed exometer as a dependency
@scohen scohen merged commit d0ac829 into pinterest:master Jan 8, 2016
obmarg added a commit to obmarg/elixometer that referenced this pull request Feb 6, 2016
Using the current example in an application causes the following error:

> You have configured application :exometer in your configuration
> file but the application is not available.

I think this is because pinterest#20 updated elixometer to depend on
exometer_core instead of exometer, but did not update the README to
configure exometer_core.
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

3 participants