Build System: Use PACKAGE_TARNAME in NSIS script #7603

JeremyRand commented at 12:44 AM on February 26, 2016: contributor

This PR replaces the hardcoded string "bitcoin" in the NSIS script with the autoconf variable PACKAGE_TARNAME; fixes #7265 .

Places where I chose not to replace:

bitcoin.ico wasn't replaced because it doesn't seem to be relevant to the build system and its filename never affects the end user.
InstallDir wasn't replaced because the current text has an uppercase B, and I'm not sure of a good way to capitalize the result of PACKAGE_TARNAME.
A comment in the Main Installer section wasn't replaced because comments don't ever face the end user.
The registry value "URL:Bitcoin" wasn't replaced for the same reason as InstallDir.
Startup shortcut wasn't replaced for the same reason as InstallDir.

All other appearances of "bitcoin" in the NSIS script were replaced.

MarcoFalke commented at 5:43 AM on February 26, 2016: member

Concept ACK.

jonasschnelli commented at 7:32 AM on February 26, 2016: contributor

Concept ACK. Kicked a build to see if this works over gitian/depends build: https://bitcoin.jonasschnelli.ch/pulls/7603/

jonasschnelli added the label Windows on Feb 26, 2016

jonasschnelli added the label Build system on Feb 26, 2016

MarcoFalke commented at 3:37 PM on February 26, 2016: member

Looks like gitian is happy with this

in share/setup.nsi.in:None in ad8ca48dcd outdated

  19 | @@ -20,7 +20,7 @@ SetCompressor /SOLID lzma
  20 |  !define MUI_STARTMENUPAGE_REGISTRY_KEY ${REGKEY}
  21 |  !define MUI_STARTMENUPAGE_REGISTRY_VALUENAME StartMenuGroup
  22 |  !define MUI_STARTMENUPAGE_DEFAULTFOLDER "@PACKAGE_NAME@"
  23 | -!define MUI_FINISHPAGE_RUN $INSTDIR\bitcoin-qt.exe
  24 | +!define MUI_FINISHPAGE_RUN $INSTDIR\@PACKAGE_TARNAME@-qt.exe

laanwj commented at 4:41 PM on March 1, 2016:

Hmm, if we're parametrizing executable names anyway I'd prefer to be able to customize the entire name, e.g. have a @BITCOIN_QT_NAME@, and set that to @PACKAGE_TARNAME@-qt somewhere higher in the build system.

There's been talk of getting rid of the -qt suffix for the user interface in the past, so having it hard-coded in as few places as possible would be a step forward.

JeremyRand commented at 6:07 AM on March 5, 2016:

I agree that that would be useful. Unfortunately my familiarity with autoconf is limited enough that I'm not fully comfortable implementing that myself. Maybe someone else could do that as another pull request?

laanwj commented at 3:23 PM on March 7, 2016:

Fine with me.

in share/setup.nsi.in:None in ad8ca48dcd outdated

  86 |      File /r @abs_top_srcdir@/doc\*.*
  87 |      SetOutPath $INSTDIR
  88 |      WriteRegStr HKCU "${REGKEY}\Components" Main 1
  89 |  
  90 |      # Remove old wxwidgets-based-bitcoin executable and locales:
  91 | -    Delete /REBOOTOK $INSTDIR\bitcoin.exe

laanwj commented at 4:45 PM on March 1, 2016:

Heh, these are from <0.5.0, probably not a bad time to remove them.

JeremyRand commented at 6:08 AM on March 5, 2016:

I'd be fine with that. Should that be done as part of this pull request?

MarcoFalke commented at 4:58 PM on March 5, 2016:

Sure, go ahead.

laanwj commented at 12:44 PM on March 14, 2016: member

@theuni I suppose this is ok with you?

JeremyRand force-pushed on Mar 19, 2016

JeremyRand commented at 2:48 PM on March 19, 2016: contributor

I just rebased against latest master, and added a 2nd commit which removes the wxwidgets stuff as @laanwj and @MarcoFalke requested.

laanwj assigned theuni on Mar 31, 2016

theuni commented at 6:28 PM on March 31, 2016: member

Ooh neat, I'm assigned. I guess I should take that as a hint and fix my mail filters so I'm not missing pings :)

Agree with @laanwj wrt avoiding hard-coding filenames. The names are already set in the top Makefile as @BITCOIN_QT_BIN@ and @BITCOIN_WIN_INSTALLER@, we just need to modify BITCOIN_QT_BIN to not include a path. I'll work up a quick change for that.

theuni commented at 8:35 PM on April 1, 2016: member

@JeremyRand Can you please take the top two commits from https://github.com/theuni/bitcoin/tree/7603 ? You can squash the NSIS one into your own. After that, I'm good with this.

We can't use BITCOIN_QT_BIN/BITCOIN_WIN_INSTALLER as I said above because they're not yet defined. I added new vars and used them in one place to ensure that everything stays synced up.

JeremyRand commented at 10:10 PM on April 1, 2016: contributor

@theuni Is it okay if I modify the 4 lines at https://github.com/theuni/bitcoin/commit/411a71f57e075c7d0a7c79bf1f7c7d249d5145da#diff-67e997bcfdac55191033d57a16d1408aR17 to use PACKAGE_TARNAME instead of hardcoding the "bitcoin" substring on those lines?

