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

Build System: Use PACKAGE_TARNAME in NSIS script #7603

Merged
merged 3 commits into from Apr 15, 2016

Conversation

Projects
None yet
5 participants
@JeremyRand
Contributor

JeremyRand commented Feb 26, 2016

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:

  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" in the NSIS script were replaced.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Feb 26, 2016

Member

Concept ACK.

Member

MarcoFalke commented Feb 26, 2016

Concept ACK.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Feb 26, 2016

Member

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

Member

jonasschnelli commented Feb 26, 2016

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

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Feb 26, 2016

Member

Looks like gitian is happy with this

Member

MarcoFalke commented Feb 26, 2016

Looks like gitian is happy with this

@laanwj

View changes

Show outdated Hide outdated share/setup.nsi.in
@laanwj

View changes

Show outdated Hide outdated share/setup.nsi.in
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 14, 2016

Member

@theuni I suppose this is ok with you?

Member

laanwj commented Mar 14, 2016

@theuni I suppose this is ok with you?

@JeremyRand

This comment has been minimized.

Show comment
Hide comment
@JeremyRand

JeremyRand Mar 19, 2016

Contributor

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

Contributor

JeremyRand commented Mar 19, 2016

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

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Mar 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.

Member

theuni commented Mar 31, 2016

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

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Apr 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.

Member

theuni commented Apr 1, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@JeremyRand

JeremyRand Apr 1, 2016

Contributor

@theuni Is it okay if I modify the 4 lines at theuni@411a71f#diff-67e997bcfdac55191033d57a16d1408aR17 to use PACKAGE_TARNAME instead of hardcoding the "bitcoin" substring on those lines?

Contributor

JeremyRand commented Apr 1, 2016

@theuni Is it okay if I modify the 4 lines at theuni@411a71f#diff-67e997bcfdac55191033d57a16d1408aR17 to use PACKAGE_TARNAME instead of hardcoding the "bitcoin" substring on those lines?

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Apr 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.

Member

theuni commented Apr 4, 2016

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

@JeremyRand

This comment has been minimized.

Show comment
Hide comment
@JeremyRand

JeremyRand Apr 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.)

Contributor

JeremyRand commented Apr 6, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 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.

Member

laanwj commented Apr 6, 2016

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

This comment has been minimized.

Show comment
Hide comment
@JeremyRand

JeremyRand Apr 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?

Contributor

JeremyRand commented Apr 6, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Apr 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 644e6de 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.

Member

MarcoFalke commented Apr 7, 2016

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 644e6de 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

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Apr 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.

Member

theuni commented Apr 8, 2016

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

This comment has been minimized.

Show comment
Hide comment
@JeremyRand

JeremyRand Apr 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).

Contributor

JeremyRand commented Apr 11, 2016

@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).

JeremyRand and others added some commits Feb 26, 2016

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.
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.
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.
@JeremyRand

This comment has been minimized.

Show comment
Hide comment
@JeremyRand

JeremyRand Apr 11, 2016

Contributor

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

Contributor

JeremyRand commented Apr 11, 2016

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

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke
Member

MarcoFalke commented Apr 11, 2016

utACK 0528e30

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 15, 2016

Member

Looks much better now!
utACK 0528e30

Member

laanwj commented Apr 15, 2016

Looks much better now!
utACK 0528e30

@laanwj laanwj merged commit 0528e30 into bitcoin:master Apr 15, 2016

1 check passed

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

laanwj added a commit that referenced this pull request Apr 15, 2016

Merge #7603: Build System: Use PACKAGE_TARNAME in NSIS script
0528e30 Remove wxwidgets references from NSIS script. (JeremyRand)
26880c3 build: Use PACKAGE_TARNAME and new bin names in NSIS script. (JeremyRand)
0dbf6e4 build: define base filenames for use elsewhere in the buildsystem (Cory Fields)

@JeremyRand JeremyRand deleted the JeremyRand:nsis-tarname branch May 5, 2016

kyuupichan referenced this pull request in kyuupichan/BitcoinUnlimited Apr 12, 2017

Merge #7603: Build System: Use PACKAGE_TARNAME in NSIS script
0528e30 Remove wxwidgets references from NSIS script. (JeremyRand)
26880c3 build: Use PACKAGE_TARNAME and new bin names in NSIS script. (JeremyRand)
0dbf6e4 build: define base filenames for use elsewhere in the buildsystem (Cory Fields)

codablock added a commit to codablock/dash that referenced this pull request Sep 16, 2017

Merge bitcoin#7603: Build System: Use PACKAGE_TARNAME in NSIS script
0528e30 Remove wxwidgets references from NSIS script. (JeremyRand)
26880c3 build: Use PACKAGE_TARNAME and new bin names in NSIS script. (JeremyRand)
0dbf6e4 build: define base filenames for use elsewhere in the buildsystem (Cory Fields)

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

Merge bitcoin#7603: Build System: Use PACKAGE_TARNAME in NSIS script
0528e30 Remove wxwidgets references from NSIS script. (JeremyRand)
26880c3 build: Use PACKAGE_TARNAME and new bin names in NSIS script. (JeremyRand)
0dbf6e4 build: define base filenames for use elsewhere in the buildsystem (Cory Fields)

