Great. I'll push it tomorrow. /Christoph
> -----Original Message----- > From: Erik Joelsson <[email protected]> > Sent: Donnerstag, 13. Februar 2020 18:18 > To: René Schünemann <[email protected]>; Langer, Christoph > <[email protected]> > Cc: [email protected] > 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 <[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
