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

Packaging for macOS #28

Closed
wants to merge 15 commits into from
Closed

Conversation

mook-as
Copy link
Contributor

@mook-as mook-as commented Dec 17, 2020

This implements packaging for macOS; we can get a zip file with the .app out at the other end. This fixes #9.

I've tried to make the individual commits individually reviewable, so doing commit-by-commit might be easier.

I'm not entirely happy about having a new app:// protocol handler, but electron gets unhappy about loading things off file:// — it thinks it's insecure. I'd be happy to switch it to something else if it works.

New run scripts:

  • npm run package — output the packaged .app in out/
  • npm run make — output distributable .zip in out/

(Those are the default names from electron-forge.)

This avoids issues with require('electron') using fs.readSync() breaking
when we end up packaging it.
These are the defaults for electron-forge.
When packaged, we will not be able to load file:// URLs (currently not an
issue because the dev workflow includes running a local HTTP server).  So
we will need an additional protocol for loading resources.
This will be used to package the application.
We will, at the short term at least, not target Linux.
This will be needed later as we get more complex electron-forge
configuration.
This was making it hard for webpack to figure out dependencies.
When packaged, we can't just read files off the filesystem (as the layout
will be different); however, we don't really need to, as we can just import
it as a module, at which point webpack can find the file ahead of time.
This is needed as the path will be different when the application is
packaged.
Because file:// doesn't really work due to security restrictions.
This will be needed for Vue.js integration
This does all the fixups necessary to make the build work correctly.
This is useful to have artifacts to test locally if necessary.
@mattfarina
Copy link
Contributor

I tried to run the packaged app and it didn't work. The specific error was 65 from minikube which means the provider (hyperkit) is not found.

@jandubois
Copy link
Member

The specific error was 65 from minikube which means the provider (hyperkit) is not found.

@mattfarina Is this with a new install of minikube? The first time minikube uses the hyperkit driver it has to fix permissions to make it possible to run it with setuid root:

$ ls -l ~/.minikube/bin/docker-machine-driver-hyperkit
-rwsr-xr-x  1 root  wheel  11428724  2 Jun  2020 /Users/jan/.minikube/bin/docker-machine-driver-hyperkit

When this happens minikube shells out to sudo, which will ask for a password (or fail if it has no terminal to ask for one):

https://github.com/kubernetes/minikube/blob/master/pkg/minikube/driver/install.go#L84-L94

Note also that hyperkit itself (not the driver) must be installed separately, either by having docker installed, or by installing it via homebrew:

https://github.com/kubernetes/minikube/blob/master/pkg/minikube/registry/drvs/hyperkit/hyperkit.go#L86-L100

See also kubernetes/minikube#5478

Of course eventually RD will have to install it itself if not already present. Not sure if it also must be installed as root; at least docker does do it:

$ ls -l $(which hyperkit)
lrwxr-xr-x  1 root  admin  67 19 Oct 17:50 /usr/local/bin/hyperkit -> /Applications/Docker.app/Contents/Resources/bin/com.docker.hyperkit

@mattfarina
Copy link
Contributor

@jandubois In the non-packaged application, if you don't have hyperkit installed it should handle that case for you without you needing to do anything. It will install hyperkit and handle the permissions. hyperkit is stored in the Library with the other data.

What changed about packaging the app that this no longer happens?

@jandubois
Copy link
Member

@mattfarina Sorry, I should have checked the latest RD sources to see that it already does the permissions thing...

What changed about packaging the app that this no longer happens?

It is not the packaging; it is the way the app is started. Everything works when you start if from your controlling terminal:

open out/rancher-desktop-darwin-x64/rancher-desktop.app

I can reproduce the error when launching it via Finder, in which case I believe launchd is tasked to start it on the users behalf. This may be related to some permissions thing, where the app inherits permissions from your Terminal or iTerm console, but not from Finder, but I don't really know. Somebody will have to research it. 😄

@jandubois
Copy link
Member

if you don't have hyperkit installed it should handle that case for you without you needing to do anything

