RFR: 8339336: Fix build system whitespace to adhere to coding conventions
Erik Joelsson
erikj at openjdk.org
Fri Aug 30 20:26:23 UTC 2024
On Fri, 30 Aug 2024 16:27:18 GMT, Magnus Ihse Bursie <ihse at openjdk.org> wrote:
> The build system code has unfortunately diverted in some places from the conventions as described in https://openjdk.org/groups/build/doc/code-conventions.html.
>
> Instead of trying to fix these when touching code nearby, I'd like to make an effort to fix all issues at once and separately. Incremental fixes has their benefit, but they can also muddy the actual fix and are not always appreciated.
>
> The updates in this patch have all been discovered using automated tools, but each and every change has been manually scrutinized. Those that the automatic tools pointed out that, but that were not obviously or clear-cut safe (e.g. adding spaces after comma, in `subst` or similar situations) were reverted before I pushed. I chose to err on the "First, do no harm" side, so there might be places that could have been corrected, but were not.
>
> I have made a single type of change per commit in this branch. It might be easier to review this by looking at one commit at a time.
I'm pretty sure most of these are safe changes, but there are some where I'm a bit hesitant. I would feel safer if you ran a full compare build on it and also manually verified a few incremental build scenarios, including running tests.
In general I very much approve of this patch, thank you for taking the time to clean house! I'm amazed at the number of 79 character wide comment line dividers.
make/MainSupport.gmk line 202:
> 200: $(foreach i, 2 3 4 5 6 7 8, $(if $(strip $($i)),$(strip $1)_$(strip $($i)))$(NEWLINE))
> 201: $(if $(9), $(error Internal makefile error: Too many arguments to \
> 202: DeclareRecipesForPhase, please update MakeHelper.gmk))
I know this is a whitespace cleanup, so this should likely be addressed in a separate issue, but this error message is referencing `MakeHelper.gmk` while being in the file `MainSupport.gmk`. Also, shouldn't this be able to use the `NamedParamsMacroTemplate` instead of using this old technique for named arguments?
make/test/BuildMicrobenchmark.gmk line 71:
> 69: # Need double \n to get new lines and no trailing spaces
> 70: MICROBENCHMARK_MANIFEST := Build: $(FULL_VERSION)\n \
> 71: \nJMH-Version: $(JMH_VERSION)\n \
Are you sure these spaces are safe? The comment mentions trailing spaces as something undesired and depending on how this is used, I suspect adding a space between the \n could interfere with that.
-------------
PR Review: https://git.openjdk.org/jdk/pull/20798#pullrequestreview-2273355090
PR Review Comment: https://git.openjdk.org/jdk/pull/20798#discussion_r1739308489
PR Review Comment: https://git.openjdk.org/jdk/pull/20798#discussion_r1739378849
More information about the build-dev
mailing list