Skip to content

Building Environment: Set ARFLAGS to cr#10766

Merged
sipa merged 1 commit intobitcoin:masterfrom
ReneNyffenegger:ARFLAGS
Jul 16, 2017
Merged

Building Environment: Set ARFLAGS to cr#10766
sipa merged 1 commit intobitcoin:masterfrom
ReneNyffenegger:ARFLAGS

Conversation

@ReneNyffenegger
Copy link
Contributor

@ReneNyffenegger ReneNyffenegger commented Jul 7, 2017

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.

@practicalswift
Copy link
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.

@theuni
Copy link
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

@ReneNyffenegger ReneNyffenegger mentioned this pull request Jul 10, 2017
Closed
@ReneNyffenegger
Copy link
Contributor Author

ReneNyffenegger commented Jul 10, 2017

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

@theuni
Copy link
Member

theuni commented Jul 10, 2017

Thanks. utACK after squash.

@jonasschnelli
Copy link
Contributor

jonasschnelli commented Jul 13, 2017

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

@ReneNyffenegger
Copy link
Contributor Author

ReneNyffenegger commented Jul 13, 2017

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

TheBlueMatt
TheBlueMatt reviewed Jul 14, 2017
View reviewed changes
Copy link
Contributor

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK, though I tested and overriding doesnt work.

configure.ac Outdated

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
Copy link
Contributor

@TheBlueMatt TheBlueMatt Jul 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"
Copy link
Contributor

@TheBlueMatt TheBlueMatt Jul 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ReneNyffenegger
Copy link
Contributor Author

ReneNyffenegger commented Jul 15, 2017

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

@ReneNyffenegger
Copy link
Contributor Author

ReneNyffenegger commented Jul 15, 2017

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

@TheBlueMatt
Copy link
Contributor

TheBlueMatt commented Jul 15, 2017

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

@ReneNyffenegger ReneNyffenegger force-pushed the ARFLAGS branch 2 times, most recently from 824ae38 to 152d488 Compare July 15, 2017 19:38
@ReneNyffenegger
Copy link
Contributor Author

ReneNyffenegger commented Jul 15, 2017

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.

@practicalswift
Copy link
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

@ReneNyffenegger
Copy link
Contributor Author

ReneNyffenegger commented Jul 15, 2017

done rebase.

@practicalswift
Copy link
Contributor

practicalswift commented Jul 15, 2017

utACK 517bfb174a5affc426d8eeb008cb42c49b8f59a9

@ReneNyffenegger
Use AC_ARG_VAR to set ARFLAGS.
912da1d 
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.
@TheBlueMatt
Copy link
Contributor

TheBlueMatt commented Jul 16, 2017 •
edited
Loading

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

@sipa
Copy link
Member

sipa commented Jul 16, 2017

utACK 912da1d

@practicalswift
Copy link
Contributor

practicalswift commented Jul 16, 2017

utACK 912da1d

@sipa sipa merged commit 912da1d into bitcoin:master Jul 16, 2017
sipa added a commit that referenced this pull request Jul 16, 2017
@sipa
Merge #10766: Building Environment: Set ARFLAGS to cr
b4d03be 
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
@str4d str4d mentioned this pull request Dec 1, 2017
Merged
zkbot added a commit to zcash/zcash that referenced this pull request Dec 1, 2017
@zkbot
Auto merge of #2786 - str4d:2074-build, r=<try>
6aef403 
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
@zkbot
Auto merge of #2786 - str4d:2074-build, r=str4d
83af270 
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.
kotodev pushed a commit to koto-dev/koto.old that referenced this pull request Jan 25, 2018
@zkbot @kotodev
Auto merge of #2786 - str4d:2074-build, r=str4d
c8a979f 
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
@renium9
Auto merge of #2786 - str4d:2074-build, r=str4d
23640da 
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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 18, 2019
@sipa @PastaPastaPasta
Merge bitcoin#10766: Building Environment: Set ARFLAGS to cr
d7057d4 
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
@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.

Reviewers

1 more reviewer

@TheBlueMatt TheBlueMatt TheBlueMatt left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Build system

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@ReneNyffenegger @practicalswift @theuni @jonasschnelli @TheBlueMatt @sipa @fanquake