RFR: JDK-8238534

Thu Feb 13 21:33:27 UTC 2020

Great. I'll push it tomorrow.

/Christoph

> -----Original Message-----
> From: Erik Joelsson <erik.joelsson at oracle.com>
> Sent: Donnerstag, 13. Februar 2020 18:18
> To: René Schünemann <rene.schuenemann at gmail.com>; Langer, Christoph
> <christoph.langer at sap.com>
> Cc: build-dev at openjdk.java.net
> Subject: Re: RFR: JDK-8238534
> 
> Looks good.
> 
> /Erik
> 
> On 2020-02-13 01:17, René Schünemann wrote:
> > Hi Erik and Christoph,
> >
> > thank you for your reviews.
> > The added touch works for me too. I have adapted the suggested
> whitespace fixes.
> >
> > Here is the updated WebRev:
> > http://cr.openjdk.java.net/~rschuenemann/wr/2020/8238534-
> macos_sign_bundles/04/
> >
> > Rene
> >
> > On Wed, Feb 12, 2020 at 8:09 PM Erik Joelsson <erik.joelsson at oracle.com>
> wrote:
> >> Hello Rene,
> >>
> >> On 2020-02-12 02:54, René Schünemann wrote:
> >>> 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/
> >> Much better. I still have some style issues [1]. The recipe lines for
> >> the signing rule all have extra spaces, which is not something we do.
> >> Also there is no continuation indent for the long command line.
> >>
> >> I would also recommend against redirecting stderr as that will hide any
> >> error message from codesign.
> >>
> >>> 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?
> >> Yes, it seems the CodeResources file does not get timestamped with a
> >> newer date than the source files. Adding a touch fixes it for me. (We
> >> also use that in our closed implementation)
> >>
> >> Here is a webrev with my suggested whitespace fixes and the touch.
> >>
> >> http://cr.openjdk.java.net/~erikj/8238534/webrev.02/
> >>
> >> [1] http://openjdk.java.net/groups/build/doc/code-conventions.html
> >>
> >> /Erik
> >>
> >>> 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