On Wed, Mar 11, 2020 at 03:13:25PM +0100, Pierre Neidhardt wrote:
> I've tested it and run the example here
> https://iroha.readthedocs.io/en/latest/getting_started/cli-guide.html
> It works!
> 
> Patch looks good, feel free to merge.
> A few comments below:
> 
> > +              (snippet
> > +               '(begin
> > +                  ;; Backport unversioned fmt dependency:
> > +                  (substitute* "libs/logger/logger.hpp"
> > +                    (("fmt::v5") "fmt"))
> > +                  #t))))
> 
> Mention the commit you've sent me and that this should be removed on
> next update.

Done

> 
> > +    (build-system cmake-build-system)
> > +    (arguments
> > +     `(#:configure-flags
> > +       '("-DTESTING=OFF"
> > +         ; Don't install the shared libraries of the dependencies:
> > +         "-DENABLE_LIBS_PACKAGING=OFF")
> > +       #:tests? #f
> 
> Explain why the tests must be disabled.
> 

Noted

> > +       #:test-target "."
> 
> Why setting the test-target if you are not running it?
> 

I added a note to the build instructions, for when we can run the tests

> > +       #:phases
> > +       (modify-phases %standard-phases
> > +         (add-after 'unpack 'change-compilation-flags
> > +           (lambda _
> > +             (substitute* "cmake/functions.cmake"
> > +               (("-Werror") ""))
> > +             #t)))))
> 
> Why do you need to do this?
> 

Turns out we don't, I removed it

> > +    ;; See https://iroha.readthedocs.io/en/latest/build/index.html.
> > +    (native-inputs
> > +     `(("googletest" ,googletest)))
> > +    (inputs
> > +     `(("boost" ,boost-1.69)
> > +       ("fmt" ,fmt)
> > +       ("gflags" ,gflags)
> > +       ("grpc" ,grpc)
> > +       ("hyperledger-iroha-ed25519" ,hyperledger-iroha-ed25519)
> > +       ("postgresql" ,postgresql)
> > +       ("protobuf" ,protobuf)
> > +       ("rapidjson" ,rapidjson)
> > +       ("rxcpp" ,rxcpp)
> > +       ("soci" ,soci)
> > +       ("spdlog" ,spdlog)
> > +       ("tbb" ,tbb)))
> > +    (synopsis "Simple, decentralized ledger")
> > +    (description "Iroha is a distributed ledger technology (DLT). Iroha has
> 
> Double spacing.
> 
> Thanks!
> Cheers!
> 
> -- 
> Pierre Neidhardt
> https://ambrevar.xyz/



-- 
Efraim Flashner   <efraim@flashner.co.il>   אפרים פלשנר
GPG key = A28B F40C 3E55 1372 662D  14F7 41AA E7DC CA3D 8351
Confidentiality cannot be guaranteed on emails sent or received unencrypted