Page MenuHomePhabricator

Add a build command to arcanist
Needs ReviewPublic

Authored by Fabien on Wed, Apr 10, 12:05.

Details

Reviewers
deadalnix
jasonbcox
Group Reviewers
Restricted Project
Summary

This add a workflow which is a wrapper to the cmake/ninja build.
This will later be used as a preliminary step to arc unit.

Argument (optional):

  • targets: specify which Ninja target to build.
Test Plan

Initial state: a clean project tree.

arc build

Should output:

OKAY Build is successful (<build duration>)

WHICH_CMAKE=`which cmake`
sudo mv "${WHICH_CMAKE}" "${WHICH_CMAKE}.broken"
arc build

Should output:

FAILED  The `cmake` command is required to run `arc build`. Please
install CMake (https://cmake.org) then run `arc build` again.
sudo mv "${WHICH_CMAKE}.broken" "${WHICH_CMAKE}"
WHICH_NINJA=`which ninja`
sudo mv "${WHICH_NINJA}" "${WHICH_NINJA}.broken"
arc build

Should output:

FAILED  The `ninja` command is required to run `arc build`. Please
install Ninja (https://ninja-build.org) hen run `arc build` again.
sudo mv "${WHICH_NINJA}.broken" "${WHICH_NINJA}"
mv .arcbuild .arcbuild.bak
arc build

Should output:

FAILED  Unable to find the `.arcbuild` configuration file. Please create
the file at the root of the project.
mv .arcbuild.bak .arcbuild
sed -i "s/build/dliub/g" .arcbuild
arc build

Should output:

FAILED  No build directory is configured. Set the `build_directory`
option in your `.arcbuild` configuration file and run `arc build` again.
sed -i "s/dliub/build\"/g" .arcbuild
arc build

Should output:

FAILED  The `.arcbuild` file is not a valid JSON file, please check for
syntax errors.
sed -i "s/build\"/build/g" .arcbuild
mkdir build-arcanist
chmod -w .
arc build

Should output:

FAILED  The build directory </path/to/build-arcanist> already exists and
cannot be deleted. Please check the permissions or delete it manually
then run `arc build` again.
chmod +w .
rm -rf build-arcanist
chmod -w .
arc build

Should output:

FAILED  Unable to create the build directory:
/home/fabien/bitcoin-abc/build-arcanist. Check you have write
permissions to the parent directory and run `arc build` again.
chmod +w .

Comment the line

include(AddCompilerFlags)

in the src/CMakeLists.txt file.

arc build

Should output:

FAILED Build failed !

Uncomment the previously commented line.

Comment the line

#include "init.h"

in the src/init.cpp file.

arc build

Should output:

FAILED Build failed !

Uncomment the previously commented line.

arc build --trace test_bitcoin

Should output:

OKAY Build is successful (<build duration>)

Check in the trace that the test_bitcoin target is passed to ninja.

Change the build directory in .arcbuild to your project root (try absolute, relative "" and "." paths).

arc build

Should output:

FAILED The build directory is set to the project root: </path/to/project/root>. The directory is removed after the build, please set it to something else.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
arc_build
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 5501
Build 9064: Bitcoin ABC Teamcity Staging
Build 9063: arc lint + arc unit

Event Timeline

Fabien created this revision.Wed, Apr 10, 12:05
Herald added a reviewer: Restricted Project. · View Herald TranscriptWed, Apr 10, 12:05
Fabien edited the test plan for this revision. (Show Details)Wed, Apr 10, 13:43
Fabien retitled this revision from [WIP] Add a build command to arcanist to Add a build command to arcanist.
Fabien edited the summary of this revision. (Show Details)
Fabien edited the test plan for this revision. (Show Details)
Fabien updated this revision to Diff 8024.Wed, Apr 10, 13:45

Improve error messages

jasonbcox requested changes to this revision.Wed, Apr 10, 20:36
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
arcanist/workflow/ArcanistBuildWorkflow.php
7 ↗(On Diff #8024)

Just a thought: I've been using build-make and build-cmake for my build directories. The added - makes for better reading imo.

112 ↗(On Diff #8024)

This only works if the build dir is one level below the project root. Try adding this to the test plan and making sure it works (and probably apply a fix): arc build some-dir/build-cmake

This revision now requires changes to proceed.Wed, Apr 10, 20:36
Fabien added inline comments.Thu, Apr 11, 08:44
arcanist/workflow/ArcanistBuildWorkflow.php
112 ↗(On Diff #8024)

Good catch !

Fabien edited the test plan for this revision. (Show Details)Thu, Apr 11, 08:45
Fabien updated this revision to Diff 8033.Thu, Apr 11, 08:45
Fabien edited the test plan for this revision. (Show Details)

Allow to set the buil dir at any level.
Change the default build dir to build-cmake.

Fabien updated this revision to Diff 8034.Thu, Apr 11, 09:58

Move the default build directory to the configuration file.

Fabien edited the test plan for this revision. (Show Details)Thu, Apr 11, 09:59
deadalnix requested changes to this revision.Fri, Apr 12, 11:23

The goal is not to replace a build system. You don't need 25 configs or special cases. the goal is to provide a way for arc to build to it can run unit tests.

The more configuration this has, the more complex the rest of the pipeline will be and there are really no good reasons to go that road. If things needs to be cleared and otherwise managed, then this fails. If this is running in the same build dir I'm working in then you have no control over the build parameters.

The build workflow is using command but do not checks that these command are effectively usable, which will lead to people doing arc build and getting an error about ninja not found which makes no sense to them.

.arcconfig
10 ↗(On Diff #8034)
.arcbuild ?
arcanist/workflow/ArcanistBuildWorkflow.php
133 ↗(On Diff #8034)

At no point the user called cmake, so this error is 100% completely irrelevant.

144 ↗(On Diff #8034)

This is not how you build command lines.

155 ↗(On Diff #8034)

dito

This revision now requires changes to proceed.Fri, Apr 12, 11:23
Fabien updated this revision to Diff 8042.Fri, Apr 12, 15:59

Remove most of the arguments, check the cmake and ninja are installed

Fabien edited the summary of this revision. (Show Details)Fri, Apr 12, 16:00
Fabien edited the test plan for this revision. (Show Details)
Fabien added inline comments.
arcanist/workflow/ArcanistBuildWorkflow.php
144 ↗(On Diff #8034)

This printf-like syntax is what the Phutil ExecFuture expects.

Fabien planned changes to this revision.Wed, Apr 17, 09:08
Fabien updated this revision to Diff 8120.Thu, Apr 18, 16:01

Rebase and add some more checks.
Move the build directory removal to the finalize() method so it can
be called by arc unit after the tests are run.

Fabien edited the test plan for this revision. (Show Details)Thu, Apr 18, 16:04