| Message ID | 48fdd198-93ad-7282-27e6-9a0c6de93067@ramsayjones.plus.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | speed up 'make clean' | expand |
Ramsay Jones <ramsay@ramsayjones.plus.com> writes: > diff --git a/.gitignore b/.gitignore > index 6232d33924..425b8cc2a4 100644 > --- a/.gitignore > +++ b/.gitignore > @@ -191,6 +191,7 @@ > /gitweb/static/gitweb.min.* > /config-list.h > /command-list.h > +/dist-tars > *.tar.gz > *.dsc > *.deb > diff --git a/Makefile b/Makefile > index 90e91a2185..bc9ce28bc3 100644 > --- a/Makefile > +++ b/Makefile > @@ -3083,6 +3083,7 @@ dist: git-archive$(X) configure > --prefix=$(GIT_TARNAME)/ HEAD^{tree} > $(GIT_TARNAME).tar > @$(RM) -r .dist-tmp-dir > gzip -f -9 $(GIT_TARNAME).tar > + @echo $(GIT_TARNAME).tar.gz >>dist-tars Sorry, but I'd rather not to see such a longer-term "list of files to be removed" on the filesystem. This invites attackers to write a rogue test addition that writes into ../../dist-tars something like "~/.gitconfig" and wait for me to say "make clean".
Junio C Hamano <gitster@pobox.com> writes: > Ramsay Jones <ramsay@ramsayjones.plus.com> writes: > >> diff --git a/.gitignore b/.gitignore >> index 6232d33924..425b8cc2a4 100644 >> --- a/.gitignore >> +++ b/.gitignore >> @@ -191,6 +191,7 @@ >> /gitweb/static/gitweb.min.* >> /config-list.h >> /command-list.h >> +/dist-tars >> *.tar.gz >> *.dsc >> *.deb >> diff --git a/Makefile b/Makefile >> index 90e91a2185..bc9ce28bc3 100644 >> --- a/Makefile >> +++ b/Makefile >> @@ -3083,6 +3083,7 @@ dist: git-archive$(X) configure >> --prefix=$(GIT_TARNAME)/ HEAD^{tree} > $(GIT_TARNAME).tar >> @$(RM) -r .dist-tmp-dir >> gzip -f -9 $(GIT_TARNAME).tar >> + @echo $(GIT_TARNAME).tar.gz >>dist-tars > > Sorry, but I'd rather not to see such a longer-term "list of files > to be removed" on the filesystem. This invites attackers to write a > rogue test addition that writes into ../../dist-tars something like > "~/.gitconfig" and wait for me to say "make clean". Having said that, I also think that "make clean" target should help your desire to keep tarballs that would not have been generated by the current checkout (e.g. the last release), and I do not think $(RM) git-htmldocs-*.tar.gz git-manpages-*.tar.gz is an unconditionally better alternative to what you did in this step. How about moving removal of these "distribution" artifacts that are created by "make dist$something" targets from "clean" to "distclean"?
On 05/11/2020 22:01, Junio C Hamano wrote: > Ramsay Jones <ramsay@ramsayjones.plus.com> writes: > >> diff --git a/.gitignore b/.gitignore >> index 6232d33924..425b8cc2a4 100644 >> --- a/.gitignore >> +++ b/.gitignore >> @@ -191,6 +191,7 @@ >> /gitweb/static/gitweb.min.* >> /config-list.h >> /command-list.h >> +/dist-tars >> *.tar.gz >> *.dsc >> *.deb >> diff --git a/Makefile b/Makefile >> index 90e91a2185..bc9ce28bc3 100644 >> --- a/Makefile >> +++ b/Makefile >> @@ -3083,6 +3083,7 @@ dist: git-archive$(X) configure >> --prefix=$(GIT_TARNAME)/ HEAD^{tree} > $(GIT_TARNAME).tar >> @$(RM) -r .dist-tmp-dir >> gzip -f -9 $(GIT_TARNAME).tar >> + @echo $(GIT_TARNAME).tar.gz >>dist-tars > > Sorry, but I'd rather not to see such a longer-term "list of files > to be removed" on the filesystem. This invites attackers to write a > rogue test addition that writes into ../../dist-tars something like > "~/.gitconfig" and wait for me to say "make clean". Yes, that is a reasonable concern. I suppose we could drop the last two patches then - most of the saving comes from the first four patches (as you can see from the table in the cover letter). Also, I had an alternative patch for the last patch, which did away with the '-include GIT-VERSION-FILE' entirely! (That had treewide implications that I hadn't sorted through yet). ATB, Ramsay Jones
On 05/11/2020 23:05, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: [snip] >> Sorry, but I'd rather not to see such a longer-term "list of files >> to be removed" on the filesystem. This invites attackers to write a >> rogue test addition that writes into ../../dist-tars something like >> "~/.gitconfig" and wait for me to say "make clean". > > Having said that, I also think that "make clean" target should help > your desire to keep tarballs that would not have been generated by > the current checkout (e.g. the last release), and I do not think > > $(RM) git-htmldocs-*.tar.gz git-manpages-*.tar.gz > > is an unconditionally better alternative to what you did in this > step. > > How about moving removal of these "distribution" artifacts that are > created by "make dist$something" targets from "clean" to "distclean"? I did have a patch #9 that moved the removal of the tarballs from the 'clean' target to the 'distclean' target (that is move the new 'iterate over the dist-tars file' code). However, I wasn't convinced that it was a better place for it. Also, just moving the new removal code would not alter the concerns about it that you express above. So, we need to address that either way. Thanks. ATB, Ramsay Jones
Ramsay Jones <ramsay@ramsayjones.plus.com> writes: > I did have a patch #9 that moved the removal of the tarballs from > the 'clean' target to the 'distclean' target (that is move the new > 'iterate over the dist-tars file' code). However, I wasn't convinced > that it was a better place for it. I am not either, other than "things created by 'make distX' cleaned by 'make distclean'---that sounds consistent" ;-). > Also, just moving the new removal code would not alter the concerns > about it that you express above. So, we need to address that either > way. "make distclean" can still depend on GIT-VERSION-FILE and your daily cleaning needs can use "make clean" that cleans only the files that can be cleaned without knowing what GIT_VERSION is, no? That way, we do not have to use dist-tars file at all, no?
On 06/11/2020 01:58, Junio C Hamano wrote: > Ramsay Jones <ramsay@ramsayjones.plus.com> writes: > >> I did have a patch #9 that moved the removal of the tarballs from >> the 'clean' target to the 'distclean' target (that is move the new >> 'iterate over the dist-tars file' code). However, I wasn't convinced >> that it was a better place for it. > > I am not either, other than "things created by 'make distX' cleaned > by 'make distclean'---that sounds consistent" ;-). :-D > >> Also, just moving the new removal code would not alter the concerns >> about it that you express above. So, we need to address that either >> way. > > "make distclean" can still depend on GIT-VERSION-FILE and your daily > cleaning needs can use "make clean" that cleans only the files that > can be cleaned without knowing what GIT_VERSION is, no? That way, > we do not have to use dist-tars file at all, no? Hmm, so rather than dropping the last two patches, you are suggesting replacing this patch with a patch that moves: $(RM) $(GIT_TARNAME).tar.gz $(RM) $(htmldocs).tar.gz $(manpages).tar.gz to the 'distclean' target? Well, that would address your concerns, but I was hoping to fix the 'your current branch has to be the same as the one you created the tarballs on' for the '(dist)clean' target to actually remove those files issue. As I said in the commit message, my preferred solution was to simply put them in a 'dist-tars/' directory. Then you would only delete the files actually generated by the 'dist' and 'dist-doc' targets, and you could easily remove that directory (be it in 'clean' or 'distclean' or 'clean-dist-tars', ...). Hmm, I wonder if my concerns about that solution are overblown? dunno. ATB, Ramsay Jones
Ramsay Jones <ramsay@ramsayjones.plus.com> writes: >> "make distclean" can still depend on GIT-VERSION-FILE and your daily >> cleaning needs can use "make clean" that cleans only the files that >> can be cleaned without knowing what GIT_VERSION is, no? That way, >> we do not have to use dist-tars file at all, no? > > Hmm, so rather than dropping the last two patches, you are suggesting > replacing this patch with a patch that moves: > > $(RM) $(GIT_TARNAME).tar.gz > $(RM) $(htmldocs).tar.gz $(manpages).tar.gz > > to the 'distclean' target? Yup. FWIW, I consider it a feature that for m in maint-2.{27,28,29} do git checkout "$m" && make distclean && make dist || break do gives me three distribution tarballs of from-scratch builds. It matters when I need to push out releases from multiple maintenance tracks at the same time (think: coordinated security releases). I could of course move them away before running distclean in each step, but then I'd be following a different workflow from what I normally use when cutting a single release---I would rather avoid deviating from the routine when I have to be absolutely careful (again, think: coordinated security releases). > Well, that would address your concerns, but I was hoping to fix the > 'your current branch has to be the same as the one you created the > tarballs on' for the '(dist)clean' target to actually remove those > files issue. So we on this part have directly opposing wish, unfortunately.
On 06/11/2020 17:53, Junio C Hamano wrote: > Ramsay Jones <ramsay@ramsayjones.plus.com> writes: >> Hmm, so rather than dropping the last two patches, you are suggesting >> replacing this patch with a patch that moves: >> >> $(RM) $(GIT_TARNAME).tar.gz >> $(RM) $(htmldocs).tar.gz $(manpages).tar.gz >> >> to the 'distclean' target? > > Yup. FWIW, I consider it a feature that > > for m in maint-2.{27,28,29} > do > git checkout "$m" && > make distclean && > make dist || break > do > > gives me three distribution tarballs of from-scratch builds. It > matters when I need to push out releases from multiple maintenance > tracks at the same time (think: coordinated security releases). Ah, yes. I hadn't thought about that kind of usage. Hmm, but in that case, wouldn't you rather get rid of the deletion of the tarballs in the 'clean' target completely? Then you could either remove them by hand (which I actually always do in practice anyway) at a time that suits you, or maybe add a new 'clean-tarballs' target. (That still leaves the issue of how you would identify the tarballs in such a target, of course). ATB, Ramsay Jones
diff --git a/.gitignore b/.gitignore index 6232d33924..425b8cc2a4 100644 --- a/.gitignore +++ b/.gitignore @@ -191,6 +191,7 @@ /gitweb/static/gitweb.min.* /config-list.h /command-list.h +/dist-tars *.tar.gz *.dsc *.deb diff --git a/Makefile b/Makefile index 90e91a2185..bc9ce28bc3 100644 --- a/Makefile +++ b/Makefile @@ -3083,6 +3083,7 @@ dist: git-archive$(X) configure --prefix=$(GIT_TARNAME)/ HEAD^{tree} > $(GIT_TARNAME).tar @$(RM) -r .dist-tmp-dir gzip -f -9 $(GIT_TARNAME).tar + @echo $(GIT_TARNAME).tar.gz >>dist-tars rpm:: @echo >&2 "Use distro packaged sources to run rpmbuild" @@ -3112,6 +3113,7 @@ dist-doc: $(MAKE) -C Documentation WEBDOC_DEST=../.doc-tmp-dir install-webdoc cd .doc-tmp-dir && $(TAR) cf ../$(htmldocs).tar $(TAR_DIST_EXTRA_OPTS) . gzip -n -9 -f $(htmldocs).tar + @echo $(htmldocs).tar.gz >>dist-tars : $(RM) -r .doc-tmp-dir mkdir -p .doc-tmp-dir/man1 .doc-tmp-dir/man5 .doc-tmp-dir/man7 @@ -3122,6 +3124,7 @@ dist-doc: install cd .doc-tmp-dir && $(TAR) cf ../$(manpages).tar $(TAR_DIST_EXTRA_OPTS) . gzip -n -9 -f $(manpages).tar + @echo $(manpages).tar.gz >>dist-tars $(RM) -r .doc-tmp-dir ### Cleaning rules @@ -3151,8 +3154,12 @@ clean: profile-clean coverage-clean cocciclean $(RM) -r po/build/ $(RM) *.pyc *.pyo */*.pyc */*.pyo $(GENERATED_H) $(ETAGS_TARGET) tags cscope* $(RM) -r .dist-tmp-dir .doc-tmp-dir - $(RM) $(GIT_TARNAME).tar.gz - $(RM) $(htmldocs).tar.gz $(manpages).tar.gz + @if test -s dist-tars; then \ + for i in $$(cat dist-tars); do \ + $(RM) $$i; \ + done \ + fi + $(RM) dist-tars $(MAKE) -C Documentation/ clean $(RM) Documentation/GIT-EXCLUDED-PROGRAMS ifndef NO_PERL
A future patch requires the 'clean' target not to depend, either directly or indirectly, on the $(GIT_VERSION) variable. However, the distribution tarballs, $(GIT_TARNAME).tar.gz, $(htmldocs).tar.gz and $(manpages).tar.gz, all depend on $(GIT_VERSION). The 'clean' target attempts to remove these tarballs by name and so has the unwanted dependency. The first attempt to remove this dependency involved creating the distribution tarballs in a new top-level directory (e.g. 'dist-tars/'), that didn't reference the version number, and could be simply removed in the 'clean' target. Unfortunately, this could lead to breaking an unknown number of scripts for an unknown number of developers. (This actually breaks one of my own scripts!). Despite this being my preferred solution, I had to abandon this approach, since I don't know what problems it may cause. The second attempt involved using a wildcard pattern, in place of the $(GIT_VERSION) variable, in the distribution tarball filenames. This also proved to be an inadequate solution, no matter how elaborate the pattern became, because it was always possible that it could lead to the removal of some '*.tar.gz' file that would, otherwise, not have been removed (eg. git-2.29.0-saved-build.tar.gz). Also, note that the current 'clean' target requires that you do not move the current branch away from the commit you were on, when creating the distribution tarballs, before issuing the 'make clean'. If you do so, then you will find that the tarballs are not removed: $ git checkout nclean ... $ make dist ... $ ls *.tar.gz git-2.29.0.6.g8255a76caf.tar.gz $ $ git checkout master Switched to branch 'master' Your branch is up to date with 'origin/master'. $ make clean ... rm -f git-2.29.2.154.g8a58376a31.tar.gz rm -f git-htmldocs-2.29.2.154.g8a58376a31.tar.gz git-manpages-2.29.2.154.g8a58376a31.tar.gz ... $ $ ls *.tar.gz git-2.29.0.6.g8255a76caf.tar.gz $ [I always find the documentation tarballs of the last release intact when I am just about to create the new tarballs for this release. This means that I invariably remove them by hand.] In order to remove the version dependency, append the name of each distribution tarball created, in the 'dist' and 'doc-dist' targets, to a file ('dist-tars'). Then in the 'clean' target, simply iterate through the names in this file, if any, removing them as we go. (Not forgetting to clean up the 'dist-tars' file as well). Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com> --- .gitignore | 1 + Makefile | 11 +++++++++-- 2 files changed, 10 insertions(+), 2 deletions(-)