depends: Add 'make clean' rule #12715

hkjn commented at 10:47 AM on March 18, 2018: contributor

It's useful to have a standard way to clean up the work done by the depends system when testing changes to it.

fanquake added the label Build system on Mar 18, 2018

fanquake requested review from theuni on Mar 18, 2018

randolf approved

randolf commented at 3:04 PM on March 18, 2018: contributor

More cleaning is good. I just tested this change on NetBSD 7.0 (64-bit), and it does result in the removal of a lot of files. Although it doesn't seem to clean up everything (see output from "du" below, which I modified to make it easier to read), I feel that this is heading in a good direction.

Before gmake:

netbsd# du -d 2 -k
  105,684 ./bitcoin/.git
        8 ./bitcoin/.github
        8 ./bitcoin/.tx
      956 ./bitcoin/build-aux
      796 ./bitcoin/contrib
      356 ./bitcoin/depends
    1,084 ./bitcoin/doc
      556 ./bitcoin/share
   31,668 ./bitcoin/src
    1,692 ./bitcoin/test
    3,892 ./bitcoin/autom4te.cache
  148,592 ./bitcoin
  148,596 .

After gmake:

netbsd# du -d 2 -k
  105,684 ./bitcoin/.git
        8 ./bitcoin/.github
        8 ./bitcoin/.tx
      956 ./bitcoin/build-aux
      796 ./bitcoin/contrib
      356 ./bitcoin/depends
    1,084 ./bitcoin/doc
      556 ./bitcoin/share
1,394,340 ./bitcoin/src
    1,692 ./bitcoin/test
    3,892 ./bitcoin/autom4te.cache
1,511,264 ./bitcoin
1,511,268 .

After gmake clean:

netbsd# du -d 2 -k
  105,684 ./bitcoin/.git
        8 ./bitcoin/.github
        8 ./bitcoin/.tx
      956 ./bitcoin/build-aux
      796 ./bitcoin/contrib
      356 ./bitcoin/depends
    1,084 ./bitcoin/doc
      556 ./bitcoin/share
   50,016 ./bitcoin/src
    1,692 ./bitcoin/test
    3,892 ./bitcoin/autom4te.cache
  166,940 ./bitcoin
  166,944 .

in depends/Makefile:169 in bf5be19319 outdated

 164 | @@ -165,6 +165,9 @@ $(host_prefix)/share/config.site: check-packages
 165 |  
 166 |  check-packages: check-sources
 167 |  
 168 | +clean:
 169 | +	@rm -rf work/ built/ sources/ x86_64* i686* mips* arm* aarch64*

laanwj commented at 8:35 AM on March 19, 2018:

Concept ACK, but having to special-case architectures shows a problem in naming.

hkjn commented at 10:16 AM on March 19, 2018:

Agreed that it's less than ideal to need to list all supported architectures here..

If we agree that changing where the architecture-specific stuff ends up is for another PR, what would the ideal directory structure look like here?

depends/arch/armv7l-unknown-linux-gnueabihf/...?

(This line could after such a change then rm -rf arch.)

laanwj commented at 3:32 PM on March 19, 2018:

Sound like a good idea to me, but would like @theuni's opinion on this.

laanwj assigned theuni on Mar 19, 2018

hkjn commented at 10:22 AM on March 19, 2018: contributor

@randolf: Maybe I'm missing something, but in your commands the depends/ directory is shown with the same size both before / after. Note that this only changes the make clean for the depends system. You can test the change by from the root of the repo doing cd depends && make && du -sh . > before.txt && make clean && du -sh > after.txt.

fanquake commented at 6:31 AM on March 20, 2018: member

Concept ACK

-0 on cleaning up /sources. Lots of the time I can end up on slow internet connections, and nuking all the packages I've downloaded would be less than ideal.

hkjn commented at 11:05 AM on March 20, 2018: contributor

@fanquake: I hear what you're saying, but removing sources/ to be able to test downloading (including the fallback behavior to fetch from bitcoincore.org, which had issues recently) was indeed one of the things I wanted from this.. if you want we could have a separate clean-all rule or somesuch, with the default make clean not touching sources/, if that would work for you?

