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

[OSX] rename Bitcoin-Qt.app to Bitcoin-Core.app #6116

Merged
merged 2 commits into from May 20, 2015

Conversation

Projects
None yet
5 participants
@jonasschnelli
Member

jonasschnelli commented May 7, 2015

Bitcoin-Qt is a strange name for a GUI application. IMO we could try to slowly remove the "-Qt" part and try to be consistent with "Bitcoin-Core". The main windows title is already saying that we are running "Bitcoin Core".

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj May 7, 2015

Member

This obviously needs to be done at some point.
However there are some ancillary issues, such as auto-start-on-startup, that need to be considered.
As well as the installer (making sure the old executable, as well as links to it, are deleted on upgrade), as well as making sure QSettings still end up in the same place or are migrated.
These may not be an issue on mac though.

BTW: The eventual DMG in releases is already called bitcoin-0.10.1-osx.dmg. No qt in there?

Member

laanwj commented May 7, 2015

This obviously needs to be done at some point.
However there are some ancillary issues, such as auto-start-on-startup, that need to be considered.
As well as the installer (making sure the old executable, as well as links to it, are deleted on upgrade), as well as making sure QSettings still end up in the same place or are migrated.
These may not be an issue on mac though.

BTW: The eventual DMG in releases is already called bitcoin-0.10.1-osx.dmg. No qt in there?

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 7, 2015

Member

The .dmg name is correct. But when you open the disk image you will find an application called "Bitcoin-Qt".

I'll wait now for other opinions and if there are no reasons to stay with Bitcoin-Qt, i think it then makes sense to continue and complete this PR.

Member

jonasschnelli commented May 7, 2015

The .dmg name is correct. But when you open the disk image you will find an application called "Bitcoin-Qt".

I'll wait now for other opinions and if there are no reasons to stay with Bitcoin-Qt, i think it then makes sense to continue and complete this PR.

@fanquake

This comment has been minimized.

Show comment
Hide comment
@fanquake

fanquake May 7, 2015

Member

Concept ACK

Member

fanquake commented May 7, 2015

Concept ACK

@jonasschnelli jonasschnelli changed the title from [Mac only] rename Bitcoin-Qt.app to Bitcoin-Core.app to rename Bitcoin-Qt.app to Bitcoin-Core.app May 7, 2015

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 7, 2015

Member

Tested on windows regarding persisted settings (QSettings) and autostart. Also tested nsi installer.

If we continue with this, somebody with binary signing capabilities (win/mac) should check how a name change would affect signing.
Process of updating Bitcoin-Qt is also something which should be tested.

Member

jonasschnelli commented May 7, 2015

Tested on windows regarding persisted settings (QSettings) and autostart. Also tested nsi installer.

If we continue with this, somebody with binary signing capabilities (win/mac) should check how a name change would affect signing.
Process of updating Bitcoin-Qt is also something which should be tested.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 7, 2015

Member

A gitian build (with logs to check) can be downloaded here: https://builds.jonasschnelli.ch/pulls/6116/

Member

jonasschnelli commented May 7, 2015

A gitian build (with logs to check) can be downloaded here: https://builds.jonasschnelli.ch/pulls/6116/

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 7, 2015

Member

After discussion this on IRC we came to the point to not changing the actual executable names.
Rolled this back so this PR will only change the OSX Bundle Name (executable and signing entities stays the same).

Member

jonasschnelli commented May 7, 2015

After discussion this on IRC we came to the point to not changing the actual executable names.
Rolled this back so this PR will only change the OSX Bundle Name (executable and signing entities stays the same).

@jonasschnelli jonasschnelli changed the title from rename Bitcoin-Qt.app to Bitcoin-Core.app to [OSX] rename Bitcoin-Qt.app to Bitcoin-Core.app May 7, 2015

@Michagogo

This comment has been minimized.

Show comment
Hide comment
@Michagogo

Michagogo May 7, 2015

Contributor

Why Bitcoin-Core.app and not Bitcoin\ Core.app? I'm fairly certain .app names can have spaces, and I feel like this is similar to the start menu entry/shortcut change, which does include a space.

Contributor

Michagogo commented May 7, 2015

Why Bitcoin-Core.app and not Bitcoin\ Core.app? I'm fairly certain .app names can have spaces, and I feel like this is similar to the start menu entry/shortcut change, which does include a space.

@Michagogo

This comment has been minimized.

Show comment
Hide comment
@Michagogo

Michagogo May 7, 2015

Contributor

And re: the installer, I don't know that that can really be done, considering the installation process for OS X is "drag the .app folder into /Applications". (I'm assuming we don't want to use a .[m]pkg)

Contributor

Michagogo commented May 7, 2015

And re: the installer, I don't know that that can really be done, considering the installation process for OS X is "drag the .app folder into /Applications". (I'm assuming we don't want to use a .[m]pkg)

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 7, 2015

Member

"Bitcoin\ Core.app" sounds good to me. I'll change it that way. The installation process will be manually (user need to delete Bitcoin-Qt.app before or after he installed/copied the new Bitcoin-Core.app. I think we don't want to go for a .pkg (uncommon on OSX).

Member

jonasschnelli commented May 7, 2015

"Bitcoin\ Core.app" sounds good to me. I'll change it that way. The installation process will be manually (user need to delete Bitcoin-Qt.app before or after he installed/copied the new Bitcoin-Core.app. I think we don't want to go for a .pkg (uncommon on OSX).

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 11, 2015

Member

Changing it to "Bitcoin\ Core.app" seems not to be possible. Somehow Automake and co. are not ready to build binaries with names containing whitespaces. I think we should keep it Bitcoin-Core.app (as this PR like to change it to) to avoid heavy-maintainable hacks.

Member

jonasschnelli commented May 11, 2015

Changing it to "Bitcoin\ Core.app" seems not to be possible. Somehow Automake and co. are not ready to build binaries with names containing whitespaces. I think we should keep it Bitcoin-Core.app (as this PR like to change it to) to avoid heavy-maintainable hacks.

@Michagogo

This comment has been minimized.

Show comment
Hide comment
@Michagogo

Michagogo May 11, 2015

Contributor

If it can't be done without a hack, then it's not such a huge deal... It's
just that from the limited experience I have with Macs, having multiple
words separated by something other than spaces is non-standard and I
figured it would look weird, kinda like how the standard installer is a
.dmg with an .app and a shortcut to /Applications. (Also, nit: "Bitcoin
Core.app" is redundant, since you don't need \ in quotes. (And if I made
that mistake... oops.))

On Monday, May 11, 2015, Jonas Schnelli notifications@github.com wrote:

Changing it to "Bitcoin\ Core.app" seems not to be possible. Somehow
Automake and co. are not ready to build binaries with names containing
whitespaces. I think we should keep it Bitcoin-Core.app (as this PR like to
change it to) to avoid heavy-maintainable hacks.


Reply to this email directly or view it on GitHub
#6116 (comment).

Contributor

Michagogo commented May 11, 2015

If it can't be done without a hack, then it's not such a huge deal... It's
just that from the limited experience I have with Macs, having multiple
words separated by something other than spaces is non-standard and I
figured it would look weird, kinda like how the standard installer is a
.dmg with an .app and a shortcut to /Applications. (Also, nit: "Bitcoin
Core.app" is redundant, since you don't need \ in quotes. (And if I made
that mistake... oops.))

On Monday, May 11, 2015, Jonas Schnelli notifications@github.com wrote:

Changing it to "Bitcoin\ Core.app" seems not to be possible. Somehow
Automake and co. are not ready to build binaries with names containing
whitespaces. I think we should keep it Bitcoin-Core.app (as this PR like to
change it to) to avoid heavy-maintainable hacks.


Reply to this email directly or view it on GitHub
#6116 (comment).

@laanwj laanwj added the macOS label May 12, 2015

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj May 12, 2015

Member

Changing it to "Bitcoin\ Core.app" seems not to be possible. Somehow Automake and co. are not ready to build binaries with names containing whitespaces.

