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

Renames #149

Merged
merged 30 commits into from
Jun 29, 2015
Merged

Renames #149

merged 30 commits into from
Jun 29, 2015

Conversation

joshaber
Copy link
Contributor

Sometimes, apps change their names. Sometimes. Squirrel should support that. We only change the name if:

  1. The user hasn't renamed the existing app.
  2. There's no app already named the new name.

Also, it should be noted that I have no idea what I'm doing.

@@ -413,6 +428,28 @@ - (RACSignal *)verifyBundleAtURL:(NSURL *)bundleURL usingSignature:(SQRLCodeSign

#pragma mark Installation

- (RACSignal *)renameTargetIfNeededWithTargetURL:(NSURL *)targetURL sourceURL:(NSURL *)sourceURL {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe call this renamed target to make it sound less like it's renaming the target on disk?

Also, this method could really use documentation.

//
// Returns a request which can be written to disk for ShipIt to read and
// perform.
- (instancetype)initWithUpdateBundleURL:(NSURL *)updateBundleURL targetBundleURL:(NSURL *)targetBundleURL bundleIdentifier:(NSString *)bundleIdentifier launchAfterInstallation:(BOOL)launchAfterInstallation;
- (instancetype)initWithUpdateBundleURL:(NSURL *)updateBundleURL targetBundleURL:(NSURL *)targetBundleURL bundleIdentifier:(NSString *)bundleIdentifier launchAfterInstallation:(BOOL)launchAfterInstallation allowRename:(BOOL)allowRename;
Copy link
Member

Choose a reason for hiding this comment

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

I think I’d like to call this useUpdateBundleName so that it’s clearer what the final name will be, rename here could imply using or not using rename(2).

Alternatively, we could be even more descriptive and make it a targetPathComponent: NSString? parameter. This would indicate the lastPathComponent on top of the targetBundleURL.stringByRemovingLastPathComponent that we’re requesting the update service to put the update into.

@joshaber
Copy link
Contributor Author

Alright @keithduncan, see how that strikes you.

We still have the launchd problem, but I wanted to make sure this addressed your other concern first.

return [RACSignal return:targetURL];
}

NSString *newAppName = [sourceExecutableName stringByAppendingPathExtension:@"app"];
Copy link
Member

Choose a reason for hiding this comment

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

Can we use sourceURL.lastPathComponent here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the bundle's executable name is more canonical than what the zipped thing is.

Copy link
Member

Choose a reason for hiding this comment

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

@mdiep could you tie break on this one

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know why these would ever be different at this point. It seems a little weird to me that this is even a key in the Info.plist. But given that it's there, I agree that it seems more canonical than the URL.

@keithduncan
Copy link
Member

I’d like to get another 👍 from @jspahrsummers (if still interested in maintaining Squirrel.Mac).

What do you think we should do about the launchd concern?

@joshaber
Copy link
Contributor Author

What do you think we should do about the launchd concern?

Yeah so this is interesting. It seems like Squirrel has always had this problem.

Before:

  1. Start the update.
  2. "Acquire" the bundle to update, which really means move it.
  3. 💥 ShipIt is no longer where we registered it.

Now:

  1. Start the update.
  2. Rename the thing if needed, or "acquire" the bundle.
  3. 💥 ShipIt is no longer where we registered it.

Both before and now, after the first step in the update process there's no going back.

We could try re-registering the job with the new path, but it's not clear to me if we can since it'd be a different bundle than the original one.

But at any rate, it seems like a pre-existing condition so I'd prefer to get this merged and address it later.

@keithduncan
Copy link
Member

Yeah so this is interesting. It seems like Squirrel has always had this problem.

Weird, you’re totally right 😖

I’ll wait to see what @mdiep says about which name to prefer then we can :shipit:

@keithduncan
Copy link
Member

shia_labeouf_delivers_the_most_intense_motivational_speech_of_all_time

keithduncan pushed a commit that referenced this pull request Jun 29, 2015
@keithduncan keithduncan merged commit 6252f3d into master Jun 29, 2015
@keithduncan keithduncan deleted the renames branch June 29, 2015 07:10
@joshaber
Copy link
Contributor Author

@jspahrsummers
Copy link
Member

IIRC launchd copies the ShipIt executable when the job is submitted.

@keithduncan
Copy link
Member

$ launchctl list com.github.GitHub.ShipIt
{
    "StandardOutPath" = "/Users/keith/Library/Application Support/com.github.GitHub.ShipIt/ShipIt_stdout.log";
    "LimitLoadToSessionType" = "Aqua";
    "StandardErrorPath" = "/Users/keith/Library/Application Support/com.github.GitHub.ShipIt/ShipIt_stderr.log";
    "MachServices" = {
        "com.github.GitHub.ShipIt" = mach-port-object;
    };
    "Label" = "com.github.GitHub.ShipIt";
    "TimeOut" = 30;
    "OnDemand" = true;
    "LastExitStatus" = 0;
    "Program" = "/Users/keith/Downloads/GitHub.app/Contents/Frameworks/Squirrel.framework/Resources/ShipIt";
    "ProgramArguments" = (
        "/Users/keith/Downloads/GitHub.app/Contents/Frameworks/Squirrel.framework/Resources/ShipIt";
        "com.github.GitHub.ShipIt";
    );
};

Nope 😕

@jspahrsummers
Copy link
Member

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.

4 participants