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

[ML] Make the ML native code more ".app"-like on macOS #593

Merged
merged 11 commits into from
Sep 30, 2019

Conversation

droberts195
Copy link
Contributor

@droberts195 droberts195 commented Aug 14, 2019

Successful notarization and acceptance by Gatekeeper on macOS
Catalina requires that the ML programs and dynamic libraries be
arranged into the directory structure used by macOS apps.

This functionality may be required for notarization on
macOS Catalina.
Copy link
Contributor Author

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

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

The values can be extracted from a program built after this change as follows:

bash-3.2$ launchctl plist __TEXT,__info_plist .objs/autodetect 
{
        "CFBundleIdentifier" = "co.elastic.ml-cpp.autodetect";
        "CFBundleDevelopmentRegion" = "English";
        "CFBundleDisplayName" = "autodetect";
        "CFBundleName" = "autodetect";
        "CFBundleVersion" = "8.0.0";
        "CFBundleInfoDictionaryVersion" = "6.0";
};

<key>CFBundleDevelopmentRegion</key>
<string>English</string>
<key>CFBundleIdentifier</key>
<string>co.elastic.ml-cpp.$ML_TARGET</string>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly what to use here beyond co.elastic is not decided. I've used the repo name followed by the program name as a start.

<key>CFBundleDisplayName</key>
<string>$ML_TARGET</string>
<key>CFBundleVersion</key>
<string>$ML_VERSION_NUM</string>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This deliberately excludes qualifiers like beta1 or SNAPSHOT - the field only tolerates digits and periods.

Static analysis files are now named *.xml (after all .plist
files really are .xml files)
Also make sure the identifier only uses valid characters
@droberts195 droberts195 changed the title [ML] Embed an Info.plist in macOS programs [ML] Make the ML native code more ".app"-like on macOS Sep 18, 2019
Copy link
Contributor

@edsavage edsavage left a comment

Choose a reason for hiding this comment

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

LGTM - just spotted one thing which I think is the cause of the CI failure

set_env.sh Outdated
@@ -36,7 +36,7 @@ case `uname` in
if [ -z "$CPP_CROSS_COMPILE" ] ; then
BUNDLE_PLATFORM=linux-x86_64
else
if [ "$CPP_CROSS_COMPILE" = macosx ] ; then
if [ "$CPP_CROSS_COMPILE" = macos ] ; then
Copy link
Contributor

Choose a reason for hiding this comment

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

This will also require a change to CPP_CROSS_COMPILE in dev-tools//docker/macosx_builder/Dockerfile

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are actually quite a few places I'll have to change to switch CPP_CROSS_COMPILE from using macosx to macos, and it will introduce an annoying discrepancy between branches in the Jenkins infrastructure. So I think I'll revert this one place in this PR. Maybe a future PR can do the rename fully.

droberts195 added a commit to droberts195/elasticsearch that referenced this pull request Sep 24, 2019
This is the Java side of elastic/ml-cpp#593
with a fallback so that ml-cpp bundles with either the
new or old directory structure work for the time being.
A few days after merging the C++ changes a followup to
this change will be made that removes the fallback.
When this PR started out we thought we'd be shipping a number
of standalone "command line tools".  But that didn't work with
notarization, so we had to make a more radical change, to become
an app.

(I also removed some dead code related to detecting Linux MUSL.)
@droberts195
Copy link
Contributor Author

I think this is ready now, but this PR must not be merged until a few days after elastic/elasticsearch#47013

droberts195 added a commit to elastic/elasticsearch that referenced this pull request Sep 25, 2019
This is the Java side of elastic/ml-cpp#593
with a fallback so that ml-cpp bundles with either the
new or old directory structure work for the time being.
A few days after merging the C++ changes a followup to
this change will be made that removes the fallback.
droberts195 added a commit to droberts195/elasticsearch that referenced this pull request Sep 25, 2019
This change removes the temporary controller
location fallback introduced in elastic#47013.