That's really sad, if true. It's not just a matter of escaping in the right way?

Error: Invalid or corrupt jarfile /home/travis/build/bitcoin/bitcoin/depends/x86_64-unknown-linux-gnu/native/share/BitcoindComparisonTool_jar/BitcoindComparisonTool.jar

Strange error, doesn't seem to be caused by any change here. WIll retrigger Travis.

Member

laanwj commented May 12, 2015

Changing it to "Bitcoin\ Core.app" seems not to be possible. Somehow Automake and co. are not ready to build binaries with names containing whitespaces.

That's really sad, if true. It's not just a matter of escaping in the right way?

Error: Invalid or corrupt jarfile /home/travis/build/bitcoin/bitcoin/depends/x86_64-unknown-linux-gnu/native/share/BitcoindComparisonTool_jar/BitcoindComparisonTool.jar

Strange error, doesn't seem to be caused by any change here. WIll retrigger Travis.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 12, 2015

Member

@laanwj:
After trying to use whitespace with escape sequences like '\ ' or taking the whole app name into string quotes and also did a google research on how autotools and whitespace can live together i found out that this is not just a trivial escaping thing.
I try now rename the .app file during the make process before adding it to the disk image. This could be a quick and stable workaround.

And yes. The travis issue is strange and i can't cure it from my side. Other PRs work fine. I force pushed / rebased this PR some times but the .jar issue is still there.I don't get this.

Member

jonasschnelli commented May 12, 2015

@laanwj:
After trying to use whitespace with escape sequences like '\ ' or taking the whole app name into string quotes and also did a google research on how autotools and whitespace can live together i found out that this is not just a trivial escaping thing.
I try now rename the .app file during the make process before adding it to the disk image. This could be a quick and stable workaround.

And yes. The travis issue is strange and i can't cure it from my side. Other PRs work fine. I force pushed / rebased this PR some times but the .jar issue is still there.I don't get this.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 12, 2015

Member

Added another commit: now Bitcoin-Core.app (folder) gets renamed to Bitcoin Core.app during the disk image process.
Just started another PR gitian build to test this more authentic: https://builds.jonasschnelli.ch/pulls/6116/

Member

jonasschnelli commented May 12, 2015

Added another commit: now Bitcoin-Core.app (folder) gets renamed to Bitcoin Core.app during the disk image process.
Just started another PR gitian build to test this more authentic: https://builds.jonasschnelli.ch/pulls/6116/

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 12, 2015

Member

This is not ready yet. Gitian build process bypassed macdeployqtplus. It first need a fix for the DMG name as well as for the missing background picture.

Member

jonasschnelli commented May 12, 2015

This is not ready yet. Gitian build process bypassed macdeployqtplus. It first need a fix for the DMG name as well as for the missing background picture.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 12, 2015

Member

Solved. The window title of the disk image is now "Bitcoin-Core" (instead of Bitcoin-Qt). The app name inside the disk image is now "Bitcoin Core" (with whitespace between "Bitcoin" and "Core"). Tested over gitian.

Ready to ACK and Merge. :)

Member

jonasschnelli commented May 12, 2015

Solved. The window title of the disk image is now "Bitcoin-Core" (instead of Bitcoin-Qt). The app name inside the disk image is now "Bitcoin Core" (with whitespace between "Bitcoin" and "Core"). Tested over gitian.

Ready to ACK and Merge. :)

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 13, 2015

Member

The travis issues seems to be connected to the JAR comparison tool. I can't see any connection to this PR.

Member

jonasschnelli commented May 13, 2015

The travis issues seems to be connected to the JAR comparison tool. I can't see any connection to this PR.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 13, 2015

Member

Here is the line where travis fails. Looks like the nativ comparison tool can not be unpacked: https://travis-ci.org/bitcoin/bitcoin/jobs/62393150#L272

Member

jonasschnelli commented May 13, 2015

Here is the line where travis fails. Looks like the nativ comparison tool can not be unpacked: https://travis-ci.org/bitcoin/bitcoin/jobs/62393150#L272

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost May 13, 2015

