git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Denton Liu <liu.denton@gmail.com>
To: Git Mailing List <git@vger.kernel.org>
Cc: "Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
	"SZEDER Gábor" <szeder.dev@gmail.com>,
	"Jeff King" <peff@peff.net>, "Junio C Hamano" <gitster@pobox.com>
Subject: [PATCH v3 0/4] Makefile: run coccicheck on all non-upstream sources
Date: Mon, 16 Sep 2019 12:23:03 -0700	[thread overview]
Message-ID: <cover.1568661443.git.liu.denton@gmail.com> (raw)
In-Reply-To: <cover.1568309119.git.liu.denton@gmail.com>

Hi all, I spent the weekend brooding over this and I opted to not
convert $(FIND_SOURCE_FILES) into a list of filenames. I put the
justification for it in 4/4 so, hopefully, it'll make sense for someone
who might want to do the same change in the future.


Before, when we ran coccicheck, it would only run on files that are
currently being compiled. However, this leaves us with a blindspot where
Windows-only sources are not checked since Coccinelle does not run on
Windows.

This patchset addresses this by making the "coccicheck" target run
against all source files in the repository, except for source files that
are pulled from some upstream source.

This patchset should serve as a continuation of the original discussion
about running coccicheck on all sources[1].

These patches depend on "ds/midx-expire-repack" and "dl/compat-cleanup".

[1]: https://public-inbox.org/git/nycvar.QRO.7.76.6.1905031127170.45@tvgsbejvaqbjf.bet/


Denton Liu (4):
  Makefile: strip leading ./ in $(LIB_H)
  Makefile: define THIRD_PARTY_SOURCES
  Makefile: strip leading ./ in $(FIND_SOURCE_FILES)
  Makefile: run coccicheck on more source files

 Makefile | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

Range-diff against v2:
-:  ---------- > 1:  e7fa5e1df7 Makefile: strip leading ./ in $(LIB_H)
1:  72b4cc68f5 ! 2:  a7c855a973 Makefile: define THIRD_PARTY_SOURCES
    @@ Metadata
      ## Commit message ##
         Makefile: define THIRD_PARTY_SOURCES
     
    -    After looking through the source files in compat/ and investigating the
    -    files' content and/or its Git history, I've determined the list of files
    -    that were copied from a third-party source. Place the names of these
    -    files into the THIRD_PARTY_SOURCES variable in the Makefile.
    +    Some files in our codebase are borrowed from other projects, and
    +    minimally updated to suit our own needs. We'd sometimes need to tell
    +    our own sources and these third-party sources apart for management
    +    purposes (e.g. we may want to be less strict about coding style and
    +    other issues on third-party files).
     
    -    In addition, add the sha1collisiondetection/ and sha1dc/ sources as well
    -    since they are also imported from a third-party source.
    -
    -    In a future commit, this variable will be used to determine which files
    -    are excluded when running the "coccicheck" target.
    +    Define the $(MAKE) variable THIRD_PARTY_SOURCES that can be used to
    +    match names of third-party sources.
     
         Signed-off-by: Denton Liu <liu.denton@gmail.com>
    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      ## Makefile ##
     @@ Makefile: SCRIPT_SH =
    @@ Makefile: BUILTIN_OBJS += builtin/verify-tag.o
      BUILTIN_OBJS += builtin/worktree.o
      BUILTIN_OBJS += builtin/write-tree.o
      
    ++# THIRD_PARTY_SOURCES is a list of patterns compatible with
    ++# the $(filter) and $(filter-out) family of functions
     +THIRD_PARTY_SOURCES += compat/inet_ntop.c
     +THIRD_PARTY_SOURCES += compat/inet_pton.c
     +THIRD_PARTY_SOURCES += compat/obstack.%
2:  a51acfec66 ! 3:  71861cbdf8 Makefile: strip leading ./ in $(FIND_SOURCE_FILES)
    @@ Commit message
         to remove the `./` prefix in the $(FIND) case.
     
         Signed-off-by: Denton Liu <liu.denton@gmail.com>
    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      ## Makefile ##
     @@ Makefile: FIND_SOURCE_FILES = ( \
3:  75c36bc9a1 ! 4:  0ecfcd8858 Makefile: run coccicheck on more source files
    @@ Commit message
     
         * compat/obstack.c
     
    +    Instead of generating $(FOUND_C_SOURCES) from a
    +    `$(shell $(FIND_SOURCE_FILES))` invocation, an alternative design was
    +    considered which involved converting $(FIND_SOURCE_FILES) into
    +    $(SOURCE_FILES) which would hold a list of filenames from the
    +    $(FIND_SOURCE_FILES) invocation. We would simply filter `%.c` files into
    +    $(ALL_C_SOURCES). $(SOURCE_FILES) would then be passed directly to the
    +    etags, ctags and cscope commands. We can see from the following
    +    invocation
    +
    +            $ git ls-files '*.[hcS]' '*.sh' ':!*[tp][0-9][0-9][0-9][0-9]*' ':!contrib' | wc -c
    +               12779
    +
    +    that the number of characters in this list would pose a problem on
    +    platforms with short command-line length limits (such as CMD which has a
    +    max of 8191 characters). As a result, we don't perform this change.
    +
    +    However, we can see that the same issue may apply when running
    +    Coccinelle since $(COCCI_SOURCES) is also a list of filenames:
    +
    +            if ! echo $(COCCI_SOURCES) | xargs $$limit \
    +                    $(SPATCH) --sp-file $< $(SPATCH_FLAGS) \
    +                    >$@+ 2>$@.log; \
    +
    +    This is justified since platforms that support Coccinelle generally have
    +    reasonably long command-line length limits and so we are safe for the
    +    foreseeable future.
    +
         Signed-off-by: Denton Liu <liu.denton@gmail.com>
    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      ## Makefile ##
     @@ Makefile: check: command-list.h
    @@ Makefile: check: command-list.h
     -else
     -COCCI_SOURCES = $(filter-out sha1dc/%,$(C_SOURCES))
     -endif
    -+FIND_C_SOURCES = $(filter %.c,$(shell $(FIND_SOURCE_FILES)))
    -+COCCI_SOURCES = $(filter-out $(THIRD_PARTY_SOURCES),$(FIND_C_SOURCES))
    ++FOUND_C_SOURCES = $(filter %.c,$(shell $(FIND_SOURCE_FILES)))
    ++COCCI_SOURCES = $(filter-out $(THIRD_PARTY_SOURCES),$(FOUND_C_SOURCES))
      
      %.cocci.patch: %.cocci $(COCCI_SOURCES)
      	@echo '    ' SPATCH $<; \
-- 
2.23.0


  parent reply	other threads:[~2019-09-16 19:23 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-10  7:44 [PATCH 0/2] Makefile: run coccicheck on all non-upstream sources Denton Liu
2019-09-10  7:44 ` [PATCH 1/2] Makefile: define UPSTREAM_SOURCES Denton Liu
2019-09-10  7:44 ` [PATCH 2/2] Makefile: run coccicheck on more source files Denton Liu
2019-09-10 13:28   ` SZEDER Gábor
2019-09-10 16:07     ` Denton Liu
2019-09-10 14:18   ` SZEDER Gábor
2019-09-12 17:28 ` [PATCH v2 0/3] Makefile: run coccicheck on all non-upstream sources Denton Liu
2019-09-12 17:28   ` [PATCH v2 1/3] Makefile: define THIRD_PARTY_SOURCES Denton Liu
2019-09-12 21:42     ` Junio C Hamano
2019-09-12 17:28   ` [PATCH v2 2/3] Makefile: strip leading ./ in $(FIND_SOURCE_FILES) Denton Liu
2019-09-12 18:18     ` Junio C Hamano
2019-09-13 12:05     ` SZEDER Gábor
2019-09-12 17:28   ` [PATCH v2 3/3] Makefile: run coccicheck on more source files Denton Liu
2019-09-12 18:40     ` Junio C Hamano
2019-09-13 11:49       ` SZEDER Gábor
2019-09-13 17:14         ` Denton Liu
2019-09-13 18:00           ` SZEDER Gábor
2019-09-13 21:38             ` Denton Liu
2019-09-13 17:38         ` Junio C Hamano
2019-09-16 19:23   ` Denton Liu [this message]
2019-09-16 19:23     ` [PATCH v3 1/4] Makefile: strip leading ./ in $(LIB_H) Denton Liu
2019-09-16 19:23     ` [PATCH v3 2/4] Makefile: define THIRD_PARTY_SOURCES Denton Liu
2019-09-16 20:56       ` Junio C Hamano
2019-09-16 22:00         ` [PATCH] fixup! " Denton Liu
2019-09-16 19:23     ` [PATCH v3 3/4] Makefile: strip leading ./ in $(FIND_SOURCE_FILES) Denton Liu
2019-09-16 19:23     ` [PATCH v3 4/4] Makefile: run coccicheck on more source files Denton Liu
2019-09-16 20:57     ` [PATCH v3 0/4] Makefile: run coccicheck on all non-upstream sources Junio C Hamano
2019-09-17  8:18       ` SZEDER Gábor
2019-09-17 16:13         ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=cover.1568661443.git.liu.denton@gmail.com \
    --to=liu.denton@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    --cc=szeder.dev@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).