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

depends: Remove ccache #12607

Merged
merged 1 commit into from Mar 8, 2018

Conversation

Projects
None yet
8 participants
@fanquake
Copy link
Member

fanquake commented Mar 5, 2018

After discussion with @theuni, we can possibly just remove ccache from depends entirely.

Related to #12606

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Mar 5, 2018

Concept ACK, let's see what Travis says

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Mar 6, 2018

utACK ec7602b39eea0a7f8bc59eca3ac530e96e1e90e6

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Mar 6, 2018

Looks like configure picked up the ccache: checking if ccache should be used... yes

utACK ec7602b

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Mar 6, 2018

@fanquake fanquake changed the title [WIP] depends: Remove ccache depends: Remove ccache Mar 6, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Mar 6, 2018

@jonasschnelli 's gitian build seems fine

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Mar 6, 2018

What we still need to check is whether travis caches the ccache result, even though ~/.ccache is no longer in the list. I'm not sure how, is it possible to inspect the travis cache?

Edit: I tried through the GUI, as well as the travis command line tool, but with neither I was able to locate the cache for this PR. This might either mean it doesn't exist, or I couldn't get at it for another reason (e.g. some upper limit to the number of shown caches).

Seems the former. At the end of the travis logs it says:

store build cache
nothing changed, not updating cache

It's not writing the cache at all because it detects no changes?!

@laanwj laanwj requested a review from theuni Mar 6, 2018

.travis.yml Outdated
directories:
- depends/built
- depends/sdk-sources
- $HOME/.ccache

This comment has been minimized.

@laanwj

laanwj Mar 6, 2018

Member

My guess is that this needs to be re-added

@fanquake fanquake force-pushed the fanquake:ccache-removal branch to cc87967 Mar 6, 2018

@randolf

This comment has been minimized.

Copy link
Contributor

randolf commented Mar 6, 2018

Just in case it's helpful, I successfully complied this on NetBSD 7.0 (64-bit). These are the commands I used:

git clone https://www.github.com/fanquake/bitcoin/
cd bitcoin
git checkout ccache-removal
./autogen.sh
./configure CPPFLAGS="-I/usr/pkg/include" LDFLAGS="-L/usr/pkg/lib" BOOST_CPPFLAGS="-I/usr/pkg/include" BOOST_LDFLAGS="-L/usr/pkg/lib"
gmake

If you'd like me to try building with different parameters, or check for the presence of certain files/directories, please let me know.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Mar 6, 2018

@randolf Thanks for testing, however this does not affect normal autotools builds, if you don't use the depends system. And I don't think the depends system works on NetBSD (though never tried).

@randolf

This comment has been minimized.

Copy link
Contributor

randolf commented Mar 6, 2018

@laanwj I don't know enough about the "depends system" to provide helpful perspective there, but I'd be happy to try if there are some instructions somewhere that I can follow.

@theuni

theuni approved these changes Mar 6, 2018

Copy link
Member

theuni left a comment

ACK cc87967

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Mar 6, 2018

re-utACK cc87967

Caching works on arm and osx cross-builds at least

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Mar 6, 2018

utACK cc87967 - will merge this tomorrow after the meeting to minimize the amount of interference with ongoing travis testing

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Mar 8, 2018

@laanwj I assume the meeting is over?

@MarcoFalke MarcoFalke merged commit cc87967 into bitcoin:master Mar 8, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request Mar 8, 2018

Merge #12607: depends: Remove ccache
cc87967 depends: Remove ccache (fanquake)

Pull request description:

  After discussion with @theuni, we can possibly just remove ccache from depends entirely.

  Related to #12606

Tree-SHA512: ae0a60c8d97467fa41d617daa48ed22159cf32613808634a983304901dd5ed27124e77868d2314004e5144f7b35ba1333f720bb12daec4c5ca03aaf29d593ef2
@ryanofsky

This comment has been minimized.

Copy link
Contributor

ryanofsky commented Mar 8, 2018

The travis builds on all my PR's seem to be failing with errors like:

$ if [ -z "$NO_DEPENDS" ]; then depends/$HOST/native/bin/ccache --max-size=$CCACHE_SIZE; fi
/home/travis/.travis/job_stages: line 57: depends/x86_64-unknown-linux-gnu/native/bin/ccache: No such file or directory

An example is https://travis-ci.org/bitcoin/bitcoin/jobs/350509177#L1055

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Mar 8, 2018

@ryanofsky This is a travis bug. The command you mention above was removed from the travis yaml, it shouldn't be executed at all.

You can either ignore the failure or rebase your pull request on master. (Just changing the commit id via EDITOR=true git commit --ammend might be enough, but I haven't checked)

Edit: Just checked that rebase is not needed. You can just change the commit id

willyko added a commit to syscoin/syscoin that referenced this pull request Apr 20, 2018

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