theuni commented at 6:13 PM on April 4, 2016: member

@JeremyRand I'd prefer not. The naming there should be the canonical definition, so it really shouldn't depend on anything else.

JeremyRand commented at 11:23 AM on April 6, 2016: contributor

@theuni Hmm, in the use cases for changing these variables that I personally deal with (i.e. Namecoin) it's pretty much a given that when PACKAGE_TARNAME is changed, the binary filenames are going to change accordingly as well. Is there a use case I'm not seeing, where it's likely that PACKAGE_TARNAME and the prefix of the binary filenames will be different? (I admit that I'm not particularly familiar with the variety of customizations that people might make to the build scripts.)

laanwj commented at 2:12 PM on April 6, 2016: member

Well the idea would be to keep as much flexibility as possible, so to be able to configure various names separately from each other. I understand that you ideally want to keep your patch as small as possible, but if changing the executable names is at least centralized in one place that doesn't blow it up much.

JeremyRand commented at 10:27 PM on April 6, 2016: contributor

@laanwj Yes, agreed that flexibility is important here. However, is there a loss in flexibility if a forked project has to change "$PACKAGE_TARNAME" to something else in the four binary name variables, rather than changing "bitcoin" to something else in the same variables? I'm definitely not arguing that those four variables should be removed, I just think it makes sense for their default values to be dependent on PACKAGE_TARNAME since that seems to be a common configuration. Any forked project that wants the binary filename prefixes to be something other than PACKAGE_TARNAME can change them just as easily as changing them away from being "bitcoin". So I don't see a loss of flexibility -- am I missing something?

MarcoFalke commented at 3:52 PM on April 7, 2016: member

The loss in flexibility appears when someone changes PACKAGE_TARNAME and is surprised all the binaries changed their name, too. So, has to go back and adjust the names of the binaries...

I think 644e6de8f3e8a86022a2daa4666bb28dcd5a4e24 is an uncontroversial improvement (after squash), which can be merged right now. If you really want to refactor PACKAGE_TARNAME, I'd prefer to do this in another pull.

theuni commented at 4:23 PM on April 8, 2016: member

I agree with @MarcoFalke. @JeremyRand While it's obviously nice if our changes make life easier for other projects, our first priority has to be to our own.

There are plenty of scenarios where the 2 values could become out of sync, and forks may or may not wish to switch when we do. So inevitably, there's going to be some patching down the road. I'd rather not create something that appears to be a contract to downstreams in the process.

JeremyRand commented at 1:00 AM on April 11, 2016: contributor

@theuni @MarcoFalke Okay, sounds good, you've convinced me. I'll add the suggested commits to this PR when I have a few minutes (I'm traveling today).

build: define base filenames for use elsewhere in the buildsystem

Unfortunately, the target namees defined at the Makefile.am level can't be used
for *.in substitution. So these new defines will have to stay synced up with
those targets.

Using the new variables for the deploy targets in the main Makefile.am will
ensure that they stay in sync, otherwise build tests will fail.

0dbf6e4b40

build: Use PACKAGE_TARNAME and new bin names in NSIS script.

Replaces the hardcoded string "bitcoin" with the autoconf variable PACKAGE_TARNAME; fixes #7265.

Places where I chose not to replace:

1. bitcoin.ico wasn't replaced because it doesn't seem to be relevant to the build system and its filename never affects the end user.
2. InstallDir wasn't replaced because the current text has an uppercase B, and I'm not sure of a good way to capitalize the result of PACKAGE_TARNAME.
3. A comment in the Main Installer section wasn't replaced because comments don't ever face the end user.
4. The registry value "URL:Bitcoin" wasn't replaced for the same reason as InstallDir.
5. Startup shortcut wasn't replaced for the same reason as InstallDir.

All other appearances of "bitcoin" were replaced with PACKAGE_TARNAME, except for the bin names, which were instead replaced with the new bin name autoconf variables.

26880c34cd

Remove wxwidgets references from NSIS script.

The NSIS script tried to delete wxwidgets-based executables/locales.  These files are ancient, and presumably no users have them anymore, so we can simplify the NSIS script by removing those lines.

0528e30a45

JeremyRand force-pushed on Apr 11, 2016

JeremyRand commented at 6:57 AM on April 11, 2016: contributor

There we go. Hopefully I did the requested squash correctly.

MarcoFalke commented at 7:07 AM on April 11, 2016: member

utACK 0528e30

laanwj commented at 2:41 PM on April 15, 2016: member

Looks much better now! utACK 0528e30

laanwj merged this on Apr 15, 2016

laanwj closed this on Apr 15, 2016

laanwj referenced this in commit 73fc922ed6 on Apr 15, 2016

JeremyRand deleted the branch on May 5, 2016

codablock referenced this in commit fd1dd48c30 on Sep 16, 2017

codablock referenced this in commit ab2d98f26f on Sep 19, 2017

zkbot referenced this in commit 75604363cc on Dec 1, 2017

zkbot referenced this in commit 6aef4033a7 on Dec 1, 2017

zkbot referenced this in commit 83af270002 on Dec 15, 2017

codablock referenced this in commit 9bd55b1bfc on Dec 20, 2017

kotodev referenced this in commit c8a979fc92 on Jan 25, 2018

renium9 referenced this in commit 23640da445 on Feb 6, 2018

DrahtBot locked this on Sep 8, 2021