Replace readline library, Issue 673 #685

jmjatlanta · 2018-02-20T20:31:02Z

This is the bitshares-core portion of the Issue #673

The related bitshares-fc pull request is at bitshares/bitshares-fc#14

Please note that this is here for peer review. More testing needs to be done, especially on non-Linux platforms.

abitmore · 2018-02-20T20:43:45Z

Too many unrelated commits. Please develop on top of develop branch. Thanks!

jmjatlanta · 2018-02-20T21:19:47Z

Oops. I forgot that I had updated my develop branch with the official. Should I redo this?

abitmore · 2018-02-20T21:54:21Z

Yes, please. Rebase & push --force, or close this PR & submit a new PR.

xeroc · 2018-02-22T13:10:29Z

.gitmodules

@@ -4,5 +4,5 @@
    ignore = dirty
 [submodule "libraries/fc"]
    path = libraries/fc
-    url = https://github.com/bitshares/bitshares-fc.git
+    url = https://github.com/jmjatlanta/bitshares-fc.git


If you need modifications to bitshares-fc, please do so in the other repo and not redirect the submodules in general!

I have backed this out. It will have to be remembered that this PR must be merged with the related PR for bitshares-fc

xeroc · 2018-02-22T13:10:56Z

libraries/plugins/market_history/market_history_plugin.cpp

