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

Use a consistent app naming strategy across non-stable channels #17680

Merged
merged 4 commits into from Jul 12, 2018

Conversation

Projects
None yet
2 participants
@daviwil
Copy link
Member

daviwil commented Jul 12, 2018

Description of the Change

This change makes the naming of the Atom application consistent across all non-stable channels (Beta, Nightly, Dev) to simplify the logic of scripts which consume these channels (like atom/ci). This builds on some changes I made in PR #17538 which generalize some of the app naming code we were using to take other non-stable channels into account (like Nightly).

The only real impact this change has is that the .app file that's created from a master build on macOS will now be called Atom Dev.app. I'm bringing this to general awareness in case anyone has any personal scripts for regularly building and using Atom from master.

This issue originally came up when @smashwilson found that the atom/ci script didn't properly find the Atom Dev build it unpacks on Linux because the folder names have changed. While trying to fix that script, I realized that the Windows ZIP contained a folder called "Atom null" because of a bug in the code. This change fixes it for all cases.

Alternate Designs

Special case the dev channel to be called "Atom" and not "Atom Dev". I'm fine with doing this if folks think it's a better approach.

Why Should This Be In Core?

It's part of the Atom build process.

Benefits

Consistent naming for non-stable channel builds, easier to predict what the app name will be without special casing.

Possible Drawbacks

Breaking some existing workflows, but not terribly likely and easy to fix if so.

Verification Process

  • Let CI complete and verify artifacts that are generated to make sure "Atom Dev" is being used as the app name on all 3 platforms:
    • Windows
    • macOS
    • Linux

@daviwil daviwil requested review from smashwilson and maxbrunsfeld Jul 12, 2018

@daviwil

This comment has been minimized.

Copy link
Member Author

daviwil commented Jul 12, 2018

image

@smashwilson
Copy link
Member

smashwilson left a comment

I'm 👍 on this because I'm a sucker for consistency across platforms and across channels. Being able to install stable, beta, and dev side-by-side on macOS would be really nice too. Although it might warrant an @here in the maintainer's chat if this is merged 😄

...what I might do is use a bash alias to map atom to atom-dev on my Mac, btw, just to keep my muscle memory working 😉

function getAppName (channel) {
return channel === 'stable'
? 'Atom'
: `Atom ${process.env.ATOM_CHANNEL_DISPLAY_NAME || channel.charAt(0).toUpperCase() + channel.slice(1)}`

This comment has been minimized.

@smashwilson

smashwilson Jul 12, 2018

Member

Custom display name support, eh? Time to build "Atom 3000"

This comment has been minimized.

@daviwil

daviwil Jul 12, 2018

Author Member

Haha, yeah, just in case we want some one-off release branches :)

@daviwil

This comment has been minimized.

Copy link
Member Author

daviwil commented Jul 12, 2018

One thing that I kept was the behavior of how Atom installs atom/apm commands on macOS. On dev I'm still using atom rather than atom-dev. I think when I tried to change that it broke some CI tests, but if we want to have a fully consistent naming experience, it's probably worth changing this.

Relevant code:

getCommandNameForChannel (commandName) {
let channelMatch = this.appVersion.match(/beta|nightly/)
let channel = channelMatch ? channelMatch[0] : ''
switch (channel) {
case 'beta':
return `${commandName}-beta`
case 'nightly':
return `${commandName}-nightly`
default:
return commandName
}
}
installAtomCommand (askForPrivilege, callback) {
this.installCommand(
path.join(this.getResourcesDirectory(), 'app', 'atom.sh'),
this.getCommandNameForChannel('atom'),
askForPrivilege,
callback
)
}

@daviwil

This comment has been minimized.

Copy link
Member Author

daviwil commented Jul 12, 2018

Which reminds me, I need to fix atom.sh for this too!

@daviwil

This comment has been minimized.

Copy link
Member Author

daviwil commented Jul 12, 2018

Aaaand now I need to update the GitHub package to use Atom Dev Helper.app instead of Atom Helper.app 🤦‍♂️

@daviwil daviwil merged commit 8c3e36d into master Jul 12, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@daviwil daviwil deleted the dw-consistent-app-name branch Jul 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.