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.

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

make keeps redoing some of the earlier done work. #22134

Closed
dgoncharov opened this issue Jun 2, 2021 · 6 comments
Closed

make keeps redoing some of the earlier done work. #22134

dgoncharov opened this issue Jun 2, 2021 · 6 comments

Comments

@dgoncharov
Copy link

dgoncharov commented Jun 2, 2021

This issue is about depends subsystem only.

Once everything is built, subsequent runs of make result in
make redoing some of the earlier done work.

There are 2 files which are being rebuilt.
config.site and .stamp_$(final_build_id).

config.site is rebuilt because

  1. check-packages and check-sources are both phony and create no files.
  2. A real file (config.site) depends on check-packages and check-sources.
    When a file depends on a phony prerequisite, the file is rebuilt every time. In
    this case, config.site is rebuilt every time.

.stamp_$(final_build_id) is rebuild because

  1. every package (expat, boost, etc) is phony.
    .PHONY: $(1)
    See funcs.mk:239.
  2. The rule for every package
    $(1): | $($(1)_cached_checksum)
    does not create any file.
    See funcs.mk:240.
  3. A real file (.stamp_$(final_build_id)) depends on packages.
    When a file depends on a phony prerequisite, the file is rebuilt every time. In
    this case, .stamp_$(final_build_id) is rebuilt every time.

What can be done about this?

There are a few options.

Regarding config.site.

  1. Given that subsequent runs of make do not download the tarball, then what is
    the purpose of computing the hash of the earlier downloaded tarball and
    comparing against the hash stored earlier in the stamp file?
    If some malicious person could substitute the local copy of the tarball, then
    that same person could store the new hash in the stamp file.

i see, that check-sources and check-packages were introduced in commit
235b3a7. The comment says this is for travis.
Given that travis is no longer used, we should figure out if this commit can
be reverted.

  1. Another option is to mark check-packages and check-sources as not phony and
    introduce sentinel files.
    E.g.
check-packages: $(prereqs_of_check_packages)
    $(foreach package,$(all_packages),$(call check_or_remove_cached,$(package));)
    touch $@

check-sources: $(prereqs_of_check_sources)
    $(foreach package,$(all_packages),$(call check_or_remove_sources,$(package));)
    touch $@

In addition, both of these rules will need to have prerequisites.
In addition, the same trouble with .stamp_$(final_build_id) has to be fixed,
because config.site depends on .stamp_$(final_build_id).

  1. Or keep the makefile as is. Apparently, this does not bother anybody.

As for .stamp_$(final_build_id), either the rule can be modified to create a
sentinel file and each package can be marked to be not phony or the keep the
makefile as is.

<!--- What behavior did you expect? -->
"Nothing to be done for 'all'." message from make.

<!--- How reliably can you reproduce the issue, what are the steps to do so? -->
Every time.

<!-- What version of Bitcoin Core are you using, where did you get it (website, self-compiled, etc)? -->
Latest soucrce from git.
@dgoncharov
Copy link
Author

Rebuild of .stamp_$(final_build_id) is the expensive one.
It copies all the cached tarballs and unpacks them sequentially, one by one.
Takes about 27 seconds on my box.

dgoncharov pushed a commit to dgoncharov/bitcoin that referenced this issue Aug 12, 2021
This partically reverts 235b3a7.
Fixes bitcoin#22134.
@dgoncharov
Copy link
Author

This is an example of the how a missing fetched stamp file causes make to skip checksum verification.

$ ls -la /home/dgoncharov/src/bitcoin/depends/sources/download-stamps/.stamp_fetched-libevent*
ls: cannot access '/home/dgoncharov/src/bitcoin/depends/sources/download-stamps/.stamp_fetched-libevent*': No such file or directory
$ make  NO_QT=1 NO_QR=1 NO_ZMQ=1 NO_WALLET=1 NO_BDB=1 NO_SQLITE=1 NO_UPNP=1 NO_NATPMP=1
copying packages: native_b2 boost libevent
to: /home/dgoncharov/src/bitcoin/depends/x86_64-pc-linux-gnu
$

This happens because check_or_remove_sources has the following shell code

test -f $($(package)_fetched) && ( $(build_SHA256SUM) -c $($(package)_fetched) >/dev/null 2>/dev/null || \                      
    ( echo "Checksum missing or mismatched for $(package) source. Forcing re-download."; \                                        
      rm -f $($(package)_all_sources) $($(1)_fetched))) || true
      

Effectively, this shell code tests presence of the fetched stamp file. If the stamp file is missing no checksum verification is performed and the sources are considered good.

@dongcarl
Copy link
Contributor

dongcarl commented Feb 7, 2022

@hebasto, @theuni any thoughts on this? I think it'd be worthwhile to remove such a long, unnecessary step: #22134 (comment)

@hebasto
Copy link
Member

hebasto commented Feb 9, 2022

@hebasto, @theuni any thoughts on this? I think it'd be worthwhile to remove such a long, unnecessary step: #22134 (comment)

Only a few developers in this repo really could concern about efficiency of consequential runs of make -C depends. And for me it is not an issue at all. And benefits of changing makefiles are significantly less in comparison to enabling ccache in depends.

Rebuild of .stamp_$(final_build_id) is the expensive one.
It copies all the cached tarballs and unpacks them sequentially, one by one.

And I appreciate such behavior, because when using consequential runs of make -C depends with different NO_<PACKAGE>=1 variables, it guaranties that prefix directory does not contain any leftovers of a previous build.

@dgoncharov
Copy link
Author

Keeping the behavior intact is an option, given that this apparently does not bother users.
i think, we'd still want to fix skipped checksum verification when the stamp file is absent.
The comment in the commit says this is needed for travis.
Cory or Hennadii, can you please clarify why this copying is still needed?

@willcl-ark
Copy link
Member

There doesn't seem to be much interest in implementing this feature.

Please feel free to submit a pull request if this is something that you still want.

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

No branches or pull requests

5 participants