Relates elastic/ml-cpp#593
@droberts195
Copy link
Contributor Author

elastic/elasticsearch#47104 needs to be merged a few days after this

droberts195 added a commit to elastic/elasticsearch that referenced this pull request Sep 27, 2019
This is the Java side of elastic/ml-cpp#593
with a fallback so that ml-cpp bundles with either the
new or old directory structure work for the time being.
A few days after merging the C++ changes a followup to
this change will be made that removes the fallback.
@droberts195 droberts195 merged commit b831cf6 into elastic:master Sep 30, 2019
@droberts195 droberts195 deleted the add_info_plist branch September 30, 2019 05:56
droberts195 added a commit to droberts195/ml-cpp that referenced this pull request Sep 30, 2019
Successful notarization and acceptance by Gatekeeper on macOS
Catalina requires that the ML programs and dynamic libraries be
arranged into the directory structure used by macOS apps.

Backport of elastic#593
droberts195 added a commit to droberts195/ml-cpp that referenced this pull request Sep 30, 2019
droberts195 added a commit to droberts195/ml-cpp that referenced this pull request Sep 30, 2019
droberts195 added a commit that referenced this pull request Sep 30, 2019
droberts195 added a commit that referenced this pull request Sep 30, 2019
Successful notarization and acceptance by Gatekeeper on macOS
Catalina requires that the ML programs and dynamic libraries be
arranged into the directory structure used by macOS apps.

Backport of #593
droberts195 added a commit to droberts195/ml-cpp that referenced this pull request Sep 30, 2019
Successful notarization and acceptance by Gatekeeper on macOS
Catalina requires that the ML programs and dynamic libraries be
arranged into the directory structure used by macOS apps.

Backport of elastic#593
droberts195 added a commit to elastic/elasticsearch that referenced this pull request Oct 1, 2019
This is the Java side of elastic/ml-cpp#593
with a fallback so that ml-cpp bundles with either the
new or old directory structure work for the time being.
A few days after merging the C++ changes a followup to
this change will be made that removes the fallback.
droberts195 added a commit that referenced this pull request Oct 1, 2019
Successful notarization and acceptance by Gatekeeper on macOS
Catalina requires that the ML programs and dynamic libraries be
arranged into the directory structure used by macOS apps.

Backport of #593
droberts195 added a commit to elastic/elasticsearch that referenced this pull request Oct 4, 2019
This change removes the temporary controller
location fallback introduced in #47013.

Relates elastic/ml-cpp#593
droberts195 added a commit to elastic/elasticsearch that referenced this pull request Oct 4, 2019
This change removes the temporary controller
location fallback introduced in #47013.

Relates elastic/ml-cpp#593
droberts195 added a commit to elastic/elasticsearch that referenced this pull request Oct 4, 2019
This change removes the temporary controller
location fallback introduced in #47013.

Relates elastic/ml-cpp#593
droberts195 added a commit to elastic/elasticsearch that referenced this pull request Nov 1, 2019
This is the Java side of elastic/ml-cpp#593
with a fallback so that ml-cpp bundles with either the
new or old directory structure work for the time being.
A few days after merging the C++ changes a followup to
this change will be made that removes the fallback.
droberts195 added a commit to droberts195/ml-cpp that referenced this pull request Nov 1, 2019
Successful notarization and acceptance by Gatekeeper on macOS
Catalina requires that the ML programs and dynamic libraries be
arranged into the directory structure used by macOS apps.

Backport of elastic#593
droberts195 added a commit that referenced this pull request Nov 1, 2019
Successful notarization and acceptance by Gatekeeper on macOS
Catalina requires that the ML programs and dynamic libraries be
arranged into the directory structure used by macOS apps.

Backport of #593
droberts195 added a commit to elastic/elasticsearch that referenced this pull request Nov 1, 2019
This change removes the temporary controller
location fallback introduced in #47013.

Relates elastic/ml-cpp#593
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants