RFR: JDK-8238534

Thu Feb 13 09:17:17 UTC 2020

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