On Tue, 30 May 2023 13:46:55 GMT, Erik Helin <ehe...@openjdk.org> wrote:
>> make/InitSupport.gmk line 251: >> >>> 249: ifeq ($$(words $$(all_spec_files)), 1) >>> 250: # We found exactly one configuration, use it >>> 251: SPECS := $$(strip $$(all_spec_files)) >> >> It may be better to keep this as the first action and only do the git checks >> if necessary. That would minimise potential interference with single branch >> repos. > > That is a good idea, and I thought about it, but it is hard to structure the > logic that way and not end up with duplicated code (early return doesn't seem > to be a thing in GNU Make macros). What I means that the following is (at > least to me) hard to express in a GNU Make macro: > > > ifeq ($$(words $$(all_spec_files)), 1) > # Somehow set SPECS and return, do not continue to execute > endif > GIT := ... > ifneq ($$(GIT),) > ... > endif > ... > > The two Git commands potentially being executed are `O(1)`, they do not scale > with repository size and should have minimal overhead. Since both flags to > `git rev-parse` being used have been around since 2009 I think the risk of > encountering a Git installation where they do not exist or do not work is > very low. The `rev-parse` command is a so called "plumbing" command in Git > which means that the Git project considers the CLI interface an API and keep > it backwards compatible (so we shouldn't run in to future Git releases > renaming and/or removing these flags). I therefore think it is worth keeping > the logic a bit nicer and instead run these two extra `git rev-parse` > commands and one extra `command -v` invocation (sometimes a shell built-in) > for the single branch repo case. Only evaluating the external commands when needed is a hard requirement here. Running a few extra external commands on every make invocation may not be noticeable on Linux, but on Windows it quickly adds up, and we have done several passes through the makefiles to eliminate those over the years. You should be able to avoid repeated code by nesting if blocks. It may also be a good idea be explicit about running `git` inside `$(topdir)` (defined in `Makefile`), but that can be debated. That variable is always pointing to the top of the OpenJDK repository. Your current implementation is based on CWD which may be in some other repository depending on the workspace setup. If going with `$(topdir)` the first git invocation can be replaced with checking for `$(wildcard $(topdir)/.git)`. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/14202#discussion_r1213452468
