RFR: JDK-8238534

[René Schünemann]

Hello Erik,

thank you for your review. Please see my answers below.

Rene

On Mon, Feb 10, 2020 at 9:34 PM Erik Joelsson <erik.joelsson at oracle.com> wrote:
>
> Hello René,
>
> That looks better. I still have some issues though.
>
> I don't understand line 273 and 305. There is no reason to declare those
> rules.
>
> Line 311, the CodeResources file needs prerequisites. Those should be
> $(CREATE_JDK_BUNDLE_DIR_SIGNED) (which is the list of all files copied
> into the signed image).
>

This doesn't work for me. Without declaring this rule I get:

Building target 'product-bundles' in configuration
'macosx-x86_64-server-release'
gmake[3]: *** No rule to make target
'[...]/hg/jdk/build/macosx-x86_64-server-release/images/jdk-bundle-signed/jdk-15.jdk',
needed by [...]/hg/jdk/build/macosx-x86_64-server-release/bundles/jdk-15-internal+0_osx-x64_bin.tar.gz'.
Stop.
gmake[2]: *** [make/Main.gmk:627: product-bundles] Error 2

ERROR: Build failed for target 'product-bundles' in configuration
'macosx-x86_64-server-release' (exit code 2)

> Instead of piping to /dev/null, I recommend using the macro
> $(LOG_DEBUG). It will resolve to >/dev/null for log levels less detailed
> than debug. Does codesign output to stderr on successful signing? If
> not, leave the stderr alone. If something goes wrong we want to see it
> in the build log.
>

I will do that.

> The FILES list for BUILD_JDK_BUNDLE should be
> $(CREATE_JDK_BUNDLE_DIR_SIGNED) and
> $(JDK_MACOSX_BUNDLE_DIR_SIGNED)/$(JDK_MACOSX_BUNDLE_TOP_DIR). Those are
> the exact files that should be included and by specifying them we get
> correct dependency declarations.
>

I tried this, it didn't work for me. I tried:

$(JDK_SIGNED_CODE_RESOURCES): $(CREATE_JDK_BUNDLE_DIR_SIGNED)

and/or

$(BUILD_JDK_BUNDLE): $(CREATE_JDK_BUNDLE_DIR_SIGNED)

and/or

$(CREATE_JDK_BUNDLE_DIR_SIGNED) in the file list of BUILD_JDK_BUNDLE

Without the line 273 and 305 rules make bails out with the said error.

> When signing the bundle, you should not need to specify entitlements.
> Those should only be supplied when signing executables that actually
> need them. Not sure if --force is a good idea here either.
>

The --force flag is needed, because libjli.dylib is getting re-signed
in the process. Without this flag codesign fails with:

[...]/hg/jdk/build/macosx-x86_64-server-release/images/jdk-bundle-signed/jdk-15.jdk:
is already signed

Can you confirm, that libjli.dylib doesn't need the entitlements?

> /Erik
>
> On 2020-02-10 10:12, René Schünemann wrote:
> > Hi Erik,
> >
> > I have implemented your requested changes. I think it is a lot cleaner
> > now and the bundling as well as the
> > signing parts are now only executed when necessary.
> >
> > New WebRev: http://cr.openjdk.java.net/~rschuenemann/wr/2020/8238534-macos_sign_bundles/02/
> >
> > Rene
> >
> > On Mon, Feb 10, 2020 at 9:23 AM René Schünemann
> > <rene.schuenemann at gmail.com> wrote:
> >> Hi Erik,
> >>
> >> thank you for your review and I totally agree with you. It would
> >> definitely be better avoid temp dirs.
> >> I will try to move the creation of the signed image to MacBundles.gmk
> >> and then re-use the SetubBundleFile in Bundles.gmk.
> >>
> >> Rene
> >>
> >> On Fri, Feb 7, 2020 at 5:19 PM Erik Joelsson <erik.joelsson at oracle.com> wrote:
> >>> Hello René,
> >>>
> >>> It's good to see an open solution to this, but I have some opinions on
> >>> the patch.
> >>>
> >>> The concept of building into "temp dirs" that are then removed is a
> >>> practice we try to avoid in the build. Whenever possible, each rule
> >>> should be a well defined transformation from a set of source files to a
> >>> target file. There is just no reason to remove the jdk-signed dir here.
> >>> If something goes wrong, you would want the dir around to investigate.
> >>> This also keeps incremental builds working as expected. Your current
> >>> patch will always rebuild the bundles, which is not ok.
> >>>
> >>> I would recommend putting the jdk-signed dir in
> >>> $(IMAGES_OUTPUTDIR)/jdk-signed and just leave it there. I would create a
> >>> separate rule for the signing part, where the target file is the
> >>> CodeResources file that codesign actually creates, and the prerequisite
> >>> files simply $(COPY_SIGNED_JDK_BUNDLE).
> >>>
> >>> Separate rules for creating a top level directory are not needed. The
> >>> rules generated from SetupCopyFiles will create all directories needed.
> >>>
> >>> I would also keep using the existing SetupBundleFile for the actual
> >>> bundling, even if most of the functionality in it is not used, just to
> >>> avoid more separate code paths than necessary.
> >>>
> >>> /Erik
> >>>
> >>> On 2020-02-07 02:05, René Schünemann wrote:
> >>>> For the Apple notarization process, the whole bundle in its final form
> >>>> has to be signed with the codesign tool.
> >>>> See the discussion here: https://bugs.openjdk.java.net/browse/JDK-8238225
> >>>>
> >>>> This change copies all JDK/JRE files to a temporary directory, which
> >>>> is then passed to the codesign tool. The temporary directory is then
> >>>> used as the base directory for the bundle archive and is getting
> >>>> removed after the archive has been created. This only applies when a
> >>>> valid code signing identity is set and the build type is release.
> >>>>
> >>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8238534
> >>>> WebRev: http://cr.openjdk.java.net/~rschuenemann/wr/2020/8238534-macos_sign_bundles/01/
> >>>>
> >>>> Rene