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.

Sign up for GitHub

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

Jump to bottom

Remove bundled libevent #104

Closed
wants to merge 1 commit into from
Closed

Remove bundled libevent #104

wants to merge 1 commit into from

Conversation

rubenk
Copy link
Contributor

@rubenk rubenk commented Apr 26, 2018

It is ancient and doesn't compile on Fedora Rawhide.
Most modern distributions ship with libevent now.

@rubenk
Remove bundled libevent
57e96d1 
It is ancient and doesn't compile on Fedora Rawhide.
Most modern distributions ship with libevent now.
@mfwitten
Copy link
Contributor

mfwitten commented May 15, 2018 •
edited

I agree wholeheartedly; this dependency is better handled by the end-user.

I've taken the liberty of "backporting" this removal to branch-netatalk-3-0, and then merging the resulting commit with branch-netatalk-3-1, and then merging the resulting commit with master; I also removed contrib/misc/libevent.patch. These commits can be retrieved as follows:

git fetch https://github.com/mfwitten/Netatalk.git \
  'refs/heads/tmp/libevent-removed/*:refs/heads/tmp/libevent-removed/*'

Then, you can see that there is a clearly recorded cross-branch alteration:

$ log --graph --oneline -3 tmp/libevent-removed/master
*   d75ed223 (HEAD -> tmp/libevent-removed/master) Merge removal of bundled libevent
|\
| *   08dd8b88 (tmp/libevent-removed/branch-netatalk-3-1) Merge removal of bundled libevent
| |\
| | * be42f030 (tmp/libevent-removed/branch-netatalk-3-0) Remove bundled libevent

Your authorship has been preserved:

$ git log -1 tmp/libevent-removed/branch-netatalk-3-0
commit be42f030ee3fa4d070166aabeaf26bb4039aee8c
Author: Ruben Kerkhof <ruben@rubenkerkhof.com>
Date:   Fri Apr 27 00:31:46 2018 +0200

    Remove bundled libevent
    
    It is ancient and doesn't compile against modern
    dependencies; also, modern distributions tend to
    ship with libevent now.
    
    Co-authored-by: Michael Witten <mfwitten@gmail.com>

The easiest way to apply all this would be for @slowfranklin or @hat001 to fetch the commits as described above, and then push each one into its respective branch:

git push https://hat001@github.com/Netatalk/Netatalk.git \
  'refs/heads/tmp/libevent-removed/*:refs/heads/*'

If done now, before any other pushes, that should yield fast-forward "merges" for each one, and then this pull request may be closed.

@slowfranklin
Copy link
Member

slowfranklin commented May 15, 2018 via email

@rubenk
Copy link
Contributor Author

rubenk commented May 15, 2018

I can look into that, but which distro doesn't ship libevent these days?

@slowfranklin
Copy link
Member

slowfranklin commented May 15, 2018 via email

@rubenk
Copy link
Contributor Author

rubenk commented May 15, 2018

And using the one from OpenCSW isn't an option?

@slowfranklin
Copy link
Member

slowfranklin commented May 15, 2018 via email

@andy-js
Copy link
Contributor

andy-js commented May 15, 2018

I don't think having libevent as an external dependency is really that big of a deal. And if we want to ship binaries packages that include it we can still do that.

@slowfranklin
Copy link
Member

slowfranklin commented May 15, 2018 via email

@andy-js
Copy link
Contributor

andy-js commented May 15, 2018

If you are already building netatalk from source building libevent first doesn't seem like it's much of a stretch. What am I missing?

@mfwitten
Copy link
Contributor

mfwitten commented May 15, 2018 •
edited

@slowfranklin
Because you're building the binaries, you should have complete control over the build environment.

  • You could provide any version of libevent you want as a static dependency, or even bundle it has a dynamic dependency.
  • If you're building a binary for some system that does ship libevent, then I suggest obtaining a copy of the relevant files so that you can build against the system's actual software base.
  • It might be worthwhile to look into one of these "deterministic" build environment solutions, so that these issues don't seem so ad hoc.

@slowfranklin
Copy link
Member

slowfranklin commented May 15, 2018 via email

@rubenk
Copy link
Contributor Author

rubenk commented May 15, 2018

In the long run keeping the bundled libevent up to date (which it now isn't) seems like much more work than a day, including handling all the reports of issues that have been fixed in upstream libevent.

There's also a security risk, and the fact that the libs are installed in /usr/lib, potentially conflicting with libevent from distros.

@mfwitten
Copy link
Contributor

@slowfranklin @rubenk
Here's a compromise solution: How about there be created a new, separate repository whose sole purpose is to provide anyone with the tools necessary to automate binary builds of the main source repository?

Someone who has time (e.g., perhaps me or @rubenk or @andy-js) can get this binary-build infrastructure working, and then it will be a simple matter of removing libevent from the main repository and using the new binary-build infrastructure to deal with building.

Not only would this codify the details necessary to build certain binaries, but it would increase this project's bus factor.

@slowfranklin
Copy link
Member

slowfranklin commented May 15, 2018 via email

@rubenk
Copy link
Contributor Author

rubenk commented May 15, 2018

I just checked, there already is a configure flag:

netatalk (master %=) > ./configure --help | grep -i event
  --with-libevent         whether to use the bundled libevent (default: yes)
  --with-libevent-header  path to libevent header files
  --with-libevent-lib     path to libevent library

@mfwitten
Copy link
Contributor

Well, of course there is already a configure flag; the whole point is to get rid of that stuff.

This repo is not the proper place to be storing dependencies like this.

@slowfranklin
Copy link
Member

slowfranklin commented May 15, 2018 via email

@slowfranklin
Copy link
Member

The day has come! Let's finally remove libevent, cf MR #132. Closing this one. Thanks for contributing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@rubenk @mfwitten @slowfranklin @andy-js