Skip to content

Conversation

@fanquake
Copy link
Member

@fanquake fanquake commented Jan 4, 2017

See #8867 for discussion around moving to libevent 2.1.x .

We can drop both of our patches with the switch to the 2.1.x branch.

The libevent-2-fixes patch (#8730), was already included in the 2.1.6beta release, so can be removed.

The reuseaddr patch was introduced here. Looking through the 2.17 code, it looks like it might have been a backport from the 2.1.x branch, and so can also be dropped.

libevent 2.1.7-rc changelog
Closes #8867

@fanquake fanquake added this to the 0.14.0 milestone Jan 4, 2017
@fanquake fanquake requested a review from theuni January 4, 2017 14:08
$(package)_version=2.1.7
$(package)_download_path=https://github.com/libevent/libevent/archive/
$(package)_file_name=release-$($(package)_version)-rc.tar.gz
$(package)_sha256_hash=548362d202e22fe24d4c3fad38287b4f6d683e6c21503341373b89785fa6f991
Copy link
Member

Choose a reason for hiding this comment

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

Minor note: This hash is not guaranteed to be constant, I think.

Copy link
Member

Choose a reason for hiding this comment

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

Hm but not checking the hash at all would not be acceptable.

Copy link
Member

Choose a reason for hiding this comment

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

Of course, but we need to keep a backup of the GitHub generated zip file. (Which reminds me we already do so on bitcoincore.org/)

Copy link
Contributor

Choose a reason for hiding this comment

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

I may be off track, but if the hash changes for the given release, doesn't that mean we WANT the build system to explode in everyone's faces so people react? I can only imagine a hash change for the same package version would be due to some serious issue being hot-fixed upstream. Keeping and using a backup would mean that hot-fix goes unnoticed.

Copy link
Member

Choose a reason for hiding this comment

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

I can only imagine a hash change for the same package version would be due to some serious issue being hot-fixed upstream

This would require a new release of libevent. What I was referring to, is that the hash changes after GitHub purges its caches for zipped downloads of git tags/commits. (Note the above zip is not provided/signed by libevent but generated by GitHub for convenience.) I could image that GitHub does not deterministically generate zips.

Copy link
Member

Choose a reason for hiding this comment

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

@MarcoFalke I remember dealing with this at one point already, and I seem to remember github fixing the downloads to be deterministic, but I can't find any reference to that now.

It looks like our native_cctools comes from a similar link, so I assume that we would've noticed new hashes by now.

@kallewoof You're very much on-track. The issue (if I recall) is that github always serves the same .tar, but the gzipped result can change if it's been regenerated.

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be possible to have a checksum check for the decompressed .tar then?

Copy link
Member

Choose a reason for hiding this comment

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

@sipa Sure, if that's necessary. Given that we have similar downloads atm that aren't causing issues though, I suspect this is no longer an issue.

Copy link
Member

Choose a reason for hiding this comment

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

Let's just take the chance then and merge this.

@laanwj
Copy link
Member

laanwj commented Jan 10, 2017

Is there any reason this still has a WIP tag?

@fanquake fanquake changed the title [WIP][depends] libevent 2.1.7rc [depends] libevent 2.1.7rc Jan 10, 2017
@fanquake
Copy link
Member Author

@laanwj Really only due to the URL uncertainty. However we can update to a more stable URL in future if needed.

@maflcko
Copy link
Member

maflcko commented Jan 11, 2017

Agree with @fanquake

@laanwj laanwj merged commit 8217bd1 into bitcoin:master Jan 11, 2017
laanwj added a commit that referenced this pull request Jan 11, 2017
8217bd1 [depends] libevent 2.1.7rc (fanquake)
@laanwj
Copy link
Member

laanwj commented Jan 11, 2017

utACK 8217bd1. I'm not worried about the URL.

@fanquake fanquake deleted the depends-0-14-0-libevent branch January 11, 2017 13:07
@maflcko maflcko mentioned this pull request Jan 11, 2017
@jtimon
Copy link
Contributor

jtimon commented Jan 11, 2017

post-merge Concept ACK

$(package)_patches=reuseaddr.patch libevent-2-fixes.patch
$(package)_version=2.1.7
$(package)_download_path=https://github.com/libevent/libevent/archive/
$(package)_file_name=release-$($(package)_version)-rc.tar.gz
Copy link
Member

Choose a reason for hiding this comment

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

Looks like they just released the stable version, so we could use that, maybe.

Copy link
Member

Choose a reason for hiding this comment

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

That's annoying, we delay this update for months in the expectation that there will be a new stable version any time, and after we merge this there suddenly it is, the new stable version.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Move to Libevent 2.1.x

7 participants