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

Fix quoting of copyright holders in configure.ac. #7477

Merged
merged 1 commit into from Mar 31, 2016

Conversation

Projects
None yet
4 participants
@domob1812
Contributor

domob1812 commented Feb 7, 2016

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.

Fix quoting of copyright holders in configure.ac.
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

This comment has been minimized.

Show comment
Hide comment
@domob1812

domob1812 Feb 7, 2016

Contributor

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.

Contributor

domob1812 commented Feb 7, 2016

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 laanwj added the Build system label Feb 8, 2016

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Feb 8, 2016

Member

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 [].

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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Feb 8, 2016

Member

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

Member

sipa commented Feb 8, 2016

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

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Feb 8, 2016

Member

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

This comment has been minimized.

Show comment
Hide comment
@domob1812

domob1812 Feb 8, 2016

Contributor

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.

Contributor

domob1812 commented Feb 8, 2016

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

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Feb 8, 2016

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Feb 9, 2016

Member

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.

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

Merge #7477: Fix quoting of copyright holders in configure.ac.
72fd008 Fix quoting of copyright holders in configure.ac. (Daniel Kraft)

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

Merge #7477: Fix quoting of copyright holders in configure.ac.
72fd008 Fix quoting of copyright holders in configure.ac. (Daniel Kraft)

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

Merge #7477: Fix quoting of copyright holders in configure.ac.
72fd008 Fix quoting of copyright holders in configure.ac. (Daniel Kraft)

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

Merge #7477: Fix quoting of copyright holders in configure.ac.
72fd008 Fix quoting of copyright holders in configure.ac. (Daniel Kraft)

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

Merge #7477: Fix quoting of copyright holders in configure.ac.
72fd008 Fix quoting of copyright holders in configure.ac. (Daniel Kraft)

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

Merge #7477: Fix quoting of copyright holders in configure.ac.
72fd008 Fix quoting of copyright holders in configure.ac. (Daniel Kraft)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment