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 <[email protected]> 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 <[email protected]> > > 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 <[email protected]> > >>> 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 > >>>>> <[email protected]> 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 > >>>>>> <[email protected]> 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