@@ -410,7 +410,7 @@ void market_history_plugin::plugin_set_program_options(
   )
 {
   cli.add_options()
-         ("bucket-size", boost::program_options::value<string>()->default_value("[60,300,900,1800,3600,14400,86400]"),
+         ("bucket-size", boost::program_options::value<string>()->default_value("[15,60,300,3600,86400]"),


change unrelated to readline

Grrr. Git picked up a change from oxarbitrage. I'm not sure why. It is evidently something that happened when I rebased. I've taken it out.

abitmore

Please update fc version.

jmjatlanta · 2018-03-03T22:35:39Z

cmake is expecting the libraries to already be built, and because they aren't the first time through, they are not including them. I'm working on this now.

abitmore · 2018-03-04T21:18:57Z

@jmjatlanta looks like the cli_wallet built with this patch behaves a little different in comparison to the old one, makes it a bit hard to use:

tab key doesn't work
new prompt e.g. new >>> or locked >>> doesn't show after executed a command, instead, it shows after pressed "ENTER" key.

I'm trying with Ubuntu 16.04 LTS.

jmjatlanta · 2018-03-04T22:24:54Z

Yes, @abitmore I believe that is related to my comment above. The library wasn't built, and I mistakenly had cmake look for it instead of build it. When it didn't find it, it tried to work without it, which doesn't work well. I am attempting to make it part of the build without requiring changes to the files in the repository. If that doesn't work, we'll have to fork editline.

abitmore · 2018-03-04T22:43:52Z

Oh, that may explain the tab key issue I mentioned above. But I still think the prompt issue is strange, because the prompt shows fine with the Windows binaries I built without readline, although the Windows one doesn't respond to ctrl+D but the Ubuntu-editline one does.

abitmore · 2018-03-05T00:38:17Z

I've managed to build editline in Ubuntu and linked with cli_wallet. Will submit a PR soon.

However, the behavior is still a bit different (minor issues) in comparison to the old one:

when pressing tab key,
- a good thing: it will list all available commands with the prefix inputted
- a not-so-good thing: if only one command is available, it doesn't complete the command, instead, it still lists the one command
- another thing which I don't know if it's good: if press tab key after inputted a command and a space, it will still list all commands. (update: at least it's good to do gethelp command)
  * There is no blank line after printed result of every command. (removed since it's same behavior)

At least it works.

abitmore · 2018-03-05T01:02:25Z

Submitted a PR to fc repo: bitshares/bitshares-fc#16.

Here is patch for this repo:

diff --git a/programs/cli_wallet/CMakeLists.txt b/programs/cli_wallet/CMakeLists.txt
index 852bd74..66de5f0 100644
--- a/programs/cli_wallet/CMakeLists.txt
+++ b/programs/cli_wallet/CMakeLists.txt
@@ -9,14 +9,10 @@ if( GPERFTOOLS_FOUND )
     list( APPEND PLATFORM_SPECIFIC_LIBS tcmalloc )
 endif()

-find_package( Editline )
-if ( EDITLINE_FOUND )
-       message ( STATUS "Found Editline; compiling cli_wallet with Editline" )
-       list ( APPEND PLATFORM_SPECIFIC_LIBS editline )
-       add_library (editline STATIC IMPORTED )
-       set_target_properties ( editline PROPERTIES IMPORTED_LOCATION ${Editline_LIBRARY} )
-endif()
-
+list ( APPEND PLATFORM_SPECIFIC_LIBS editline )
+ExternalProject_Get_Property(project_editline binary_dir)
+add_library(editline STATIC IMPORTED)
+set_property(TARGET editline PROPERTY IMPORTED_LOCATION ${binary_dir}/src/.libs/libeditline${CMAKE_STATIC_LIBRARY_SUFFIX})

 target_link_libraries( cli_wallet
                        PRIVATE graphene_app graphene_net graphene_chain graphene_egenesis_brief graphene_utilities graphene_wallet fc ${CMAKE_DL_LIBS} ${PLATFORM_SPECIFIC_LIBS} )

abitmore · 2018-03-05T09:12:58Z

if only one command is available, it doesn't complete the command, instead, it still lists the one command

Seems there is a bug:

if type "get_ass" then press tab key, a list contains only "get_asset" will appear;
if type "get_asse" then press tab key, the command will be completed as "get_asseaccount".

jmjatlanta · 2018-03-05T11:38:55Z

The bug and differences you mention have been fixed by a minor change to bitshares-fc. Take a look at PR bitshares/bitshares-fc#17

abitmore · 2018-03-05T23:42:58Z

The patch works fine, although it's still a bit different than the old one. For example,

type "get_ac" and press tab key, the old one will partially complete the command to "get_account", but the new one doesn't do partially completion but only list commands which are all start with "get_account";
if only one command is matched, the old client will complete with it and add a space at the end which indicates the job is done, the new client doesn't behavior like this.

abitmore · 2018-03-07T11:25:05Z

programs/cli_wallet/CMakeLists.txt

+else (MSVC)
+   add_library(editline STATIC IMPORTED)
+   set_target_properties(editline PROPERTIES
+         IMPORTED_LOCATION "${CMAKE_SOURCE_DIR}/libraries/fc/vendor/editline/src/project_editline-build/src/.libs/libeditline.a"


This is not correct. Lib file should be "BINARY_DIR" based. I think my patch mentioned in a previous comment in this PR would work better (it didn't handle MSVC though).

Sorry, I misread your patch several comments above. It is better than what I've done. I'm stealing it.

abitmore · 2018-03-07T11:27:23Z

programs/cli_wallet/CMakeLists.txt

 if(MSVC)
+  target_link_libraries( cli_wallet
+         PRIVATE graphene_app graphene_net graphene_chain graphene_egenesis_brief graphene_utilities graphene_wallet fc ${CMAKE_DL_LIBS} ${PLATFORM_SPECIFIC_LIBS} )


Please don't duplicate this line, instead, should make use of PLATFORM_SPECIFIC_LIBS. E.G.

if ( not MSVC ) list ( APPEND PLATFORM_SPECIFIC_LIBS editline ) ... endif ( not MSVC )

By the way, is it sure that editline doesn't work under Windows? If it would work, we don't need this check at all.

abitmore · 2018-03-07T11:33:41Z

programs/cli_wallet/CMakeLists.txt

+   add_library(editline STATIC IMPORTED)
+   set_target_properties(editline PROPERTIES
+         IMPORTED_LOCATION "${CMAKE_SOURCE_DIR}/libraries/fc/vendor/editline/src/project_editline-build/src/.libs/libeditline.a"
+         INTERFACE_INCLUDE_DIRECTORIES "$(CMAKE_SOURCE_DIR}/libraries/fc/vendor/editline/include")


Should not hard code "fc/vendor/editline" here since the directory structure should be decided by fc. Actually we don't need to specify this parameter here, since cli_wallet doesn't directly include the header files. Again, my patch mentioned above works.

abitmore · 2018-03-07T15:54:19Z

programs/cli_wallet/CMakeLists.txt

@@ -9,8 +9,12 @@ if( GPERFTOOLS_FOUND )
    list( APPEND PLATFORM_SPECIFIC_LIBS tcmalloc )
 endif()

+list(APPEND PLATFORM_SPECIFIC_LIBS editline)


It is no longer platform specific lib, so can remove this.

abitmore · 2018-03-08T19:11:37Z

@jmjatlanta update fc revision then I think this can be merged. Thanks a lot!

abitmore · 2018-03-09T08:20:48Z

@jmjatlanta it seems that we don't need to add editline to programs/cli_wallet/CMakeLists.txt at all, please check.

jmjatlanta · 2018-03-09T11:52:35Z

I have verified that all references to editline can be removed from the CMakeLists.txt file. The wallet compiles fine, and the editline features are included in the executable. It seems the editline library has been included as part of PLATFORM_SPECIFIC_LIBS. I am still trying to understand why and how.

pmconrad · 2018-03-09T15:30:15Z

I think cmake handles library dependencies transitively. libfc requires editline, cli_wallet requires libfc, therefore cli_wallet is linked with libeditline.

ghost · 2018-03-09T17:55:44Z

I WAS READY TO GET MY WALLET THEN THE PAGE DISAPPEARED AND I COULD NOT GET IT BACK... From: Peter Conrad <notifications@github.com> Sent: Friday, March 09, 2018 10:30 AM To: bitshares/bitshares-core <bitshares-core@noreply.github.com> Cc: Subscribed <subscribed@noreply.github.com> Subject: Re: [bitshares/bitshares-core] Replace readline library, Issue 673 (#685) I think cmake handles library dependencies transitively. libfc requires editline, cli_wallet requires libfc, therefore cli_wallet is linked with libeditline. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub<#685 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AjeX8xU0ciL-HSUi-Fre9swA5_VGQezZks5tcqAVgaJpZM4SMnk4>.

pmconrad · 2018-03-10T17:06:00Z

@serpac this is not a support forum. Please got to https://bitsharestalk.org and ask for help there.

* upgrade to latest fc * Removed references to readline library

jmjatlanta mentioned this pull request Feb 20, 2018

Replace Readline, bitshares-core issue 673 bitshares/bitshares-fc#14

Merged

jmjatlanta force-pushed the Issue_673 branch 2 times, most recently from a788336 to 81f7f78 Compare February 22, 2018 10:47

xeroc reviewed Feb 22, 2018

View reviewed changes

jmjatlanta added 3 commits February 22, 2018 11:54

Added editline project to build

345f157

Added editline submodule

38376b2

Rolled back change to .gitmodules

9efb656

jmjatlanta force-pushed the Issue_673 branch from 21e9353 to 9efb656 Compare February 22, 2018 17:02

Removed unneeded section

0465ca6

abitmore added this to the Next Non-Consensus-Changing Release milestone Feb 25, 2018

abitmore requested changes Feb 28, 2018

View reviewed changes

Switching to use the master branch of bitshares-fc

6580cbf

modified cli_wallet to include editline library if available

51ac5b0

abitmore requested changes Mar 7, 2018

View reviewed changes

reverted to patch by abitmore

f14c8bc

abitmore reviewed Mar 7, 2018

View reviewed changes

Remove editline from platform_specific_libs

3d1d863

jmjatlanta added 2 commits March 8, 2018 20:44

upgrade to latest fc

af3bc7a

latest bitshares-fc

26659fb

Removed references to editline library

e789b35

abitmore approved these changes Mar 9, 2018

View reviewed changes

abitmore merged commit 75b137e into bitshares:develop Mar 9, 2018

xeroc pushed a commit that referenced this pull request Mar 20, 2018

Replace readline library, Issue 673 (#685)

7ff57c0

* upgrade to latest fc * Removed references to readline library

xeroc pushed a commit that referenced this pull request Mar 21, 2018

Replace readline library, Issue 673 (#685)

8e0ae9c

* upgrade to latest fc * Removed references to readline library

jmjatlanta deleted the Issue_673 branch May 25, 2018 20:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace readline library, Issue 673 #685

Replace readline library, Issue 673 #685

jmjatlanta commented Feb 20, 2018 •

edited

Loading

abitmore commented Feb 20, 2018

jmjatlanta commented Feb 20, 2018 •

edited

Loading

abitmore commented Feb 20, 2018

xeroc Feb 22, 2018

jmjatlanta Feb 22, 2018 •

edited

Loading

xeroc Feb 22, 2018

jmjatlanta Feb 22, 2018 •

edited

Loading

abitmore left a comment

jmjatlanta commented Mar 3, 2018

abitmore commented Mar 4, 2018 •

edited

Loading

jmjatlanta commented Mar 4, 2018

abitmore commented Mar 4, 2018

abitmore commented Mar 5, 2018 •

edited

Loading

abitmore commented Mar 5, 2018 •

edited

Loading

abitmore commented Mar 5, 2018

jmjatlanta commented Mar 5, 2018 •

edited

Loading

abitmore commented Mar 5, 2018

abitmore Mar 7, 2018

jmjatlanta Mar 7, 2018

abitmore Mar 7, 2018

abitmore Mar 7, 2018

abitmore Mar 7, 2018

abitmore commented Mar 8, 2018

abitmore commented Mar 9, 2018

jmjatlanta commented Mar 9, 2018

pmconrad commented Mar 9, 2018

ghost commented Mar 9, 2018 via email

pmconrad commented Mar 10, 2018

Replace readline library, Issue 673 #685

Replace readline library, Issue 673 #685

Conversation

jmjatlanta commented Feb 20, 2018 • edited Loading

abitmore commented Feb 20, 2018

jmjatlanta commented Feb 20, 2018 • edited Loading

abitmore commented Feb 20, 2018

xeroc Feb 22, 2018

Choose a reason for hiding this comment

jmjatlanta Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

xeroc Feb 22, 2018

Choose a reason for hiding this comment

jmjatlanta Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

abitmore left a comment

Choose a reason for hiding this comment

jmjatlanta commented Mar 3, 2018

abitmore commented Mar 4, 2018 • edited Loading

jmjatlanta commented Mar 4, 2018

abitmore commented Mar 4, 2018

abitmore commented Mar 5, 2018 • edited Loading

abitmore commented Mar 5, 2018 • edited Loading

abitmore commented Mar 5, 2018

jmjatlanta commented Mar 5, 2018 • edited Loading

abitmore commented Mar 5, 2018

abitmore Mar 7, 2018

Choose a reason for hiding this comment

jmjatlanta Mar 7, 2018

Choose a reason for hiding this comment

abitmore Mar 7, 2018

Choose a reason for hiding this comment

abitmore Mar 7, 2018

Choose a reason for hiding this comment

abitmore Mar 7, 2018

Choose a reason for hiding this comment

abitmore commented Mar 8, 2018

abitmore commented Mar 9, 2018

jmjatlanta commented Mar 9, 2018

pmconrad commented Mar 9, 2018

ghost commented Mar 9, 2018 via email

pmconrad commented Mar 10, 2018

jmjatlanta commented Feb 20, 2018 •

edited

Loading

jmjatlanta commented Feb 20, 2018 •

edited

Loading

jmjatlanta Feb 22, 2018 •

edited

Loading

jmjatlanta Feb 22, 2018 •

edited

Loading

abitmore commented Mar 4, 2018 •

edited

Loading

abitmore commented Mar 5, 2018 •

edited

Loading

abitmore commented Mar 5, 2018 •

edited

Loading

jmjatlanta commented Mar 5, 2018 •

edited

Loading