Unify product name to fewer places #6812
Conversation
(cherry picked from commit bitcoin/bitcoin@d5f4683)
Zcash: Applies to 2015 for Zcash purposes. (cherry picked from commit bitcoin/bitcoin@1a6c67c)
(cherry picked from commit bitcoin/bitcoin@de619a3)
…e name This helps avoid accidental removal of upstream copyright names (cherry picked from commit bitcoin/bitcoin@917b1d0)
(cherry picked from commit bitcoin/bitcoin@3cae140)
Zcash: We don't have `share/setup.nsi.in` anymore, but this will ensure that any uses of `@PACKAGE_URL@` in future will be substituted. (cherry picked from commit bitcoin/bitcoin@29598e4)
…ons so it gets passed to extract-strings correctly (cherry picked from commit bitcoin/bitcoin@cddffaf)
…ctly (cherry picked from commit bitcoin/bitcoin@cc2095e)
…add a second line to copyrights in -version, About dialog, and splash screen (cherry picked from commit bitcoin/bitcoin@027fdb8)
str4d
added
the
C-upstream-port
| if (!mapArgs.count("-version")) { | ||
| strUsage += "\n" + _("Usage:") + "\n" + | ||
| " zcash-cli [options] <command> [params] " + _("Send command to Zcash") + "\n" + | ||
| " zcash-cli [options] <command> [params] " + strprintf(_("Send command to %s"), _(PACKAGE_NAME)) + "\n" + |
There was a problem hiding this comment.
Shouldn't this actually be "Send RPC command to zcashd"?
There was a problem hiding this comment.
In upstream this would say "Send command to Bitcoin Core", because "Bitcoin Core" is the "package name" (as opposed to other distributions). We've aliased "Zcash" to mean several things here, and zcashd isn't necessarily the "package name" we want. So the question is whether here we want this to mean the package name for the server, or the binary name for the server.
There was a problem hiding this comment.
"Send command to Zcash" is meaningless to me. Zcash is a protocol and a block chain; it's not something you can send a command to. Also "send command" is ambiguous; it could mean executing zcashd. You have to already know what it is intended to mean in order for it to make sense.
"Send an RPC request given by <command> to the running zcashd instance." is more precisely what we mean.
There was a problem hiding this comment.
Given all your subsequent comments, it sounds like what you really want is to set PACKAGE_NAME to "zcashd". Shall we just do that?
There was a problem hiding this comment.
I checked all of the pre-existing uses of PACKAGE_NAME:
./src/consensus/params.cpp:188: throw std::runtime_error("Funding stream address was not a valid " PACKAGE_NAME " address.");
./src/wallet/walletdb.cpp:1346: std::pair<std::string, std::string> networkInfo(PACKAGE_NAME, networkId);
./src/wallet/wallet.cpp:4233: std::pair<string, string> networkInfo(PACKAGE_NAME, networkIdString);
./src/wallet/wallet.cpp:6603: walletFile, _(PACKAGE_NAME)));
./src/wallet/wallet.cpp:6606: return UIError(strprintf(_("Wallet needed to be rewritten: restart %s to complete"), _(PACKAGE_NAME)));
./src/wallet/wallet.cpp:6610: return UIError(strprintf(_("Wallet %s is not for %s %s network"), walletFile, _(PACKAGE_NAME), params.NetworkIDString()));
./src/wallet/rpcwallet.cpp:421: throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, std::string("Invalid " PACKAGE_NAME " transparent address: ") + destStr);
./src/wallet/rpcwallet.cpp:1052: throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, std::string("Invalid " PACKAGE_NAME " transparent address: ") + strAddress);
./src/wallet/rpcwallet.cpp:1112: throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, std::string("Invalid " PACKAGE_NAME " transparent address: ") + destStr);
./src/wallet/rpcwallet.cpp:1287: throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, std::string("Invalid " PACKAGE_NAME " transparent address: ") + name_);
./src/init.cpp:1910: return InitError(_("-mineraddress is not a valid " PACKAGE_NAME " address."));The one at src/wallet/wallet.cpp:6606 could be "zcashd", but all of the rest should stay as "Zcash". So I think the best approach would be to add a DAEMON_NAME variable and change the relevant uses of PACKAGE_NAME to that. But that does not need to be done in this PR.
There was a problem hiding this comment.
I think most of those are misusing PACKAGE_NAME and actually mean "Zcash the network", not "Zcash the package maintained and distributed by ECC".
| @@ -39,7 +39,7 @@ | |||
| #define DO_STRINGIZE(X) #X | |||
|
|
|||
| //! Copyright string used in Windows .rc files | |||
| #define COPYRIGHT_STR "2009-" STRINGIZE(COPYRIGHT_YEAR) " The Bitcoin Core Developers and The Zcash developers" | |||
| #define COPYRIGHT_STR "2009-" STRINGIZE(COPYRIGHT_YEAR) " " COPYRIGHT_HOLDERS_FINAL | |||
There was a problem hiding this comment.
This appears not to be correct. It replaces "The Bitcoin Core Developers and The Zcash developers" with just "The Zcash developers".
There was a problem hiding this comment.
We made that change ourselves when we forked the codebase. AFAICT the upstream PR fixes this the way they want: the final commit appends a "The Bitcoin Core Developers" line if the main copyright line is changed to not contain it, and everywhere else makes it programmatic. So as long as this PR reflects the same programmatic fields as upstream, then this is being set to the correct value they want.
There was a problem hiding this comment.
Yes, but see https://github.com/zcash/zcash/pull/6812/files#r1441169298 .
There was a problem hiding this comment.
As I said, this is doing precisely what the Bitcoin Core developers wanted. If you think there's a mistake there, it is a mistake in upstream.
There was a problem hiding this comment.
Yes I think it is a mistake for (probably) every fork that takes this PR as-is. (It isn't a mistake upstream, because upstream, COPYRIGHT_STR will still contain the attribution to "The Bitcoin Core developers".)
More fundamentally, Bitcoin Core doesn't get to change Zcash's copyright attributions — even if the effect of their changes is to incorrectly remove the attribution to "The Bitcoin Core developers" from some places. We have a legal obligation to retain that attribution everywhere that claims to be a statement of who holds the copyright.
Note: the particular developers who wrote this PR upstream don't legally have the right to remove the copyright attribution to other Bitcoin developers from anywhere it occurs, whether that's upstream or in forks. If we copy their mistake, then it becomes our mistake.
There was a problem hiding this comment.
| #define COPYRIGHT_STR "2009-" STRINGIZE(COPYRIGHT_YEAR) " " COPYRIGHT_HOLDERS_FINAL | |
| #define COPYRIGHT_STR "2015-" STRINGIZE(COPYRIGHT_YEAR) " " COPYRIGHT_HOLDERS_FINAL "; 2009-" STRINGIZE(COPYRIGHT_YEAR) " The Bitcoin Core developers" |
(This will not change the formatting in places that use the CopyrightHolders function. I checked that COPYRIGHT_STR is only used in the .rc files.)
The old configure.ac did not work for a copyright holders string containing commas due to insufficient quoting. The new one allows this. While this is, of course, not of direct consequence to the current code (where the string is "Bitcoin Core"), it should still be fixed now that the string is actually factored out. (cherry picked from commit bitcoin/bitcoin@72fd008)
This was added in 386efb7 to address spammy Clang warnings when building with ccache. The issue was addressed in ccache 3.2 (https://bugzilla.samba.org/show_bug.cgi?id=8118, Nov 2014), and from a look at all major distros, it's only Debian Jessie that has a version of ccache older than that (3.1). Therefore I think it's acceptable to drop this workaround, and re-enable warnings for unused driver arguments (when compiling using Clang). (cherry picked from commit bitcoin/bitcoin@a029805)
MinGW Clang complains about `-mthreads` as an unused option. See msys2/MINGW-packages#9850
str4d
force-pushed
the
unify-product-name
branch
from
5d58718
to
4a46d2b
Compare
| @@ -17,7 +17,7 @@ BEGIN | |||
| BLOCK "040904E4" // U.S. English - multilingual (hex) | |||
| BEGIN | |||
| VALUE "CompanyName", "Zcash" | |||
| VALUE "FileDescription", "zcash-cli (JSON-RPC client for Zcash)" | |||
| VALUE "FileDescription", "zcash-cli (JSON-RPC client for " PACKAGE_NAME ")" | |||
| VALUE "FileVersion", VER_FILEVERSION_STR | |||
| VALUE "InternalName", "zcash-cli" | |||
| VALUE "LegalCopyright", COPYRIGHT_STR | |||
There was a problem hiding this comment.
COPYRIGHT_STR is used here directly (not via the CopyrightHolders function), so it's a regression in the Windows executable metadata that it now omits attribution to the Bitcoin Core Developers. That is, this is no longer the correct legal copyright. Similarly in the .rc files for the other executables: bitcoin-tx-res.rc and bitcoind-res.rc.
| return "\n" + | ||
| FormatParagraph(strprintf(_("Copyright (C) 2009-%i The Bitcoin Core Developers"), COPYRIGHT_YEAR)) + "\n" + | ||
| FormatParagraph(strprintf(_("Copyright (C) 2015-%i The Zcash Developers"), COPYRIGHT_YEAR)) + "\n" + | ||
| return CopyrightHolders(strprintf(_("Copyright (C) %i-%i"), 2009, COPYRIGHT_YEAR) + " ") + "\n" + |
There was a problem hiding this comment.
This changes the copyright year range to start at 2009 instead of 2015 for The Zcash Developers. But the Zcash Developers didn't exist as such in 2009; 2015 is more correct for that line.
| return CopyrightHolders(strprintf(_("Copyright (C) %i-%i"), 2009, COPYRIGHT_YEAR) + " ") + "\n" + | |
| return CopyrightHolders(strprintf(_("Copyright (C) %i-%i"), 2015, COPYRIGHT_YEAR) + " ") + "\n" + |
(This depends on my suggested fix to CopyrightHolders below, which is generally correct for any fork and would be upstreamable.)
|
Edit: my latest suggested changes address my objections. |
| strCopyrightHolders = strprintf(strCopyrightHolders, _(COPYRIGHT_HOLDERS_SUBSTITUTION)); | ||
| } | ||
| if (strCopyrightHolders.find("Bitcoin Core developers") == strCopyrightHolders.npos) { | ||
| strCopyrightHolders += "\n" + strPrefix + "The Bitcoin Core developers"; |
There was a problem hiding this comment.
| strCopyrightHolders += "\n" + strPrefix + "The Bitcoin Core developers"; | |
| strCopyrightHolders += "\n" + strprintf(_("Copyright (C) %i-%i"), 2009, COPYRIGHT_YEAR) + | |
| " The Bitcoin Core developers"; |
Cherry-picked from the following upstream PRs: