Subject: Bug#1469: [X2Go-Dev] Bug#1469: But wait; there is more... patches!

From: Mihai Moldovan <[email-masked]>
Date: Mon, 25 May 2020 07:13:43 +0200

* On 5/20/20 5:04 PM, Melroy van den Berg wrote:
>> Using (std::)endl is actually a bug
> 
> Ah, thanks for the heads-up! I think in that case we should in fact remove all the endl instead of adding it :).

Yeah, probably. It creates additional newlines in the debug output which isn't
really intended. It might be a nice way to group messages to some degree, but I
figure I added most of the endls myself when I didn't know better, which was a
mistake in the past.

>> Strictly speaking, though, this should not cause any issues
> 
> Why was I getting socket error then? I really hope everything got merged correctly now. Including my changes in the separate function.
> Since all the changes together made my socket error from libssh disappear now.

Of course, I merged in most functional changes unless explicitly noted. The
issues were probably caused by the other locations, not this one specifically.
I pulled it anyway, because that's probably the more correct behavior in any case.

> I got deprecation warnings on this function/line while compiling. Why not merge it and remove build warnings?
> The change in within the #else, maybe if you think it should be in the #if you can apply the change in that section (but I think in my case the else part got compiled).
> Nevertheless, it's a deprecation that needs to be solved anyway.

It doesn't need to be solved because it *is* already solved when compiling
against newer libssh versions. The actual bug is that it seems to have used the
#else branch in your compile runs and I'm curious why. What OS have you been
using to build it and, specifically, what version of libssh-{4,dev} was installed?

Case in point, I certainly don't get a warning when building against libssh
0.9.3, which indicates that the the correct branch without the non-deprecated
function is used.

If your system has libssh >= 0.8.0 and it's taking the wrong branch, I'd like to
know what's wrong.

>> Not changing return (...) vs. return ..., the actual change
>> looks good though.
> 
> Why not changing the return(). This is wrong code, it's just a boolean! Bad practice, please don't it. Don't be afraid to change it.

That's just not true.

Up until C++14, "return (foo)" and "return foo" were functionally absolutely
equivalent and just a matter of taste/style. I like the former way better,
because it encapsulates the return value optically.

Since C++14, you're half right that it's wrong, because both statements MIGHT
behave differently. This, however, is only the case for return values that
explicitly use decltype (auto).

We're not using any C++14 (or even C++11) features anyway and probably won't do
so for a longer time, because we also target legacy systems without such
support. In the end, it is personal preference here.

> A generic remark: I think X2Go is missing a good pipeline with testcases and
> other quality checks.

Yes, it does, and that's arguably bad. We've been discussing this every single
year, but that's a mammoth task.

> Which also hopefully increases your *trust in the code*
> and enables refactoring as well.
> I'm not afraid to refactor the code and clean-up the formatting, splitting
> functions and even into multiple files. If this all improve readability,
> debuggability and test-ability long-term.

I always avoid changing stuff for no good reason. While proper version control
systems like git take out some of the complexity that is involved by, e.g.,
formatting updates, it still comes at a price that cannot be automatically
handled by the tools.

I like to git blame a lot to see how portions of code changed, but each
non-functional change adds another layer of indirection to the process, which
can make it very frustrating.

Most software projects I came across handle it the same way and will tell you to
please split out non-functional changes or to just remove them completely.

> That is why I raise a request to create a decent pipeline to allow the necessary
> changes in further improve code maturity and the needed changes to do so.
> And maybe even a better diff tool to perform refactoring changes during review.
> 
> Any ideas or suggestions? I'm running a GitLab instance myself for example;
> which enables DevOps and CI/CD within all my projects.
> Thanks once more!

We do have a public Jenkins instance for CI at https://jenkins.x2go.org .

Testing is really a badly missing thing, but we try to test things in a manual
manner especially before a release if time permits, but naturally that's not a
substitute for, e.g., unit tests.

Completely automatic testing of GUI stuff is extremely difficult, though.

Maybe someday, but I won't and cannot promise it. I'm already stumped with so
much other stuff that regularly breaks or needs to be updated/improved and is
arguably more important (because the best testing infrastructure won't help you
if all the other things don't work).

Mihai