This does not seem to be correct; I don't see any place where RD installs hyperkit itself. As I pointed out above, there are 2 pieces to it:

  1. The docker-machine-driver-hyperkit driver. minikube maintains a fork of it and bundles it by itself. It just needs to be able to set the permissions. It is installed in ~/.minikube/bin.

  2. The hyperkit application itself. It is installed by Docker Desktop, or via brew install hyperkit. minikube doesn't bundle or install it; it expects it to be already present, just like it doesn't install VirtualBox for you if you want to use the docker-machine-driver-virtualbox driver: https://minikube.sigs.k8s.io/docs/drivers/hyperkit/. Docker Desktop installs a symlink to it in /usr/local/bin/hyperkit.

This should maybe be a separate issue from packaging the app.

@mattfarina
Copy link
Contributor

@jandubois it automatically installs it. See kubernetes/minikube#5354 for the change that implemented that.

@mattfarina
Copy link
Contributor

@jandubois I stand corrected. That's just the driver that's needed. Yes, we need to make sure hyperkit is installed an included.

What I don't understand is why I have hyperkit and got that error with the packaged app. That's more my curiosity, now.

@jandubois
Copy link
Member

What I don't understand is why I have hyperkit and got that error with the packaged app. That's more my curiosity, now.

@mattfarina Do you keep the stdout/stderr from minikube somewhere?

Also, is 65 the exit code of the minikube process, or something internally from the NodeJS library?

@jandubois
Copy link
Member

What I don't understand is why I have hyperkit and got that error with the packaged app. That's more my curiosity, now.

@mattfarina The issue is that PATH does not include /usr/local/bin when the app is being launched by launchd (none of the shell .profile scripts will have executed before launch).

We could configure the PATH in the Info.plist of the app, but I think it would be better to setup the PATH at runtime before launching minikube. We'll need to look for hyperkit first anyways, to be able to offer to install it.

I assume we just use the hardcoded location /usr/local/bin/hyperkit because we won't have access to the user's customized PATH setting.

@jandubois
Copy link
Member

@mattfarina Do you keep the stdout/stderr from minikube somewhere?

Here is a command to view the logs from the last minikube command executed (add -v 2 to the minikube command to get more verbose logs):

less $(find $TMPDIR -mtime -1 -type f -name "*minikube*INFO*" | sort | tail -1)

It does show the PATH error:

I1223 14:01:21.758714    3269 start.go:691] status for hyperkit: {Installed:false Healthy:false Running:false NeedsImprovement:false Error:exec: "hyperkit": executable file not found in $PATH Fix:Run 'brew install hyperkit' Doc:https://minikube.sigs.k8s.io/docs/reference/drivers/hyperkit/}
I1223 14:01:21.811486    3269 out.go:109]
W1223 14:01:21.811589    3269 out.go:145] * Exiting due to PROVIDER_HYPERKIT_NOT_FOUND: The 'hyperkit' provider was not found: exec: "hyperkit": executable file not found in $PATH
W1223 14:01:21.811662    3269 out.go:145] * Suggestion: Run 'brew install hyperkit'
W1223 14:01:21.811704    3269 out.go:145] * Documentation: https://minikube.sigs.k8s.io/docs/reference/drivers/hyperkit/

macOS automatically deletes temp files older than 3 days, so copy out the files if you need to reference them for longer.

@jandubois
Copy link
Member

we won't have access to the user's customized PATH setting.

Well, I realized that in Emacs I use https://github.com/purcell/exec-path-from-shell to make sure I have my regular environment inside the Emacs shell, and then found https://github.com/sindresorhus/shell-path.

I haven't looked at it, nor searched if there are alternative implementations, but just an idea in case we don't want to hard-code the location. I still think hard-coding is fine. If we bundle the binary, we might even want to always use our internal version, to make sure it is compatible.

@mattfarina
Copy link
Contributor

Closing as we have #34 as a replacement.

@mattfarina mattfarina closed this Jan 6, 2021
@mook-as mook-as deleted the packaging-macos branch January 8, 2021 17:13
jandubois pushed a commit to jandubois/rancher-desktop that referenced this pull request Jun 28, 2024
…-action-lint-issue

Adds json tag to port mapping types
jandubois pushed a commit to jandubois/rancher-desktop that referenced this pull request Jun 28, 2024
…-action-lint-issue

Adds json tag to port mapping types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Package as a Mac app
3 participants