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

[RDY] third-party: update jemalloc to 4.0.2 #3289

Merged
merged 1 commit into from Sep 22, 2015

Conversation

Projects
None yet
6 participants
@fmoralesc
Contributor

fmoralesc commented Sep 4, 2015

jemalloc 4.0 adds support for Bitrig, Cygwin, DragonFlyBSD, iOS, OpenBSD and OpenRISC/or1k.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Sep 4, 2015

From the release notes (which should be linked to in the commit message IMO):

Replace --enable-cc-silence with --disable-cc-silence to suppress spurious warnings by default.

so --enable-cc-silence should probably be removed from third-party/cmake/BuildJeMalloc.cmake.

Besides that, isn't this needed as well?

diff --git a/.travis.yml b/.travis.yml
index becc7e8..a1d2db5 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -6,7 +6,7 @@ env:
     # To build third-party dependencies, set this to 'true'.
     # TODO: Change deps caching to detect updated dependencies automatically, but
     #       still don't rebuild deps every time.
-    - BUILD_NVIM_DEPS=false
+    - BUILD_NVIM_DEPS=true
     # Travis has 1.5 virtual cores according to
     # http://docs.travis-ci.com/user/speeding-up-the-build/#Paralellizing-your-build-on-one-VM
     - MAKE_CMD="make -j2"

ghost commented Sep 4, 2015

From the release notes (which should be linked to in the commit message IMO):

Replace --enable-cc-silence with --disable-cc-silence to suppress spurious warnings by default.

so --enable-cc-silence should probably be removed from third-party/cmake/BuildJeMalloc.cmake.

Besides that, isn't this needed as well?

diff --git a/.travis.yml b/.travis.yml
index becc7e8..a1d2db5 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -6,7 +6,7 @@ env:
     # To build third-party dependencies, set this to 'true'.
     # TODO: Change deps caching to detect updated dependencies automatically, but
     #       still don't rebuild deps every time.
-    - BUILD_NVIM_DEPS=false
+    - BUILD_NVIM_DEPS=true
     # Travis has 1.5 virtual cores according to
     # http://docs.travis-ci.com/user/speeding-up-the-build/#Paralellizing-your-build-on-one-VM
     - MAKE_CMD="make -j2"
@ZyX-I

This comment has been minimized.

Show comment
Hide comment
@ZyX-I

ZyX-I Sep 4, 2015

Contributor

Besides that, isn't this needed as well?

Dependencies are build by https://github.com/neovim/bot-ci.

Contributor

ZyX-I commented Sep 4, 2015

Besides that, isn't this needed as well?

Dependencies are build by https://github.com/neovim/bot-ci.

@fmoralesc

This comment has been minimized.

Show comment
Hide comment
@fmoralesc

fmoralesc Sep 4, 2015

Contributor

@ZyX-I So I should revert that change and do what, exactly? Are the dependencies kept in sync automatically? I'm uncertain as to how travis works.

Contributor

fmoralesc commented Sep 4, 2015

@ZyX-I So I should revert that change and do what, exactly? Are the dependencies kept in sync automatically? I'm uncertain as to how travis works.

@ZyX-I

This comment has been minimized.

Show comment
Hide comment
@ZyX-I

ZyX-I Sep 4, 2015

Contributor

Dependencies will be rebuilt when bot-ci is run. I am not sure what exactly triggers rerunning bot-ci (except for obvious pushing to master of bot-ci repository), but jemalloc will eventually get updated: I know that bot-ci is rerun under some circumstances.

Contributor

ZyX-I commented Sep 4, 2015

Dependencies will be rebuilt when bot-ci is run. I am not sure what exactly triggers rerunning bot-ci (except for obvious pushing to master of bot-ci repository), but jemalloc will eventually get updated: I know that bot-ci is rerun under some circumstances.

@fwalch

This comment has been minimized.

Show comment
Hide comment
@fwalch

fwalch Sep 4, 2015

Member

With the new containerized build, dependencies (except OSX, which is still done by bot-ci once per day) are built in neovim/neovim and cached by Travis. As collaborator, you can delete the cache on the Travis website manually, or you can do what @Pyrohh suggested, that should rebuild deps and update the cache.
The output in the build log under before_script has the dependency stuff.

Member

fwalch commented Sep 4, 2015

With the new containerized build, dependencies (except OSX, which is still done by bot-ci once per day) are built in neovim/neovim and cached by Travis. As collaborator, you can delete the cache on the Travis website manually, or you can do what @Pyrohh suggested, that should rebuild deps and update the cache.
The output in the build log under before_script has the dependency stuff.

@ZyX-I

This comment has been minimized.

Show comment
Hide comment
@ZyX-I

ZyX-I Sep 4, 2015

Contributor

@fwalch Will not this change make dependencies be built always?

Contributor

ZyX-I commented Sep 4, 2015

@fwalch Will not this change make dependencies be built always?

@ZyX-I

This comment has been minimized.

Show comment
Hide comment
@ZyX-I

ZyX-I Sep 4, 2015

Contributor

And I think that this information needs to be put into CONTRIBUTING.md, section “For dependencies pull requests” (same level as “For code pull requests”).

Contributor

ZyX-I commented Sep 4, 2015

And I think that this information needs to be put into CONTRIBUTING.md, section “For dependencies pull requests” (same level as “For code pull requests”).

@fmoralesc

This comment has been minimized.

Show comment
Hide comment
@fmoralesc

fmoralesc Sep 4, 2015

Contributor

Thanks, @fwalch and @ZyX-I I reverted that change and deleted the cache for this PR.

Contributor

fmoralesc commented Sep 4, 2015

Thanks, @fwalch and @ZyX-I I reverted that change and deleted the cache for this PR.

@fwalch

This comment has been minimized.

Show comment
Hide comment
@fwalch

fwalch Sep 4, 2015

Member

@ZyX-I There is a check in the CI files somewhere that only builds them conditionally. For example, in this build on master you can see "Using third-party dependencies from Travis's cache".

Without the "Extracted dependency sources detected, skip downloading" check in the third-party CMake that we added for homebrew, it would not even be necessary to do a check, we could just always build the third-party deps (which would just use the stuff that has already been compiled "last time", and if CMakeLists.txt was updated, download and build the new stuff). If I have some spare time, I'll investigate how to get rid of this.. would make things a lot easier, I think. And we wouldn't need to add a note.

Member

fwalch commented Sep 4, 2015

@ZyX-I There is a check in the CI files somewhere that only builds them conditionally. For example, in this build on master you can see "Using third-party dependencies from Travis's cache".

Without the "Extracted dependency sources detected, skip downloading" check in the third-party CMake that we added for homebrew, it would not even be necessary to do a check, we could just always build the third-party deps (which would just use the stuff that has already been compiled "last time", and if CMakeLists.txt was updated, download and build the new stuff). If I have some spare time, I'll investigate how to get rid of this.. would make things a lot easier, I think. And we wouldn't need to add a note.

@ZyX-I

This comment has been minimized.

Show comment
Hide comment
@ZyX-I

ZyX-I Sep 4, 2015

Contributor

@ZyX-I There is a check in the CI files somewhere that only builds them conditionally. For example, in this build on master you can see "Using third-party dependencies from Travis's cache".

Which can only be shown if BUILD_NVIM_DEPS environment variable is not set to true:

if [[ -f "${CACHE_MARKER}" ]] && [[ "${BUILD_NVIM_DEPS}" != true ]]; then
.

I would suggest to simply compare timestamp of ${CACHE_MARKER} with timestamp from git log -n1 --format=%ct third-party. If this appears to output nothing (due to --depth=50 used when cloning) assume dependencies are up-to-date.

Contributor

ZyX-I commented Sep 4, 2015

@ZyX-I There is a check in the CI files somewhere that only builds them conditionally. For example, in this build on master you can see "Using third-party dependencies from Travis's cache".

Which can only be shown if BUILD_NVIM_DEPS environment variable is not set to true:

if [[ -f "${CACHE_MARKER}" ]] && [[ "${BUILD_NVIM_DEPS}" != true ]]; then
.

I would suggest to simply compare timestamp of ${CACHE_MARKER} with timestamp from git log -n1 --format=%ct third-party. If this appears to output nothing (due to --depth=50 used when cloning) assume dependencies are up-to-date.

@fmoralesc

This comment has been minimized.

Show comment
Hide comment
@fmoralesc

fmoralesc Sep 4, 2015

Contributor

I don't get it. It seems like the cache is still being used, although I removed the cache before restarting the build. It seems to be using a cache from 2015-07-08. Should I delete all caches?

Contributor

fmoralesc commented Sep 4, 2015