@str4d str4d referenced this pull request Dec 1, 2017

Merged

Build system improvements #2786

zkbot added a commit to zcash/zcash that referenced this pull request Dec 1, 2017

Auto merge of #2786 - str4d:2074-build, r=<try>
Build system improvements

Includes commits cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6978
  - Only the first commit (second is for QT)
- bitcoin/bitcoin#7059
- bitcoin/bitcoin#7603
  - Only the first commit (the rest are for QT)
- bitcoin/bitcoin#7954
- bitcoin/bitcoin#8314
  - Only the second commit (first is for QT)
- bitcoin/bitcoin#8504
  - Only the first commit (second was undoing something we didn't have)
- bitcoin/bitcoin#8520
- bitcoin/bitcoin#8563
- bitcoin/bitcoin#8249
- bitcoin/bitcoin#9156
- bitcoin/bitcoin#9831
- bitcoin/bitcoin#9789
- bitcoin/bitcoin#10766

Part of #2074.

zkbot added a commit to zcash/zcash that referenced this pull request Dec 1, 2017

Auto merge of #2786 - str4d:2074-build, r=<try>
Build system improvements

Includes commits cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6978
  - Only the first commit (second is for QT)
- bitcoin/bitcoin#7059
- bitcoin/bitcoin#7603
  - Only the first commit (the rest are for QT)
- bitcoin/bitcoin#7954
- bitcoin/bitcoin#8314
  - Only the second commit (first is for QT)
- bitcoin/bitcoin#8504
  - Only the first commit (second was undoing something we didn't have)
- bitcoin/bitcoin#8520
- bitcoin/bitcoin#8563
- bitcoin/bitcoin#8249
- bitcoin/bitcoin#9156
- bitcoin/bitcoin#9831
- bitcoin/bitcoin#9789
- bitcoin/bitcoin#10766

Part of #2074.

zkbot added a commit to zcash/zcash that referenced this pull request Dec 15, 2017

Auto merge of #2786 - str4d:2074-build, r=str4d
Build system improvements

Includes commits cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6978
  - Only the first commit (second is for QT)
- bitcoin/bitcoin#7059
- bitcoin/bitcoin#7603
  - Only the first commit (without the `BITCOIN_QT_BIN` variable; the rest are for QT)
- bitcoin/bitcoin#7954
- bitcoin/bitcoin#8314
  - Only the second commit (first is for QT)
- bitcoin/bitcoin#8504
  - Only the first commit (second was undoing something we didn't have)
- bitcoin/bitcoin#8520
- bitcoin/bitcoin#8563
- bitcoin/bitcoin#8249
- bitcoin/bitcoin#9156
- bitcoin/bitcoin#9831
- bitcoin/bitcoin#9789
- bitcoin/bitcoin#10766

Part of #2074.

codablock added a commit to codablock/dash that referenced this pull request Dec 20, 2017

Merge bitcoin#7603: Build System: Use PACKAGE_TARNAME in NSIS script
0528e30 Remove wxwidgets references from NSIS script. (JeremyRand)
26880c3 build: Use PACKAGE_TARNAME and new bin names in NSIS script. (JeremyRand)
0dbf6e4 build: define base filenames for use elsewhere in the buildsystem (Cory Fields)

kotodev added a commit to koto-dev/koto.old that referenced this pull request Jan 25, 2018

Auto merge of #2786 - str4d:2074-build, r=str4d
Build system improvements

Includes commits cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6978
  - Only the first commit (second is for QT)
- bitcoin/bitcoin#7059
- bitcoin/bitcoin#7603
  - Only the first commit (without the `BITCOIN_QT_BIN` variable; the rest are for QT)
- bitcoin/bitcoin#7954
- bitcoin/bitcoin#8314
  - Only the second commit (first is for QT)
- bitcoin/bitcoin#8504
  - Only the first commit (second was undoing something we didn't have)
- bitcoin/bitcoin#8520
- bitcoin/bitcoin#8563
- bitcoin/bitcoin#8249
- bitcoin/bitcoin#9156
- bitcoin/bitcoin#9831
- bitcoin/bitcoin#9789
- bitcoin/bitcoin#10766

Part of #2074.

renium9 added a commit to koto-dev/koto.old that referenced this pull request Feb 6, 2018

Auto merge of #2786 - str4d:2074-build, r=str4d
Build system improvements

Includes commits cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6978
  - Only the first commit (second is for QT)
- bitcoin/bitcoin#7059
- bitcoin/bitcoin#7603
  - Only the first commit (without the `BITCOIN_QT_BIN` variable; the rest are for QT)
- bitcoin/bitcoin#7954
- bitcoin/bitcoin#8314
  - Only the second commit (first is for QT)
- bitcoin/bitcoin#8504
  - Only the first commit (second was undoing something we didn't have)
- bitcoin/bitcoin#8520
- bitcoin/bitcoin#8563
- bitcoin/bitcoin#8249
- bitcoin/bitcoin#9156
- bitcoin/bitcoin#9831
- bitcoin/bitcoin#9789
- bitcoin/bitcoin#10766

Part of #2074.

# Conflicts:
#	configure.ac
#	src/Makefile.am
#	src/Makefile.gtest.include
#	src/Makefile.test.include
#	zcutil/build.sh
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment