Skip to content

Conversation

@domob1812
Copy link
Contributor

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.

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.
@domob1812
Copy link
Contributor Author

Note that I'm not an expert in m4, so I cannot really tell whether the fix is "correct". It just works in my trials.

@laanwj
Copy link
Member

laanwj commented Feb 8, 2016

utACK, although this pretty much "fixes" a problem that we don't have. Derived projects with commas in their copyright string could also add the extra [].

@sipa
Copy link
Member

sipa commented Feb 8, 2016

Seems this also makes an unrelatef change (copyright year instead of version is release)?

@laanwj
Copy link
Member

laanwj commented Feb 8, 2016

@sipa Correct. Though I think that change makes sense

-AC_DEFINE(COPYRIGHT_YEAR, _COPYRIGHT_YEAR, [Version is release])
+AC_DEFINE(COPYRIGHT_YEAR, _COPYRIGHT_YEAR, [Copyright year])

Currently we get this in src/config/bitcoin-config.h:

/* Version is release */
#define COPYRIGHT_YEAR 2016

@domob1812
Copy link
Contributor Author

Yes, I agree - this is not high priority for Bitcoin. However, since we actually factored the copyright holders out, I think it makes sense to do it "correctly".

True, the copyright-year change is unrelated - but I stumbled upon it while editing, and think it also makes sense and is trivial enough. But I can also remove it from the PR if you prefer.

@luke-jr
Copy link
Member

luke-jr commented Feb 8, 2016

Hmm, I didn't expect multiple things in the substitution part, and the CopyrightHolders function probably won't react correctly to that. But utACK I suppose, this change shouldn't hurt.

@laanwj
Copy link
Member

laanwj commented Feb 9, 2016

Hmm, I didn't expect multiple things in the substitution part, and the CopyrightHolders function probably won't react correctly to that.

Right. Would make sense to have a Tested ACK here before merge.

@laanwj laanwj merged commit 72fd008 into bitcoin:master Mar 31, 2016
laanwj added a commit that referenced this pull request Mar 31, 2016
72fd008 Fix quoting of copyright holders in configure.ac. (Daniel Kraft)
codablock pushed a commit to codablock/dash that referenced this pull request Dec 19, 2017
72fd008 Fix quoting of copyright holders in configure.ac. (Daniel Kraft)
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants