Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add signing for mac osx. #6748

Merged
merged 55 commits into from
Aug 23, 2020
Merged

Add signing for mac osx. #6748

merged 55 commits into from
Aug 23, 2020

Conversation

Siedlerchr
Copy link
Member

@Siedlerchr Siedlerchr commented Aug 9, 2020

First part of fixing #6154

  • Sign and notarize dmg
  • Sign and notarize pkg

Edit: We maybe should run the signing only on master?

  • Change in CHANGELOG.md described (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

@Siedlerchr
Copy link
Member Author

Siedlerchr commented Aug 9, 2020

windows seems to fail:

Running [C:\Program Files (x86)\WiX Toolset v3.11\bin\light.exe, -nologo, -spdb, -ext, WixUtilExtension, -out, D:\a\jabref\jabref\build\distribution\JabRef-5.1.30845.msi, -sice:ICE27, -ext, WixUIExtension, -loc, D:\a\jabref\jabref\build\installer\config\MsiInstallerStrings_en.wxl, -b, D:\a\jabref\jabref\build\installer\config, D:\a\jabref\jabref\build\installer\wixobj\main.wixobj, D:\a\jabref\jabref\build\installer\wixobj\bundle.wixobj]in D:\a\jabref\jabref\build\installer\images\win-msi.image\JabRef
D:\a\jabref\jabref\build\installer\config\main.wxs(85) : error LGHT0091 : Duplicate symbol 'Property:ARPPRODUCTICON' found. This typically means that an Id is duplicated. Check to make sure all your identifiers of a given type (File, Component, Feature) are unique.
D:\a\jabref\jabref\build\installer\config\main.wxs(147) : error LGHT0092 : Location of symbol related to previous error.
java.io.IOException: Command [C:\Program Files (x86)\WiX Toolset v3.11\bin\light.exe, -nologo, -spdb, -ext, WixUtilExtension, -out, D:\a\jabref\jabref\build\distribution\JabRef-5.1.30845.msi, -sice:ICE27, -ext, WixUIExtension, -loc, D:\a\jabref\jabref\build\installer\config\MsiInstallerStrings_en.wxl, -b, D:\a\jabref\jabref\build\installer\config, D:\a\jabref\jabref\build\installer\wixobj\main.wixobj, D:\a\jabref\jabref\build\installer\wixobj\bundle.wixobj]in D:\a\jabref\jabref\build\installer\images\win-msi.image\JabRef exited with 92 

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 9, 2020
@Siedlerchr Siedlerchr changed the title [WIP] Test macsign Add signing for mac osx. Aug 9, 2020
@Siedlerchr
Copy link
Member Author

@Gena928 @k3KAW8Pnf7mkmdSMPHz27
Can you please test this version here on mac?
The pgk and the dmg should now be signed correctly:
https://builds.jabref.org/pull/6748/merge/

(Just want to make sure that it works on your machines as well, because I have the certs already imported)

@Gena928
Copy link
Contributor

Gena928 commented Aug 9, 2020

Good day,
I was able to install JabRef using DMG & PKG files.
Unfortunately icons in dmg file are not in the center in the install frame.
DMG install screen

But this looks much better now.

@Siedlerchr
Copy link
Member Author

@Gena928 Yeah! glad to hear that it works now fine.
Regarding the icons, I think we need to copy over that script and adapt the sizes:

  if theFilePath is "DEPLOY_INSTALL_LOCATION" then
      -- Position install location
      set position of item theFile of theWindow to {390, 130}
    else
      -- Position application or runtime
      set position of item theFile of theWindow to {120, 130}
    end if

https://github.com/openjdk/jdk/blob/master/src/jdk.incubator.jpackage/macosx/classes/jdk/incubator/jpackage/internal/resources/DMGsetup.scpt

@Gena928
Copy link
Contributor

Gena928 commented Aug 9, 2020

OK, let me know if you need my testing.
But I think the result is very good now. Nice work!

@k3KAW8Pnf7mkmdSMPHz27
Copy link
Sponsor Member

k3KAW8Pnf7mkmdSMPHz27 commented Aug 10, 2020

Awesome that you are working on this!

.pkg works as it should (signed and not notarized), but I don't know how to verify the signature of the .dmg.

spctl -t open --assess --verbose JabRef-5.1.dmg and spctl -a -t open --context context:primary-signature -v JabRef-5.1.dmg gives me

JabRef-5.1.dmg: rejected
source=Insufficient Context

and

JabRef-5.1.dmg: rejected
source=no usable signature

but I am guessing this is as it should be?

@Siedlerchr
Copy link
Member Author

Siedlerchr commented Aug 10, 2020

Thanks for the feedback, glad that the signing at least works so that no warning is displayed.
Notarization seems to be the next step after the build.
Requires additional handling, will look into it.
It's really confusing figuring out the certificate stuff

@Siedlerchr Siedlerchr closed this Aug 10, 2020
@Siedlerchr Siedlerchr reopened this Aug 10, 2020
@k3KAW8Pnf7mkmdSMPHz27
Copy link
Sponsor Member

k3KAW8Pnf7mkmdSMPHz27 commented Aug 11, 2020

I get no error message related to signing from either the dmg or pkg.

A minor "nitpick" is that the pkg displays a warning for it not being notarized ("... can't be opened because Apple cannot check it for malicious software.")

@Siedlerchr
Copy link
Member Author

Yeah, I noticed that too. I will try to setup the notarization workflow next. Then aftewards ist should be also possible to add it to the app store. Probably reqires another different certificate signing stuff

@@ -55,14 +56,29 @@ jobs:
- name: Set up JDK
uses: actions/setup-java@v1
with:
java-version: 14
java-version: 15-ea
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to upgrade to 15?

Copy link
Member Author

@Siedlerchr Siedlerchr Aug 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.github/workflows/deployment.yml Outdated Show resolved Hide resolved
buildres/windows/JabRef-post-image.wsf Show resolved Hide resolved
@tobiasdiez
Copy link
Member

Concerning the installation screen, the easiest option is probably to change the image.

* upstream/master:
  Improve handling of annotation processor in eclipse (#6749)
  New Crowdin updates (#6741)
  Bump appleboy/ssh-action from v0.1.2 to v0.1.3 (#6752)
  Bump WyriHaximus/github-action-wait-for-status from 0.1.0 to v1.1 (#6751)
  Squashed 'src/main/resources/csl-styles/' changes from 827b986..d23a3ab
regenerate all icons
Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix #6748 (comment).

@Siedlerchr
Copy link
Member Author

I think @Gena928 did the initial design of the install dialog. Need to find the original PR.
Personally, I don't see a big problem with using our modified version.

* upstream/master:
  Hint message for TextFieldTableCells commit JFX bug (#6772)
  Add new authors
* upstream/macsign:
  Use Major.Minor version in the file name
  Remove empty lines
  Fix comment char
  Refine comments
  Fix indent
  Fix indent to ease reviewing
* upstream/master:
  Streamline new library command (#6773)
  [6574] Added support for biblatex-software (#6747)
  Updated the eclipse doc with a note and image on how to add the run/debug configuration. (#6776)
  New Crowdin updates (#6774)
* upstream/master:
  Streamline new library command (#6773)
  [6574] Added support for biblatex-software (#6747)
  Updated the eclipse doc with a note and image on how to add the run/debug configuration. (#6776)
  New Crowdin updates (#6774)
* upstream/master:
  fix checkstyle in adr
* upstream/macsign:
  try with other jdk for linux and win
@Siedlerchr
Copy link
Member Author

Siedlerchr commented Aug 23, 2020

@k3KAW8Pnf7mkmdSMPHz27 Both the installer and the dmg should now be signed and notarized. Can you confirm that?
You can try the latest master when it's ready

@JabRef JabRef deleted a comment from github-actions bot Aug 23, 2020
@koppor koppor added this to the v5.1 milestone Aug 23, 2020
@koppor koppor merged commit 3310a23 into master Aug 23, 2020
@koppor koppor deleted the macsign branch August 23, 2020 18:55
@tobiasdiez
Copy link
Member

@Siedlerchr please also add this PR in #5226 as I guess most of these changes can be reverted as soon as the corresponding upstream java bugs are fixed.

Siedlerchr added a commit that referenced this pull request Aug 24, 2020
* upstream/master:
  Bump WyriHaximus/github-action-wait-for-status from v1.1 to v1.1.2 (#6786)
  Bump postgresql from 42.2.15 to 42.2.16 (#6785)
  Bump unirest-java from 3.9.00 to 3.10.00 (#6784)
  Bump org.beryx.jlink from 2.21.2 to 2.21.3 (#6783)
  Bump mockito-core from 3.5.0 to 3.5.5 (#6782)
  Fixed forgotten biblatex test with biblatex-software type definitions (#6780)
  Enable SpringerFetcherTest running on CI using a dedicated key (#6729)
  Add missing authors
  New Crowdin updates (#6778)
  Add signing for mac osx. (#6748)
  Linkfix
  Squashed 'src/main/resources/csl-styles/' changes from 0895562..469deb6
@stefan-kolb stefan-kolb mentioned this pull request Jan 8, 2021
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants