Building Environment: Set ARFLAGS to cr #10766

Merged
merged 1 commit into from Jul 16, 2017

Conversation

Projects
None yet
7 participants

Override the default of ARFLAGS of cru to cr.

When building, ar produces a warning for each archive, for example

  AR       libbitcoin_server.a
/usr/bin/ar: `u' modifier ignored since `D' is the default (see `U')

Since u is the default anyway, it cannot hurt to remove it.

Contributor

practicalswift commented Jul 7, 2017

Concept ACK

Nit: I don't think that u is the default - IIRC it is just that u optimization has no effect when running in deterministic mode (D), which is now the default.

Member

theuni commented Jul 7, 2017

Concept ACK. Though, please don't hard-code it to "cr" in case the change causes trouble for some toolchain. Let's use AC_ARG_VAR instead, so that it may be overridden if necessary:

AC_ARG_VAR(ARFLAGS, [flags passed to the archiver])
if test "x${ARFLAGS+set}" != "xset"; then
  ARFLAGS="cr"
fi

I agree / have created a hopefully improved pull request: 10782

Member

theuni commented Jul 10, 2017

Thanks. utACK after squash.

Member

jonasschnelli commented Jul 13, 2017

utACK bb4bb93
@ReneNyffenegger: can you squash the commits? Your first commit (bb4bb93) has "unrecoggnized author".

I've squashed to commits / Hope the user is now recognized.

@TheBlueMatt

Concept ACK, though I tested and overriding doesnt work.

configure.ac
@@ -19,6 +19,12 @@ BITCOIN_GUI_NAME=bitcoin-qt
BITCOIN_CLI_NAME=bitcoin-cli
BITCOIN_TX_NAME=bitcoin-tx
+dnl Unless the user specified ARFLAGS, force it to be cr
+AC_ARG_VAR(ARFLAGS, [Flags for the archiver, defaults to <cr> if not set])
+if test "x${ARFLAGS}" != "xset"; then
@TheBlueMatt

TheBlueMatt Jul 14, 2017

Contributor

Do you need the x${ARFLAGS+set} syntax that we use elsewhere?

configure.ac
+dnl Unless the user specified ARFLAGS, force it to be cr
+AC_ARG_VAR(ARFLAGS, [Flags for the archiver, defaults to <cr> if not set])
+if test "x${ARFLAGS}" != "xset"; then
+ ARFLAGS="cr"
@TheBlueMatt

TheBlueMatt Jul 14, 2017

Contributor

Maybe add D to the default here, so that we're consistent even on platforms with a strange ar build?

overriding didn't work.... because I goofed up when I manually tried to merge two branches: I had
if test "x${ARFLAGS}"...
instead of
if test "x${ARFLAGS+set}"...

With the fix, I've also added the D flag

Apparently, the D flag is not liked in the Darwin environment.

Contributor

TheBlueMatt commented Jul 15, 2017

Heh, ok, just leave it as cr I suppose.

I have removed the D flag again. Now, I have also commit 816aa59. I don't know why this is, if I should remove it, and how I could remove it.

Contributor

practicalswift commented Jul 15, 2017

@ReneNyffenegger Please do:

git rebase -i HEAD~2
# remove the line with "Fix multi_rpc test …"
git push -f

done rebase.

Contributor

practicalswift commented Jul 15, 2017

utACK 517bfb1

René Nyffenegger Use AC_ARG_VAR to set ARFLAGS.
The user can set ARFLAGS in the ./configure step with
  ./configure ARFLAGS=...
If he chooses not to do so, ARFLAGS will be set to cr.
912da1d
Contributor

TheBlueMatt commented Jul 16, 2017 edited

ACK 912da1d. We'll need to do something about similar warnings in leveldb and secp256k1, but those will need to be submitted separately upstream.

Owner

sipa commented Jul 16, 2017

utACK 912da1d

Contributor

practicalswift commented Jul 16, 2017

utACK 912da1d

@sipa sipa merged commit 912da1d into bitcoin:master Jul 16, 2017

1 check passed

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

@sipa sipa added a commit that referenced this pull request Jul 16, 2017

@sipa sipa Merge #10766: Building Environment: Set ARFLAGS to cr
912da1d Use AC_ARG_VAR to set ARFLAGS. (René Nyffenegger)

Pull request description:

  Override the default of ARFLAGS of `cru` to `cr`.

  When building, ar produces a warning for each archive, for example
  ```
    AR       libbitcoin_server.a
  /usr/bin/ar: `u' modifier ignored since `D' is the default (see `U')

  ```
  Since `u` is the default anyway, it cannot hurt to remove it.

Tree-SHA512: 7466764f847b70f0f67db25dac87a7794477abf1997cb946682f394fe80ae86ac3ed52cbadb35f0c18a87467755bde5a5158430444cd26fb60fa363cc7bd486d
b4d03be
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment