RFR: JDK-8238534

Tue Feb 11 17:57:09 UTC 2020

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