Page MenuHomePhabricator

Add a build command to arcanist
Changes PlannedPublic

Authored by Fabien on Apr 10 2019, 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 Passed
Unit
No Test Coverage
Build Status
Buildable 5435
Build 8932: Bitcoin ABC Buildbot (legacy)
Build 8931: arc lint + arc unit

Event Timeline

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)
jasonbcox requested changes to this revision.Apr 10 2019, 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.Apr 10 2019, 20:36
arcanist/workflow/ArcanistBuildWorkflow.php
112 ↗(On Diff #8024)

Good catch !

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.

Move the default build directory to the configuration file.

deadalnix requested changes to this revision.Apr 12 2019, 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
.arcbuild ?
arcanist/workflow/ArcanistBuildWorkflow.php
133

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

144

This is not how you build command lines.

155

dito

This revision now requires changes to proceed.Apr 12 2019, 11:23

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

Fabien edited the test plan for this revision. (Show Details)
Fabien added inline comments.
arcanist/workflow/ArcanistBuildWorkflow.php
144

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

Fabien planned changes to this revision.Apr 17 2019, 09:08

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.

deadalnix requested changes to this revision.May 16 2019, 17:06

Back on your queue. This would not serve much of a purpose before cmake build works anyways.

This revision now requires changes to proceed.May 16 2019, 17:06
Fabien planned changes to this revision.Jul 24 2019, 06:46