On Fri, 30 Aug 2024 16:27:18 GMT, Magnus Ihse Bursie <[email protected]> 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
