[verible-patch-tool] color the diffs/hunks presented to the user

fangism commented 4 years ago

For the underlying patch-tool described here: https://github.com/google/verible#interactive-formatting

It would be nice if the diff could be colorized for readability. Currently it is monochrome.

Example mock-up using (github markdown's default coloring):

$ verible-verilog-format-changed-lines-interactive.sh 
--- /home/fangism/local/src/lowRISC/opentitan/hw/ip/aes/rtl/aes_core.sv
+++ NEW/home/fangism/local/src/lowRISC/opentitan/hw/ip/aes/rtl/aes_core.sv
@@ -32,3 +32,3 @@
   // Signals
-  logic                 foo_bar;
+  logic foo_bar;
   logic                 ctrl_qe;
Apply this hunk? [y,n,a,d,s,q,?] n
@@ -107,3 +107,3 @@

-      logic   illogical;
+  logic illogical;

Apply this hunk? [y,n,a,d,s,q,?] n

Would also color the prompt Apply this hunk? a little differently like this (dark-themed):

color-patch-tool

Task:

Detect when running in interactive mode, e.g. using isatty(), and only apply colors in this case.
- Possibly detect terminal type as well.
Make sure existing tests still work (ok that they all run non-interactively)

fangism commented 4 years ago

b/161930379

DhairyaBahl commented 4 years ago

@fangism I would like to work on this issue.

Thanks :)

fangism commented 4 years ago

Thank you.

fangism commented 3 years ago

@DhairyaBahl had any time to look into this?

DhairyaBahl commented 3 years ago

@fangism I am extremely sorry for delay towards the issue. I will try my best to push a PR in a day or so.

Thank you :) sorry again :)

DhairyaBahl commented 3 years ago

@fangism Work Update: I am trying to understand how verible works, about its various features to resolve this issue and to first understand the issue. I get that i have to change the color of the text or make it look more interactive but i am confused that how to change the colordiff if its installed by default in the system by the apt-get install commands. Kindly guide me a bit to how to approach this problem.

Thanks a lot for having patience towards me :)

fangism commented 3 years ago

The patch-tool is a small piece of the Verible project that is generic and only operates on patch files.

The top-level main file is https://cs.opensource.google/verible/verible/+/master:common/tools/patch_tool.cc You only need to look downwards from here.

To build and run this:

bazel build -c opt //common/tools:verible-patch-tool
./bazel-bin/common/tools/verible-patch-tool args...
verible-patch-tool help apply-pick for usage

A simpler demo (do all of the following from the same dir):

create two files that differ by a few lines
diff -u file1 file2 > patch
verible-patch-tool apply-pick < patch and follow the prompts. Say n to decline all changes, and see the interface described in the initial comment.

So let's start small, say only colorize the prompt: Apply this hunk? [y,n,a,d,s,q,?] n

That prompt is done here.

As for colorization itself, which is done by printing control characters to the terminal, see if there's a simple way to do it without introducing a library. You might research source code for other tools that colorize output. Do you they use a common library? or do their own thing? e.g.

https://clang.llvm.org/docs/UsersManual.html (look at color-diagnostics)

Maybe report back with a few options before coding up an implementation.

In the worst case, we may make a tiny colorize library for ourselves.

DhairyaBahl commented 3 years ago

@fangism I am trying to build my verible-patch-tool but it is showing following error : ERROR: Analysis of target '//common/tools:verible-patch-tool' failed; build aborted: Unable to find flex binary

I thought this error was occuring because output_user_root contains spaces so i passed commands with different output_user_root="" commands but everytime it says unable to find flex binary

Kindly guide me further, now I am moving to the passive part i.e. understand the codebase and other things u mentioned in the previous message.

Thanks :)

fangism commented 3 years ago

@fangism I am trying to build my verible-patch-tool but it is showing following error : ERROR: Analysis of target '//common/tools:verible-patch-tool' failed; build aborted: Unable to find flex binary

I thought this error was occuring because output_user_root contains spaces so i passed commands with different output_user_root="" commands but everytime it says unable to find flex binary

Kindly guide me further, now I am moving to the passive part i.e. understand the codebase and other things u mentioned in the previous message.

Thanks :)

Strange, bazel is supposed to know how to fetch/build/install flex for you automatically. verible-patch-tool itself doesn't depend on flex, but other parts of verible do.

I wonder if this due to locally installed flex being an indirect dependency through Kythe. See all of its dependencies here https://kythe.io/getting-started/.

Try to sudo apt-get install flex and any other missing tool errors from that list.

@ivan444 in case you have any other suggestions.

DhairyaBahl commented 3 years ago

Strange, bazel is supposed to know how to fetch/build/install flex for you automatically. verible-patch-tool itself doesn't depend on flex, but other parts of verible do. I would like to mention that i was getting flex error even in the case of patch tool only

Try to sudo apt-get install flex and any other missing tool errors from that list.

If i forgot to mention i would like to tell, I am on windows. Could that be an issue ?

DhairyaBahl commented 3 years ago

@fangism So do i have to install all kythe dependencies individually or there is any other option to build the patch tool.

after installing the flex it is now giving this error: ERROR: Analysis of target '//common/tools:verible-patch-tool' failed; build aborted: empty string

Thanks:)

fangism commented 3 years ago

@fangism So do i have to install all kythe dependencies individually or there is any other option to build the patch tool.

after installing the flex it is now giving this error: ERROR: Analysis of target '//common/tools:verible-patch-tool' failed; build aborted: empty string

Thanks:)

After installing flex is the flex binary in your PATH? Can you run flex --help?

Is there any other clue where "empty string" is coming from?

I unfortunately have no experience building on Windows, but at least one contribution came from someone using Windows in the past. You might have to reach out to one of the bazel users mailing lists for help.

fangism commented 3 years ago

According to these files: https://cs.opensource.google/kythe/kythe/+/master:tools/build_rules/lexyacc/BUILD https://cs.opensource.google/kythe/kythe/+/master:tools/build_rules/lexyacc/lexyacc.bzl

It's looking for /usr/bin/flex or FLEX from environment variables, and similar for /usr/bin/bison as YACC. Can you try something like FLEX=/path/to/your/installed/flex YACC=/path/to/your/bison bazel build ...? (or whatever the equivalent of that is on Windows)

DhairyaBahl commented 3 years ago

After installing flex is the flex binary in your PATH? Can you run flex --help?

Yes I can run flex --help it is showing other possible flags like --version

Is there any other clue where "empty string" is coming from?

I have no idea, i am working for the first time on bazel itself

I unfortunately have no experience building on Windows, but at least one contribution came from someone using Windows in the past. You might have to reach out to one of the bazel users mailing lists for help.

Okay, I guess I should join the mailing list now !!

@fangism Thanks

DhairyaBahl commented 3 years ago

According to these files: https://cs.opensource.google/kythe/kythe/+/master:tools/build_rules/lexyacc/BUILD https://cs.opensource.google/kythe/kythe/+/master:tools/build_rules/lexyacc/lexyacc.bzl

It's looking for /usr/bin/flex or FLEX from environment variables, and similar for /usr/bin/bison as YACC. Can you try something like FLEX=/path/to/your/installed/flex YACC=/path/to/your/bison bazel build ...? (or whatever the equivalent of that is on Windows)

I have set the environment variables for the system so that they can find flex. however I've also made it available for the mingw64 in its local bin directory !!

Should environment variable still be an issue ??

@fangism Thanks

fangism commented 3 years ago

I don't really know myself, unfortunately. The kythe project I referenced is at https://github.com/kythe/kythe -- they might have some idea about to control the locations of flex/bison.

I'm trying to think if there's a way for you to locally patch out the kythe dependency, because you don't need it for this task. Could you try commenting out lines 143-to-end of https://cs.opensource.google/verible/verible/+/master:WORKSPACE;drc=a0d9948afb99b17f9246031434d1c4f59971ac0d;l=143 and see if bazel build -c opt //common/tools:verible-patch-tool is able to proceed?

DhairyaBahl commented 3 years ago

@fangism I tried and it worked, but now its showing new erro:

The target you are compiling requires Visual C++ build tools. Bazel couldn't find a valid Visual C++ build tools installation on your machine.

Visual C++ build tools seems to be installed at C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC But Bazel can't find the following tools: VCVARSALL.BAT, cl.exe, link.exe, lib.exe, ml64.exe for x64 target architecture

Please check your installation following https://docs.bazel.build/versions/master/windows.html#using

I guess, i should reinstall the Microsoft Visual studio package again or only the missing files if i can !! and should read the documentation linked by the bazel and try to fix it myself.

Thanks :)

fangism commented 3 years ago

Ok, that's progress, I'm glad there some documentation to guide you.

ivan444 commented 3 years ago

We'll have to do something about those Kythe dependencies. The introduced complexity is not appropriate.

I'll address it together with reducing the c++ build requirements (lowering it from c++17 to c++11).

DhairyaBahl commented 3 years ago

@fangism bazel needs some tools for which i have to install latest version of C++ redistributable build tools 2019 but my pc doesn't support it. It only support till 2015 version. What should i do ?? Is there any alternative to bazel ?? or any other way to build without using bazel then kindly tell me.

Thanks

fangism commented 3 years ago

At this time we can only support bazel building, I'm afraid. Thanks for trying. If you're still stuck, feel free to un-assign yourself from issues, we totally understand.

DhairyaBahl commented 3 years ago

@fangism I will try one more time to install this package with bazel, if I failed then I will unassign myself :)

DhairyaBahl commented 3 years ago

@fangism I am extremely sorry for inconvenience. I won't be able to solve this issue, because I am unable to solve the problem of bazel for the time-being. Thank you for your time and effort :)

fangism commented 3 years ago

@fangism I am extremely sorry for inconvenience. I won't be able to solve this issue, because I am unable to solve the problem of bazel for the time-being. Thank you for your time and effort :)

@DhairyaBahl Thanks for the update, and thanks again for trying. Some day our build support for VS will improve.

chipsalliance / verible

[verible-patch-tool] color the diffs/hunks presented to the user #526