RFR: JDK-8238534

Thu Feb 13 17:17:33 UTC 2020

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