laanwj commented at 2:55 PM on March 20, 2018: member

-0 on cleaning up /sources. Lots of the time I can end up on slow internet connections, and nuking all the packages I've downloaded would be less than ideal.

Same here. I tend to keep the sources directory, even share it (through symlinking) between multiple check-outs of bitcoin core. Generally you'd expect make clean to remove built binaries and intermediates, not sources.

If you want we could have a separate clean-all rule or somesuch, with the default make clean not touching sources/, if that would work for you?

Sounds good to me.

FWIW what I tend to use a lot in practice is: keep *arch*, keep sources, trash the intermediate files (work/ built/). But we probably don't want a zillion make targets for cleaning :)

hkjn commented at 7:17 PM on March 20, 2018: contributor

Please take another look, I added a second commit 80b8a2a with a separate make clean-all rule that deletes sources/ as well. I'll squash into one commit after review.

theuni commented at 7:34 PM on March 20, 2018: member

Yea, I've kicked myself a hundred times for not putting the resulting target dirs in a subdir.

The only sane way I can think to handle this is for 'make clean' to clean only the current host, so you'd need to use 'make clean HOST=foo' to remove the non-default ones.

I think yet-another make target should be used for cleaning sources, as that behavior would surprise me for any variant of 'make clean'.

Additionally, these need to be using the correct variables rather than hard-coded paths: work/build: $(base_build_dir) work/staging: $(base_staging_dir) sources: $(SOURCES_PATH)

hkjn commented at 12:19 PM on March 21, 2018: contributor

Thanks for the review and the suggestions.

I pushed two new commits:

d8bc3bb: depends: Add WORK_PATH var, use it and other variables in clean/clean-all
03744a1: depends: Only remove current arch for 'make clean'

Let me know if this is the direction you were going for, @theuni.

laanwj commented at 10:59 AM on April 7, 2018: member

Ping @theuni

theuni approved

theuni commented at 6:38 PM on April 9, 2018: member

@hkjn Thanks, looks good now. Mind squashing down, though? @laanwj This is another annoying one that makes sense for a Friday afternoon as it will cause all deps to be rebuilt.

depends: Add 'make clean' and 'make clean-all' rules

It's useful to have a standard way to clean up the work done by the
depends system when testing changes to it.

The `make clean-all` rule removes build artifacts for all
supported architectures (in addition to sources/), while `make clean`
only removes artifacts for current architecture (`BUILD`).

aff16fd511

hkjn force-pushed on Apr 11, 2018

hkjn commented at 12:55 PM on April 11, 2018: contributor

Thanks @theuni!

I've rebased and squashed.

fanquake commented at 7:58 AM on April 18, 2018: member

testedACK aff16fd

I've tested on macOS 10.13.4. Doing a make and make HOST=x86_64-w64-mingw32:

make clean removes the built, work and x86_64-apple-darwin17.5.0 (current host) directories.

make clean-all also removes sources and the x86_64-w64-mingw32 directory.

laanwj commented at 9:28 AM on April 18, 2018: member

@laanwj This is another annoying one that makes sense for a Friday afternoon as it will cause all deps to be rebuilt.

It seems fairly silent at the moment ,so going to merge now.

laanwj merged this on Apr 18, 2018

laanwj closed this on Apr 18, 2018

laanwj referenced this in commit 8fd62437c6 on Apr 18, 2018

deadalnix referenced this in commit 3df7a74d0e on Mar 31, 2020

PastaPastaPasta referenced this in commit 86fadb6c02 on Jun 17, 2020

PastaPastaPasta referenced this in commit c1b358aeb9 on Jun 27, 2020

ftrader referenced this in commit ad8258f589 on Aug 17, 2020

gades referenced this in commit 0985cc6293 on Jun 28, 2021

CryptoCentric referenced this in commit 725827378f on Jul 2, 2021

MarcoFalke locked this on Sep 8, 2021