LGTM. Thanks, Jose
On Fri, Oct 18, 2013 at 01:18:34PM +0200, Michele Tartara wrote: > On Fri, Oct 18, 2013 at 1:07 PM, Jose A. Lopes <[email protected]> wrote: > > On Fri, Oct 18, 2013 at 10:51:37AM +0000, Michele Tartara wrote: > >> Checking the correctness of the NEWS file syntax is an important part of > >> the > >> check process, but up to now it was only possible as part of a bigger set > >> of > >> tests. This commit creates a Makefile target to run that independently. > >> > >> The developer notes are updated to document this new target. > >> > >> Signed-off-by: Michele Tartara <[email protected]> > >> --- > >> Makefile.am | 6 ++++-- > >> doc/devnotes.rst | 4 ++++ > >> 2 files changed, 8 insertions(+), 2 deletions(-) > >> > >> diff --git a/Makefile.am b/Makefile.am > >> index d15854e..0bcd349 100644 > >> --- a/Makefile.am > >> +++ b/Makefile.am > >> @@ -2027,11 +2027,13 @@ check-dirs: $(GENERATED_FILES) > >> } > >> > >> .PHONY: check-local > >> -check-local: check-dirs $(GENERATED_FILES) > >> +check-news: > > > > Isn't a dependency on $(CHECK_NEWS) or 'check-dirs' necessary ? > > $(CHECK_NEWS) is distributed with the source, and isn't supposed to > change, so I don't see why a dependency would be of any use (and in > fact, it's not there in the existing code either). > > check-dirs is checking that the actual structure of the directories is > as described in the Makefile. Which makes it a part (and a dependence) > of check-local, but it has no link to check-news, that only tests the > NEWS file. > > > What about a dependency on the 'NEWS' file? > I don't think a dependency from the NEWS file is needed. check-news is > not building anything. It should just run the check everytime it is > invoked, no matter if the NEWS file was updated or not. So, it's not a > dependence. > But this question made me realize that check-news is actually a phony > target and should be indicated as such. > > Interdiff: > diff --git a/Makefile.am b/Makefile.am > index 0bcd349..8049d6d 100644 > --- a/Makefile.am > +++ b/Makefile.am > @@ -2026,10 +2026,11 @@ check-dirs: $(GENERATED_FILES) > test -z "$$error"; \ > } > > -.PHONY: check-local > +.PHONY: check-news > check-news: > RELEASE=$(PACKAGE_VERSION) $(CHECK_NEWS) < $(top_srcdir)/NEWS > > +.PHONY: check-local > check-local: check-dirs check-news $(GENERATED_FILES) > $(CHECK_PYTHON_CODE) $(check_python_code) > PYTHONPATH=. $(CHECK_HEADER) $(check_python_code) > > > >> + RELEASE=$(PACKAGE_VERSION) $(CHECK_NEWS) < $(top_srcdir)/NEWS > > > > Isn't it safer to use 'env' ? > > > > env RELEASE=$(PACKAGE_VERSION) ... > > I reused the exact same code we are using currently. Why should "env" be > safer? > > Thanks, > Michele > > -- > Google Germany GmbH > Dienerstr. 12 > 80331 München > > Registergericht und -nummer: Hamburg, HRB 86891 > Sitz der Gesellschaft: Hamburg > Geschäftsführer: Graham Law, Christine Elizabeth Flores -- Jose Antonio Lopes Ganeti Engineering Google Germany GmbH Dienerstr. 12, 80331, München Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Graham Law, Christine Elizabeth Flores Steuernummer: 48/725/00206 Umsatzsteueridentifikationsnummer: DE813741370
