On 11/5/2013 1:31 AM, Erik Joelsson wrote:
On 2013-11-04 20:57, Bradford Wetmore wrote:
Hi Erik,
Thanks for the update! Looks good, but one minor comment:
SignJars.gmk
============
78:100:106:111-116 > 80 chars per line.
Thanks. We have not enforced 80 chars in any other file in the new build
system. Has this been a habit before?
Depends on who you ask, and be prepared for the 80 chars Holy Wars[1].
According to the old Java style guide, 80 is the rule for Java files, so
many of us tend to apply it throughout the JDK codebase. Some
developers are adamant about not following it. "Get a modern
terminal..." Just google "java 80 characters" and you'll see what I mean.
From my point of view, as someone who routinely has multiple windows
side by side for easy file change comparisons, I'd *MUCH* prefer having
3@80 chars than 2@120 (or 1@something really wide) (using a 8x12 font).
When working on laptops with smaller screens, even 2@80 gets tight
(without squinting). This also impacts things like webrev's sdiffs &
frames: if you have two 160 line reviews, that's a wide window that you
have to vertically scroll (twice for frames). On a 24" screen at max
resolution of 1680x1050, the 160 char side-by-side lines *REALLY* suck
to review.
I personally find it very
impractical, especially with make adding a tab indent to each recipe
line already, and because we tend to use rather long and descriptive
variable names. I still try to keep lines reasonably short without
forcing breaks that just make things less readable.
I will defer to the build team, this is primarily your code, but for my
$0.02, I almost always keep all code <= 80 unless there's a really good
reason. If I need to touch long line code, I will generally shorten it,
and I liberally use "\" (make) and "+" (java strings). I won't play
"Format Tennis" unless the code is just unreadable: fortunately I don't
work in some of the other code within the JDK.
Brad
[1] Holy Wars = Differences of Opinion, with people ready to fight for
their viewpoint.
/Erik
Brad
On 11/4/2013 2:19 AM, Erik Joelsson wrote:
New webrev posted:
http://cr.openjdk.java.net/~erikj/8027698/webrev.jdk.02/
Addressed all comments below. I must have missed the save button before
generating the webrev when adding ucrypto.jar, but it's there now at
least.
/Erik
On 2013-11-02 01:35, Bradford Wetmore wrote:
On 11/1/2013 3:34 AM, Erik Joelsson wrote:
Please review this simple fix, adding sunmscapi.jar and ucrypto.jar to
the list of jars to be signed by the sign-jars target. Since these
jars
are platform dependent, they aren't always present.
Bug: https://bugs.openjdk.java.net/browse/JDK-8027698
Webrev: http://cr.openjdk.java.net/~erikj/8027698/webrev.jdk.01/
/Erik
makefiles/SignJars.gmk
======================
Overall: Some of the existing/new lines are greater than the standard
80 chars.
2: Some people update the Copyright date instead of waiting for RE to
do it. Feel free if you're one of those.
48 & 108: The RE process for signing jars for JDK 7/8 is completely
different than what was originally written/described for JDK 7. There
is an additional separate script that RE uses to include the
source_tips into the jar files, and then do the actual signing.
SignJars.gmk is here primarily for developer support.
47: Suggest changing to:
# SPECIAL NOTE TO JCE/JDK developers: The source files must eventually
# be built, signed, and then the resulting jar files MUST BE CHECKED
# INTO THE CLOSED PART OF THE WORKSPACE*. This separate step *MUST NOT
# BE FORGOTTEN*, otherwise a bug fixed in the source code will not be
# reflected in the shipped binaries.
#
# Please consult with Release Engineering, which is responsible for
# creating the final JCE builds suitable for checkin.
#
94: I don't see ucrypto.jar listed here.
108: Can you please update the code here to output:
The jar files built by the 'sign-jars' target are developer
builds only and *MUST NOT* be checked into the closed workspace.
Please consult with Release Engineering: they will generate
the proper binaries for the closed workspace.
@$(PRINTF) $(README-MAKEFILE_WARNING)
Since this bug is a subtask of JDK-8027266, I'll add a few more
comments to that bug. I'm guessing you'll probably want to address
separately.
Thanks, Erik!
Brad
