RFR: JDK-8238534

Wed Feb 12 14:08:31 UTC 2020

Hi René,

to me your latest webrev looks quite good. You could pull the PRODUCT_TARGETS += $(BUILD_JDK_BUNDLE) and LEGACY_TARGETS += $(BUILD_JRE_BUNDLE) out of the if/else blocks as they are common to both. But I also see a value in keeping them duplicated where they are, so they immediately follow the respective SetupBundleFile call.

Regarding the constant rebuild, I have no idea. Unfortunately I can't test it on my laptop, as I don't have codesigning set up there.

I'll add your patch to our nightly test queue to check it doesn't break anything - but it definitely shouldn't. I can also sponsor the push eventually.

Best regards
Christoph

> -----Original Message-----
> From: build-dev <build-dev-bounces at openjdk.java.net> On Behalf Of René
> Schünemann
> Sent: Mittwoch, 12. Februar 2020 11:55
> To: Erik Joelsson <erik.joelsson at oracle.com>
> Cc: build-dev at openjdk.java.net
> Subject: Re: RFR: JDK-8238534
> 
> Hello Erik,
> 
> thank you for your guidance.
> I have implemented your requested changes.
> 
> Updated WebRev:
> http://cr.openjdk.java.net/~rschuenemann/wr/2020/8238534-
> macos_sign_bundles/03/
> 
> There is one thin I notice though. On my machine the bundles are
> getting resigned and rebuilt every time I call make, even if they
> already exist.
> Do you notice the same behavior?
> 
> Rene
> 
> On Tue, Feb 11, 2020 at 6:57 PM Erik Joelsson <erik.joelsson at oracle.com>
> wrote:
> >
> > On 2020-02-11 01:08, René Schünemann wrote:
> > > 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)
> > That's because you declared the directory as part of FILES in the call
> > to SetupBundleFile.
> > >> 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.
> >
> > I did my suggested changes for the JDK bundle and it built fine. You
> > still need to fix the JRE part.
> >
> > http://cr.openjdk.java.net/~erikj/8238534/webrev.01/
> >
> > >> 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
> > Ah right, ok never mind that comment then.
> > > Can you confirm, that libjli.dylib doesn't need the entitlements?
> >
> > Absolutely. Apple states clearly that entitlements are only for
> > executables, not for libraries. The executable loading the library is
> > responsible for acquiring the necessary entitlements. Setting them on a
> > library has no effect as I understand it.
> >
> > /Erik
> >
> > >> /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