I don't get it. It seems like the cache is still being used, although I removed the cache before restarting the build. It seems to be using a cache from 2015-07-08. Should I delete all caches?

@fwalch

This comment has been minimized.

Show comment
Hide comment
@fwalch

fwalch Sep 4, 2015

Member

@fmoralesc Hmm, it might fall back to a cache from master. Setting BUILD_NVIM_DEPS to true should work for the PR, but we might have to delete the caches on master manually after this has been merged 😬

Member

fwalch commented Sep 4, 2015

@fmoralesc Hmm, it might fall back to a cache from master. Setting BUILD_NVIM_DEPS to true should work for the PR, but we might have to delete the caches on master manually after this has been merged 😬

@fmoralesc

This comment has been minimized.

Show comment
Hide comment
@fmoralesc

fmoralesc Sep 4, 2015

Contributor

Eh... how did this work for #3274? Will it be an issue for #3281?

Contributor

fmoralesc commented Sep 4, 2015

Eh... how did this work for #3274? Will it be an issue for #3281?

@fwalch

This comment has been minimized.

Show comment
Hide comment
@fwalch

fwalch Sep 4, 2015

Member

Eh... how did this work for #3274?

I guess it didn't.. I think it's possible to download the cache tarball from Travis to take a look (Edit: no, doesn't look like that's possible).

Member

fwalch commented Sep 4, 2015

Eh... how did this work for #3274?

I guess it didn't.. I think it's possible to download the cache tarball from Travis to take a look (Edit: no, doesn't look like that's possible).

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Sep 10, 2015

Member

@fmoralesc If I understand correctly, now that #3307 is merged, this should work correctly if you re-push without the fixup commits.

Member

justinmk commented Sep 10, 2015

@fmoralesc If I understand correctly, now that #3307 is merged, this should work correctly if you re-push without the fixup commits.

@fwalch

This comment has been minimized.

Show comment
Hide comment
@fwalch

fwalch Sep 10, 2015

Member

If I understand correctly, now that #3307 is merged, this should work correctly if you re-push without the fixup commits.

@fmoralesc Don't forget to rebase on current master first.

Member

fwalch commented Sep 10, 2015

If I understand correctly, now that #3307 is merged, this should work correctly if you re-push without the fixup commits.

@fmoralesc Don't forget to rebase on current master first.

@fmoralesc fmoralesc changed the title from third-party: update jemalloc to 4.0 to [RDY] third-party: update jemalloc to 4.0 Sep 20, 2015

@marvim marvim added the RDY label Sep 20, 2015

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Sep 22, 2015

Heads up: 4.0.2 was released (lots of bugfixes).

ghost commented Sep 22, 2015

Heads up: 4.0.2 was released (lots of bugfixes).

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Sep 22, 2015

Member

@fmoralesc If you have a chance to update this, ping me or just merge it yourself after travis passes.

Member

justinmk commented Sep 22, 2015

@fmoralesc If you have a chance to update this, ping me or just merge it yourself after travis passes.

third-party: update jemalloc to 4.0.2
jemalloc 4.0 adds support for OpenBSD, DragonFlyBSD and other platforms.

Release notes: https://github.com/jemalloc/jemalloc/releases/tag/4.0.0

4.0.1 and 4.0.2 are bugfix releases

Release notes: https://github.com/jemalloc/jemalloc/releases/tag/4.0.1
               https://github.com/jemalloc/jemalloc/releases/tag/4.0.2

@fmoralesc fmoralesc changed the title from [RDY] third-party: update jemalloc to 4.0 to [RDY] third-party: update jemalloc to 4.0.2 Sep 22, 2015

@fmoralesc

This comment has been minimized.

Show comment
Hide comment
@fmoralesc

fmoralesc Sep 22, 2015

Contributor

Thanks, for the heads up, @Pyrohh. Everything looks fine, so I'm merging this, @justinmk.

Contributor

fmoralesc commented Sep 22, 2015

Thanks, for the heads up, @Pyrohh. Everything looks fine, so I'm merging this, @justinmk.

fmoralesc added a commit that referenced this pull request Sep 22, 2015

Merge pull request #3289 from fmoralesc/update-jemalloc
third-party: update jemalloc to 4.0.2

@fmoralesc fmoralesc merged commit 1ba081a into neovim:master Sep 22, 2015

3 checks passed

continuous-integration/appveyor AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.03%) to 67.055%
Details

@jszakmeister jszakmeister removed the RDY label Sep 22, 2015

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