RFR: JDK-8001931 The new build system whitespace cleanup

[Volker Simonis]

Hi,

First of all just a little hint to other reviewers - only the "patch"-view
really shows all the changes. All the other views (Cdiffs, Udiffs, Sdiffs,
Frames) show much less and are sometimes even empty!

I've just started looking into this, but if I understand point 5:

5) Non-shell commands in a recipe (e.g. comments and make directives like
ifdef) must not start with tab, but should instead be indented to the same
level as the surrounding shell commands using spaces (with tabs interpreted
as 8 spaces wide).

correctly, the changes to jdk/makefiles/BuildJdk.gmk for example should be:

 gensrc-only:
         +$(MAKE) -f GenerateJavaSources.gmk
-#        Ok, now gensrc is fully populated.
+        # Ok, now gensrc is fully populated.

(i.e. comments should be indented to the same level as the surrounding
commands) instead of:

 gensrc-only:
         +$(MAKE) -f GenerateJavaSources.gmk
-#        Ok, now gensrc is fully populated.
+#       Ok, now gensrc is fully populated.

Or do you mean "... the content of the comments without the comment token
'#' ... "?

In my opinion I think indenting the complete comment makes the whole
make-file looking cleaner and it also makes it clearer to the reader that
the comment or ifdef belongs to a recipe. Placing the comments or make
directives at the beginning of the line and only indenting the comment text
or make directive content somehow divides the whole recipe which doesn't
look so nice.

Regards,
Volker


On Tue, Oct 8, 2013 at 9:42 AM, Magnus Ihse Bursie <
magnus.ihse.bursie at oracle.com> wrote:

> Based on the code conventions posted earlier, Erik and I have made a
> complete overhaul on the use of whitespace and indentation in the new build
> system.
>
> We set up an important limitation on this change: *only* whitespace should
> be changed. This self-imposed limitation helped us verify that nothing else
> was accidentally changed.
>
> I have verified (using basically "cat * | tr -d ' \t\n' >
> all-files-without-whitespace.**txt") that this change does indeed only
> change whitespace.
>
> It should be noted, that while this is a comfort, it is not a 100%
> guarantee on the correctness of this change, since whitespace in makefiles
> sometimes have semantic meaning.
>
> This also means that we were not able to add line breaks, since most line
> breaks would require us to add a '\' at the end, thus breaking the
> whitespace-only properly. We are considering complementing this change with
> an additional change without this strict limitation, that can fix some of
> the remaining issues we ran into.
>
> Unfortunately, apart from the whitespace-only guarantee, this is a hard
> change to review. Webrev does not do a very good job at presenting
> whitespace-only changes (a behavior shared with most diff utilities, we
> learnt). Also, a lot of files are changed, even though most of the files
> have only minimal changes in them.
>
> When preparing this change, we combined manual editing with automatic
> tools. All automatic changes were manually reviewed, and any bad changes
> reverted.
>
> The automatic changes performed include:
>  * Remove trailing whitespace
>  * No tabs except at start of line
>  * When breaking lines, always end with ' \'
>  * Condense multiple space in lines into one
>
> Purely manual editing included:
>  * Fix indentation levels
>  * Add space around assignment operators, where possible
>  * Add space after comma in function calls, where possible
>
> We have tested our changes on JRPT, the internal test system, and it works
> well.
>
> WebRevs:
> Note: The webrevs are created with -b, to show anything meaningful at all.
>
> root: http://cr.openjdk.java.net/~**ihse/JDK-8001931-build-infra-**
> whitespace-cleanup/root/**webrev.01<http://cr.openjdk.java.net/~ihse/JDK-8001931-build-infra-whitespace-cleanup/root/webrev.01>
> jdk: http://cr.openjdk.java.net/~**ihse/JDK-8001931-build-infra-**
> whitespace-cleanup/jdk/webrev.**01<http://cr.openjdk.java.net/~ihse/JDK-8001931-build-infra-whitespace-cleanup/jdk/webrev.01>
> langtools: http://cr.openjdk.java.net/~**ihse/JDK-8001931-build-infra-**
> whitespace-cleanup/langtools/**webrev.01<http://cr.openjdk.java.net/~ihse/JDK-8001931-build-infra-whitespace-cleanup/langtools/webrev.01>
> corba: http://cr.openjdk.java.net/~**ihse/JDK-8001931-build-infra-**
> whitespace-cleanup/corba/**webrev.01<http://cr.openjdk.java.net/~ihse/JDK-8001931-build-infra-whitespace-cleanup/corba/webrev.01>
> jaxp: http://cr.openjdk.java.net/~**ihse/JDK-8001931-build-infra-**
> whitespace-cleanup/jaxp/**webrev.01<http://cr.openjdk.java.net/~ihse/JDK-8001931-build-infra-whitespace-cleanup/jaxp/webrev.01>
> jaxws: http://cr.openjdk.java.net/~**ihse/JDK-8001931-build-infra-**
> whitespace-cleanup/jaxws/**webrev.01<http://cr.openjdk.java.net/~ihse/JDK-8001931-build-infra-whitespace-cleanup/jaxws/webrev.01>
>
> Bug: https://bugs.openjdk.java.net/**browse/JDK-8001931<https://bugs.openjdk.java.net/browse/JDK-8001931>
>
> /Magnus
>
>