thanks for noticing that @jonasschnelli , i'm looking into the jar issues today and yea, its buggy all-around (#6119)

ghost commented May 13, 2015

thanks for noticing that @jonasschnelli , i'm looking into the jar issues today and yea, its buggy all-around (#6119)

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 14, 2015

Member

Travis is now happy with this. Would be nice if we can include this in 0.11.

Member

jonasschnelli commented May 14, 2015

Travis is now happy with this. Would be nice if we can include this in 0.11.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj May 15, 2015

Member

@theuni Can you take a look here?

Member

laanwj commented May 15, 2015

@theuni Can you take a look here?

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni May 15, 2015

Member

Sure. I'd like to go through the entire release process with this change on top, just to make sure there's nothing missing. Will do that today.

Member

theuni commented May 15, 2015

Sure. I'd like to go through the entire release process with this change on top, just to make sure there's nothing missing. Will do that today.

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni May 15, 2015

Member

The detached sig create/apply scripts needed a good bit of quoting love to deal with the spaces. My fault there.

I'm fixing those up now.

Member

theuni commented May 15, 2015

The detached sig create/apply scripts needed a good bit of quoting love to deal with the spaces. My fault there.

I'm fixing those up now.

@@ -106,7 +106,7 @@ $(APP_DIST_DIR)/Applications:
$(APP_DIST_EXTRAS): $(APP_DIST_DIR)/$(OSX_APP)/Contents/MacOS/Bitcoin-Qt
$(OSX_DMG): $(APP_DIST_EXTRAS)
$(GENISOIMAGE) -no-cache-inodes -D -l -probe -V "Bitcoin-Qt" -no-pad -r -apple -o $@ dist
$(GENISOIMAGE) -no-cache-inodes -D -l -probe -V "Bitcoin-Core" -no-pad -r -apple -o $@ dist

This comment has been minimized.

@theuni

theuni May 15, 2015

Member

Any reason for not using "Bitcoin Core" here? These set the volume descriptor that OSX ends up using for mounting.

@theuni

theuni May 15, 2015

Member

Any reason for not using "Bitcoin Core" here? These set the volume descriptor that OSX ends up using for mounting.

This comment has been minimized.

@laanwj

laanwj May 19, 2015

Member

Agreed @theuni. Let's avoid '-' where possible.

@laanwj

laanwj May 19, 2015

Member

Agreed @theuni. Let's avoid '-' where possible.

This comment has been minimized.

@jonasschnelli

jonasschnelli May 19, 2015

Member

Agreed. Fixed.

@jonasschnelli

jonasschnelli May 19, 2015

Member

Agreed. Fixed.

@@ -33,5 +33,5 @@ script: |
tar -xf ${UNSIGNED}
./detached-sig-apply.sh ${UNSIGNED} signature.tar.gz
${WRAP_DIR}/genisoimage -no-cache-inodes -D -l -probe -V "Bitcoin-Qt" -no-pad -r -apple -o uncompressed.dmg signed-app
${WRAP_DIR}/genisoimage -no-cache-inodes -D -l -probe -V "Bitcoin-Core" -no-pad -r -apple -o uncompressed.dmg signed-app

This comment has been minimized.

@theuni

theuni May 15, 2015

Member

Same here.

@theuni

theuni May 15, 2015

Member

Same here.

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni May 15, 2015

Member

@jonasschnelli I needed theuni@c2117a6 in order to get signing working. Seems to be working fine. It even hides the old Bitcoin-Qt in Launchpad. Neat.

I'm not 100% sure that this won't cause any issues for upgraders, but I guess we'll find out when we do a real signing for rc1.

Member

theuni commented May 15, 2015

@jonasschnelli I needed theuni@c2117a6 in order to get signing working. Seems to be working fine. It even hides the old Bitcoin-Qt in Launchpad. Neat.

I'm not 100% sure that this won't cause any issues for upgraders, but I guess we'll find out when we do a real signing for rc1.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 17, 2015

Member

