CLion reporter has limitations

[claremacrae]

Sometimes the CLion diff reporter on Mac seems to show up behind the main CLion IDE window.

Also, it requires the CLion command line wrapper script to have been installed - more information at Shell scripts generated by the Toolbox App.

It also requires 'clion' to be in the user's path.

[alepez]

In my Arch Linux, CLion reporter (detected as the first working) is giving an error:

The dialog in the image above is a CLion dialog. This is happening because clion nosplash diff is being called, while clion.sh diff should be called (in my system it is installed by pacman package manager at /opt/clion/bin/clion.sh).

In Compare files from the command line JetBrains suggest to call clion.sh instead of clion on Linux systems.

With this change, CLion diff reporter is working on my Linux system:

diff --git a/ApprovalTests/reporters/DiffPrograms.cpp b/ApprovalTests/reporters/DiffPrograms.cpp
index 2cff7e7f..3a6fdd49 100644
--- a/ApprovalTests/reporters/DiffPrograms.cpp
+++ b/ApprovalTests/reporters/DiffPrograms.cpp
@@ -68,8 +68,8 @@ namespace ApprovalTests
                          Type::TEXT))

             APPROVAL_TESTS_MACROS_ENTRY(CLION,
-                                        DiffInfo("clion",
-                                                 "nosplash diff {Received} {Approved}",
+                                        DiffInfo("/opt/clion/bin/clion.sh",
+                                                 "diff {Received} {Approved}",
                                                  Type::TEXT))
         }

/opt/clion/bin/clion.sh can be replaced with clion.sh if /opt/clion/bin/ is in the PATH variable.

It seems that we have to add a new Linux::CLionDiffReporter, looking for clion.sh instead of clion. The problem here is that the Mac::CLionDiffReporter is detected as working by FirstWorkingReporter, because clion is a valid command.

[claremacrae]

Hi Alessandro,

Thanks for reporting this....

/opt/clion/bin/clion.sh can be replaced with clion.sh if /opt/clion/bin/ is in the PATH variable.

It seems that we have to add a new Linux::CLionDiffReporter, looking for clion.sh instead of clion.

Yes, good idea to add Linux::CLionDiffReporter...

The problem here is that the Mac::CLionDiffReporter is detected as working by FirstWorkingReporter, because clion is a valid command.

That's a good point...

This suggests to me that DiffReporter.cpp should add the native/host platform first...

that is, Mac first if running on macOS, Windows first if on Windows etc...

I can't immediately see a clean way to implement that, but it does make sense...

[alepez]

Another problem with CLion on Linux: it is killed before the window appears. Fun fact: if a breakpoint is hit after launching the diff reporter, the window is still visible. But if you just run it (normal run or debug without breakpoints) the window get killed immediately.

Quick fix:

diff --git a/ApprovalTests/launchers/SystemLauncher.cpp b/ApprovalTests/launchers/SystemLauncher.cpp
index f0fffca8..db9ab2ee 100644
--- a/ApprovalTests/launchers/SystemLauncher.cpp
+++ b/ApprovalTests/launchers/SystemLauncher.cpp
@@ -50,7 +50,7 @@ namespace ApprovalTests
     std::string SystemLauncher::getUnixCommandLine(const std::string& commandLine,
                                                    bool foreground) const
     {
-        std::string launch = foreground ? commandLine : (commandLine + " &");
+        std::string launch = foreground ? commandLine : (commandLine + " 1>/dev/null 2>/dev/null </dev/null &");

         return launch;
     }

[claremacrae]

Thanks @alepez - do you understand what causes it to be killed? Why does all that redirecting of input and output prevent it from being killed?

And does this depend on the earlier patch?

[alepez]

I suspect it is related to stdin/stdout/stderr pipes being closed. I'm using CLion reporter from CLion.

[alepez]

With 1>/dev/null 2>/dev/null </dev/null & we are forcing the child process to not be bound to the parent pipes.

[claremacrae]

Hi @alepez - we think that the Mac CLion reporter should not now trigger on Linux...

Could you possibly confirm that when you have time...

There is a separate question of whether to add Linux::CLionReporter (or however it is spelled), and fixing the command line args...

Would you actually like to use CLion's diff reporter (on Linux?)

[alepez]

Hi @claremacrae, I confirm. Now CLionReporter does not trigger (now meld triggers instead).

About adding CLion diff reporter... I'm not sure. Other free tools are similar or better. CLion has also another problem (in Linux, at least). If there are two tests with differences, Approvals launches two clion.sh diff instances (and this is ok). The problem is with the second instance: it seems that a full CLion instance is launched and even the splash screen appears.

[claremacrae]

Hi @alepez - I hadn't realised just how broken the CLion reporter is on Linux...

I agree, let's not do any more work on CLion for Linux then...

[claremacrae]

Closing the issue - no plan to fix it. If the CLion diff tool ends up working better on Linux, we can re-visit...