Page MenuHomePhabricator
Differential D4961

[CMAKE] Add a check_linker_flag function
ClosedPublic
Actions

Authored by Fabien on Jan 16 2020, 16:17.

Details

Reviewers
deadalnix
jasonbcox
Group Reviewers
Restricted Project
Commits
rSTAGING8f2432151c56: [CMAKE] Add a check_linker_flag function
rABC8f2432151c56: [CMAKE] Add a check_linker_flag function
Summary

Separate the check of the flag from its addition. This has the
beneficial side-effect to discard the need for removing the Werror
flag from CMAKE_REQUIRED_FLAGS, since its scope is now reduced.

There is no change in behavior.

Test Plan
ninja check

Run the Gitian builds

Diff Detail

Repository
rABC Bitcoin ABC
Branch
cmake_check_linker_flags
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 8984
Build 15931: Default Diff Build & Tests
Build 15930: arc lint + arc unit

Event Timeline

Fabien created this revision.Jan 16 2020, 16:17
Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 16 2020, 16:18
Herald added a reviewer: deadalnix. · View Herald Transcript
teamcity added a comment.Jan 16 2020, 16:18

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Jan 16 2020, 16:19

Build Bitcoin-ABC / Diffs (Resource Intensive) / Bitcoin-ABC Gitian Diff Testing started.

teamcity added a comment.Jan 16 2020, 16:22

Build Bitcoin-ABC / Diffs (Resource Intensive) / Bitcoin-ABC Gitian Diff Testing started.

teamcity added a comment.Jan 16 2020, 16:22

Build Bitcoin-ABC / Diffs / Diff Testing passed.

teamcity added a comment.Jan 16 2020, 16:22

Build Bitcoin-ABC / Diffs (Resource Intensive) / Bitcoin-ABC Gitian Diff Testing started.

teamcity added a comment.Jan 16 2020, 16:22
Build Bitcoin-ABC / Diffs (Resource Intensive) / Bitcoin-ABC Gitian Diff Testing failed.

Snippet of first build failure:

[16:19:10] :	 [Step 1/1] (Reading database ... 75%
[16:19:10] :	 [Step 1/1] (Reading database ... 80%
[16:19:10] :	 [Step 1/1] (Reading database ... 85%
[16:19:10] :	 [Step 1/1] (Reading database ... 90%
[16:19:10] :	 [Step 1/1] (Reading database ... 95%
[16:19:10] :	 [Step 1/1] (Reading database ... 100%
[16:19:10] :	 [Step 1/1] (Reading database ... 17687 files and directories currently installed.)
[16:19:10] :	 [Step 1/1] Removing rsyslog (8.1901.0-1) ...
[16:19:10] :	 [Step 1/1] invoke-rc.d: could not determine current runlevel
[16:19:10] :	 [Step 1/1] Stopping enhanced syslogd: rsyslogd already stopped.
[16:19:10] :	 [Step 1/1] (Reading database ... 
[16:19:10] :	 [Step 1/1] (Reading database ... 5%
[16:19:10] :	 [Step 1/1] (Reading database ... 10%
[16:19:10] :	 [Step 1/1] (Reading database ... 15%
[16:19:10] :	 [Step 1/1] (Reading database ... 20%
[16:19:10] :	 [Step 1/1] (Reading database ... 25%
[16:19:10] :	 [Step 1/1] (Reading database ... 30%
[16:19:10] :	 [Step 1/1] (Reading database ... 35%
[16:19:10] :	 [Step 1/1] (Reading database ... 40%
[16:19:10] :	 [Step 1/1] (Reading database ... 45%
[16:19:10] :	 [Step 1/1] (Reading database ... 50%
[16:19:10] :	 [Step 1/1] (Reading database ... 55%
[16:19:10] :	 [Step 1/1] (Reading database ... 60%
[16:19:10] :	 [Step 1/1] (Reading database ... 65%
[16:19:10] :	 [Step 1/1] (Reading database ... 70%
[16:19:10] :	 [Step 1/1] (Reading database ... 75%
[16:19:10] :	 [Step 1/1] (Reading database ... 80%
[16:19:10] :	 [Step 1/1] (Reading database ... 85%
[16:19:10] :	 [Step 1/1] (Reading database ... 90%
[16:19:10] :	 [Step 1/1] (Reading database ... 95%
[16:19:10] :	 [Step 1/1] (Reading database ... 100%
[16:19:10] :	 [Step 1/1] (Reading database ... 17629 files and directories currently installed.)
[16:19:10] :	 [Step 1/1] Purging configuration files for rsyslog (8.1901.0-1) ...
[16:19:10] :	 [Step 1/1] Processing triggers for systemd (241-7~deb10u1) ...
[16:19:10] :	 [Step 1/1] Adding 'local diversion of /sbin/initctl to /sbin/initctl.distrib'
[16:19:10] :	 [Step 1/1] Adding 'local diversion of /usr/bin/ischroot to /usr/bin/ischroot.distrib'
[16:19:10]W:	 [Step 1/1] dpkg-divert: warning: diverting file '/usr/bin/ischroot' from an Essential package with rename is dangerous, use --no-rename
[16:19:10] :	 [Step 1/1] Adding 'local diversion of /usr/sbin/policy-rc.d to /usr/sbin/policy-rc.d.distrib'
[16:19:10] :	 [Step 1/1] Starting target
[16:19:11] :	 [Step 1/1] Checking if target is up
[16:19:11] :	 [Step 1/1] Preparing build environment
[16:19:14] :	 [Step 1/1] Adding multiarch support (log in var/install.log)
[16:19:15] :	 [Step 1/1] Updating apt-get repository (log in var/install.log)
[16:19:19] :	 [Step 1/1] Installing additional packages (log in var/install.log)
[16:20:10] :	 [Step 1/1] Upgrading system, may take a while (log in var/install.log)
[16:20:18] :	 [Step 1/1] Creating package manifest
[16:20:23] :	 [Step 1/1] Creating build script (var/build-script)
[16:20:24] :	 [Step 1/1] Running build script (log in var/build.log)
[16:22:39]W:	 [Step 1/1] ./bin/gbuild:21:in `system!': failed to run on-target setarch x86_64 bash -x < var/build-script > var/build.log 2>&1 (RuntimeError)
[16:22:39]W:	 [Step 1/1] 	from ./bin/gbuild:167:in `build_one_configuration'
[16:22:39]W:	 [Step 1/1] 	from ./bin/gbuild:318:in `block (2 levels) in <main>'
[16:22:39]W:	 [Step 1/1] 	from ./bin/gbuild:313:in `each'
[16:22:39]W:	 [Step 1/1] 	from ./bin/gbuild:313:in `block in <main>'
[16:22:39]W:	 [Step 1/1] 	from ./bin/gbuild:311:in `each'
[16:22:39]W:	 [Step 1/1] 	from ./bin/gbuild:311:in `<main>'
[16:22:39]W:	 [Step 1/1] ++ move_log
[16:22:39]W:	 [Step 1/1] ++ mv var/install.log /home/teamcity/buildAgent/work/c4a5708f2bae7929/gitian-results/
[16:22:39]W:	 [Step 1/1] ++ mv var/build.log /home/teamcity/buildAgent/work/c4a5708f2bae7929/gitian-results/
[16:22:39]W:	 [Step 1/1] Process exited with code 1
[16:22:39]E:	 [Step 1/1] Process exited with code 1 (Step: Command Line)
Harbormaster completed remote builds in B8979: Diff 15519.Jan 16 2020, 16:22
teamcity added a comment.Jan 16 2020, 16:26

Build Bitcoin-ABC / Diffs (Resource Intensive) / Bitcoin-ABC Gitian Diff Testing passed.

teamcity added a comment.Jan 16 2020, 16:46

Build Bitcoin-ABC / Diffs (Resource Intensive) / Bitcoin-ABC Gitian Diff Testing passed.

Fabien planned changes to this revision.Jan 16 2020, 20:17
Fabien updated this revision to Diff 15531.Jan 16 2020, 21:02

Fix scope issue, and use string(APPEND FOO " BAR") instead of set(FOO "${FOO} BAR").

teamcity added a comment.Jan 16 2020, 21:02

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Jan 16 2020, 21:04

Build Bitcoin-ABC / Diffs (Resource Intensive) / Bitcoin-ABC Gitian Diff Testing started.

teamcity added a comment.Jan 16 2020, 21:07

Build Bitcoin-ABC / Diffs (Resource Intensive) / Bitcoin-ABC Gitian Diff Testing started.

teamcity added a comment.Jan 16 2020, 21:07

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8984: Diff 15531.Jan 16 2020, 21:07
teamcity added a comment.Jan 16 2020, 21:11

Build Bitcoin-ABC / Diffs (Resource Intensive) / Bitcoin-ABC Gitian Diff Testing passed.

teamcity added a comment.Jan 16 2020, 21:11

Build Bitcoin-ABC / Diffs (Resource Intensive) / Bitcoin-ABC Gitian Diff Testing started.

teamcity added a comment.Jan 16 2020, 21:17

Build Bitcoin-ABC / Diffs (Resource Intensive) / Bitcoin-ABC Gitian Diff Testing passed.

teamcity added a comment.Jan 16 2020, 21:19

Build Bitcoin-ABC / Diffs (Resource Intensive) / Bitcoin-ABC Gitian Diff Testing passed.

jasonbcox requested changes to this revision.Jan 16 2020, 21:56
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
cmake/modules/AddCompilerFlags.cmake
132

s/LINKERFLAGS/LINKER_FLAGS/g in this function. I had some trouble grepping until I realized...

151

Maybe I'm thinking of this too much like bash, but is there a reason f is lowercase but other variables are all caps?

157

Is this supposed to be double wrapped?

This revision now requires changes to proceed.Jan 16 2020, 21:56
Fabien updated this revision to Diff 15558.Jan 17 2020, 07:24

Address feedback.

teamcity added a comment.Jan 17 2020, 07:24

Build Bitcoin-ABC / Diffs / Diff Testing started.

Fabien added inline comments.Jan 17 2020, 07:24
cmake/modules/AddCompilerFlags.cmake
151

Not really, only habit I suppose, and consistency with other similar loops. CMake is case insensitive.
I have to admit that this is not consistent across the CMake files.

157

The funny thing is that it will give the same result, being single or double wrapped:

  • Single wrapped, you are in the case if(variable), and the variable value is interpreted as a boolean
  • Double wrapped, you are in the case if(constant), the constant being the content of the variable which is a boolean-like value.

I initially used a single wrap and switched to double wrap to make the intent more clear, since these variables containing variables is a bit convoluted. But your comment indicates that it makes it more confusing instead, so I'll better switch back to single wrap.

teamcity added a comment.Jan 17 2020, 07:28

Build Bitcoin-ABC / Diffs (Resource Intensive) / Bitcoin-ABC Gitian Diff Testing started.

teamcity added a comment.Jan 17 2020, 07:28

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9007: Diff 15558.Jan 17 2020, 07:28
teamcity added a comment.Jan 17 2020, 07:36

Build Bitcoin-ABC / Diffs (Resource Intensive) / Bitcoin-ABC Gitian Diff Testing started.

teamcity added a comment.Jan 17 2020, 07:41

Build Bitcoin-ABC / Diffs (Resource Intensive) / Bitcoin-ABC Gitian Diff Testing passed.

teamcity added a comment.Jan 17 2020, 07:41

Build Bitcoin-ABC / Diffs (Resource Intensive) / Bitcoin-ABC Gitian Diff Testing started.

teamcity added a comment.Jan 17 2020, 07:47

Build Bitcoin-ABC / Diffs (Resource Intensive) / Bitcoin-ABC Gitian Diff Testing passed.

teamcity added a comment.Jan 17 2020, 07:49

Build Bitcoin-ABC / Diffs (Resource Intensive) / Bitcoin-ABC Gitian Diff Testing passed.

deadalnix accepted this revision.Jan 17 2020, 15:07
Fabien updated this revision to Diff 15601.Jan 17 2020, 15:59

Rebase.

Fabien added a child revision: D5002: [CMAKE] Get rid of `add_compiler_flags_to_var`.Jan 17 2020, 16:02
teamcity added a comment.Jan 17 2020, 16:25

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Jan 17 2020, 16:36

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9026: Diff 15601.Jan 17 2020, 16:36
jasonbcox accepted this revision.Jan 17 2020, 22:05
This revision is now accepted and ready to land.Jan 17 2020, 22:05
Closed by commit rABC8f2432151c56: [CMAKE] Add a check_linker_flag function (authored by Fabien). · Explain WhyJan 20 2020, 15:28
This revision was automatically updated to reflect the committed changes.
Fabien added a commit: rABC8f2432151c56: [CMAKE] Add a check_linker_flag function.
Fabien added a commit: rSTAGING8f2432151c56: [CMAKE] Add a check_linker_flag function.Apr 7 2020, 22:35

Revision Contents
Changeset List

PathSize
cmake/
modules/
65 lines

Diff 15531

View Options

cmake/modules/AddCompilerFlags.cmake

Loading...