@theuni: will pull in you commit as soon as I'm back at the keys.

Upgraders should delete the old version in the /Application folder.
Launchpad uses the bundle identifier - which i kept the same in this PR - to identify the uniqueness.

Member

jonasschnelli commented May 17, 2015

@theuni: will pull in you commit as soon as I'm back at the keys.

Upgraders should delete the old version in the /Application folder.
Launchpad uses the bundle identifier - which i kept the same in this PR - to identify the uniqueness.

@laanwj laanwj added this to the 0.11.0 milestone May 18, 2015

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 18, 2015

Member

Pulled in @theuni's change and built again over gitian:
https://builds.jonasschnelli.ch/pulls/6116/

This is how it looks after and before this PR:
bildschirmfoto-2015-05-18-um-15 11 52

Member

jonasschnelli commented May 18, 2015

Pulled in @theuni's change and built again over gitian:
https://builds.jonasschnelli.ch/pulls/6116/

This is how it looks after and before this PR:
bildschirmfoto-2015-05-18-um-15 11 52

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj May 19, 2015

Member

utACK

Member

laanwj commented May 19, 2015

utACK

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 19, 2015

Member

It's currently not working:

/home/ubuntu/build/bitcoin/depends/x86_64-apple-darwin11/native/bin/genisoimage: No such file or directory. Invalid node - 'Core'.

Now i know why i left the dash there. :)

Will try to find a fix (maybe over renaming) for this.

Member

jonasschnelli commented May 19, 2015

It's currently not working:

/home/ubuntu/build/bitcoin/depends/x86_64-apple-darwin11/native/bin/genisoimage: No such file or directory. Invalid node - 'Core'.

Now i know why i left the dash there. :)

Will try to find a fix (maybe over renaming) for this.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 19, 2015

Member

It looks like genisoimage's -V (volid) parameter is not whitespace compatible (even with quotes). Therefore i suggest using "Bitcoin-Core" as disk image title.

Member

jonasschnelli commented May 19, 2015

It looks like genisoimage's -V (volid) parameter is not whitespace compatible (even with quotes). Therefore i suggest using "Bitcoin-Core" as disk image title.

@laanwj laanwj merged commit d1a3866 into bitcoin:master May 20, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request May 20, 2015

Merge pull request #6116
d1a3866 build: Cope with spaces in filenames when creating/applying OSX sigs (Cory Fields)
7cef321 [Mac only] rename Bitcoin-Qt.app to "Bitcoin Core.app" (Jonas Schnelli)
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj May 20, 2015

Member

I suppose the volume name isn't user-visible normally? In any case, not important.

Member

laanwj commented May 20, 2015

I suppose the volume name isn't user-visible normally? In any case, not important.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 20, 2015

Member

@laanwj: it is visible but not that important. See #6116 (comment). It's visible in the left part of a Finder window as well as in the window title of the opened disk-image in Finder.

Member

jonasschnelli commented May 20, 2015

@laanwj: it is visible but not that important. See #6116 (comment). It's visible in the left part of a Finder window as well as in the window title of the opened disk-image in Finder.

@fanquake

This comment has been minimized.

Show comment
Hide comment
@fanquake

fanquake May 20, 2015

Member

Post merge ACK, glad this is now done.

On Wednesday, May 20, 2015, Jonas Schnelli notifications@github.com wrote:

@laanwj https://github.com/laanwj: it is visible but not that
important. See #6116 (comment)
#6116 (comment).
It's visible in the left part of a Finder window as well as in the window
title of the opened disk-image in Finder.

Reply to this email directly or view it on GitHub
#6116 (comment).

Member

fanquake commented May 20, 2015

Post merge ACK, glad this is now done.

On Wednesday, May 20, 2015, Jonas Schnelli notifications@github.com wrote:

@laanwj https://github.com/laanwj: it is visible but not that
important. See #6116 (comment)
#6116 (comment).
It's visible in the left part of a Finder window as well as in the window
title of the opened disk-image in Finder.

Reply to this email directly or view it on GitHub
#6116 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment