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

mtr: Fix compile with libcap #6560

Merged
merged 1 commit into from
Aug 5, 2018
Merged

mtr: Fix compile with libcap #6560

merged 1 commit into from
Aug 5, 2018

Conversation

neheb
Copy link
Contributor

@neheb neheb commented Jul 23, 2018

Selecting libcap in addition to mtr causes it to error with

Package mtr is missing dependencies for the following libraries:
libcap.so.2

Signed-off-by: Rosen Penev [email protected]

Maintainer: @jmccrohan
Compile tested: ar71xx

@notnyt
Copy link
Contributor

notnyt commented Jul 27, 2018

This fixes the compile, but you also need to adjust the makefile to install mtr-packet. Also, the version comes up as unknown for some reason

root@rooter:~# mtr -v
mtr UNKNOWN

@neheb
Copy link
Contributor Author

neheb commented Jul 27, 2018

I don't maintain mtr. But I will fix those issues in a few hours.

@neheb neheb force-pushed the mtr2 branch 2 times, most recently from b83ab1d to b7278c8 Compare July 28, 2018 01:33
@neheb
Copy link
Contributor Author

neheb commented Jul 28, 2018

Fixed the first issue. I don't think I fixed the second as it depends on the actual git repository being present.

I tried switching to the official tarball but that just gave an error saying wrong version of automake.

Selecting libcap in addition to mtr causes it to error with

Package mtr is missing dependencies for the following libraries:
libcap.so.2

Signed-off-by: Rosen Penev <[email protected]>
@flipreverse
Copy link

Can you please merge this pull request?
I'm currently stuck at building my own OpenWRT image, because it contains mtr.

@neheb
Copy link
Contributor Author

neheb commented Aug 4, 2018

Ask @hnyman

@hnyman hnyman merged commit 69b24ff into openwrt:master Aug 5, 2018
@neheb neheb deleted the mtr2 branch August 5, 2018 19:19
@flipreverse
Copy link

@hnyman Thx! Works. :)

@stintel
Copy link
Member

stintel commented Aug 9, 2018

This PR should not have been merged for various reasons:
The commit contains several unrelated changes - they should each have gone in a separate commit.
Disabling libcap support could have been done with CONFIGURE_VARS += ac_cv_lib_cap_cap_set_proc=no instead - no hackish patch needed.
No maintainer approval.
And since the configure script strongly recommends libcap support for security, I would have actually preferred to add libcap as a dependency instead of disabling libcap support.

Please do proper reviews, especially if you merge PRs without maintainer approval!

@hnyman
Copy link
Contributor

hnyman commented Aug 9, 2018

We have a growing problem of disappearing maintainers.

In general, if there are PRs from regular submitters, there is maintainer timeout, the commit works in Travis, I have not been too picky on the coding style lately. Too many missing maintainers, even from among the Openwrt main repo committers...

It is really disappointing to PR authors that there is no action regarding their PRs.

Feel free to take part of reviewing and merging PRs. I can gladly stop participating in doing that and will let you handle the pile ;-)

@neheb
Copy link
Contributor Author

neheb commented Aug 9, 2018

Second.

Related: #6584

Patch vs. CONFIGURE_VARS was due to my unfamiliarity with autotools. The proper solution is to make it configurable, which I gave up on after failing.

I am not an mtr user and as such, I do not see the value in adding a new dependency.

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.

5 participants