fix/perf: don't call language detection command in enabled()

[lnu]

Prerequisites

• I have read and understand the CONTRIBUTING guide
• The commit message follows the conventional commits guidelines
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

Description

Currently, language version detection is done in the enabled() function, which is called in any directory. This call can be avoided by moving the call to get the version inside the string() method. All tests are green and only two had to be modified but not in a way that changes the current behaviour.

[lnu]

I'm also aligning the dotnet segment like other languages segment

[JanDeDobbeleer]

@lnu I suppose you're still working on this, right?

[lnu]

@lnu I suppose you're still working on this, right?

yes, I put it in draft. It's almost done but there's one thing about the dotnet segment with exit code 145. I'm not able to test it, the tests are ok but I'd like to test it a once manually.

[lnu]

I think I found something related to the perf issues when calling executables on windows.
Before git segment was taking 350ms, now it takes 228ms. Not blazing fast but 35% is still nice.
It also applies to other segments calling windows executables.
We always call hasCommand, then call runCommand. Since the command are not fully qualified, a call to LookPath is done, checking all paths to check if the executable exists. By caching the executable full path on the first hasCommand call, we can avoid calling this method multiple times.

[JanDeDobbeleer]

I think I found something related to the perf issues when calling executables on windows.

Before git segment was taking 350ms, now it takes 228ms. Not blazing fast but 35% is still nice.

It also applies to other segments calling windows executables.

We always call hasCommand, then call runCommand. Since the command are not fully qualified, a call to LookPath is done, checking all paths to check if the executable exists. By caching the executable full path on the first hasCommand call, we can avoid calling this method multiple times.

Awesome. That's something I didn't check indeed. I have a working rust lib which is faster on Windows when it comes to executing commands. So that combination could be even faster.

[JanDeDobbeleer]

@lnu let me know when this is ready to go.

[lnu]

@lnu let me know when this is ready to go.

For me it looks fine. But it needs some testing about the languages segment(especially the dotnet one) and a second pair of eyes would be useful.
For the dotnet segment,I have no idea how to reproduce the exit code 145 scenario. Has someone seen it working before? it seems this code is returned when you do a dotnet restore but I saw nothing related to that code and dotnet --version.

The only difference with the previous behaviour is that hasCommand is called instead of getVersion to check if the segment should be enabled or not.

[lnu]

found info here: golang/go#38736

[JanDeDobbeleer]

I have no idea how to reproduce the exit code 145 scenario. Has someone seen it working before?

The original author did. So we'd have to check with @tillig :-)

[lnu]

I have no idea how to reproduce the exit code 145 scenario. Has someone seen it working before?

The original author did. So we'd have to check with @tillig :-)

I tried to test it by using a global.json config file with an sdk not installed on my machine:

{
    "sdk": {
      "version": "3.1.100",
      "rollForward": "disable"
    }
}

Here are the results:
Main

Main with runCommand fix

fix/segment_language(current branch)

You can see there's already an issue with the current main branch. A change has been done when trying to optimize runCommand but we loste the exitCode in the meantime. I put the old code back and it looks ok now with acceptable performance.

I also changed to the dotnet segment to return the unsupported whenever an exit code != 0 is returned by the command. It seems ok to me, maybe @tillig can confirm?

[tillig]

145 is actually special for dotnet. There are other exit codes you can get, like if you call the command incorrectly or if something else is messed up in the environment. I'd recommend keeping the dotnet segment checking specifically for 145 to indicate "unsupported version" and considering any other non-zero code to be a more severe unrecoverable error and return "" instead.

[lnu]

145 is actually special for dotnet. There are other exit codes you can get, like if you call the command incorrectly or if something else is messed up in the environment. I'd recommend keeping the dotnet segment checking specifically for 145 to indicate "unsupported version" and considering any other non-zero code to be a more severe unrecoverable error and return "" instead.

Thank you @tillig. Could you tell me how I could test this? As said above, I put a global.json with a not existing sdk and it returns this:

I have these sdks installed:

And this global.json:

{
    "sdk": {
      "version": "3.1.100",
      "rollForward": "disable"
    }
}

[JanDeDobbeleer]

@lnu you might want to rebase too, while testing the new experimental Rust lib, I found a bug in the current implementation of runCommand where the error did not contain the actual error output.

[lnu]

@lnu you might want to rebase too, while testing the new experimental Rust lib, I found a bug in the current implementation of runCommand where the error did not contain the actual error output.

Yep. I saw the change but I think we still don't get the right exit code from the command(error output is ok though).
The actual code(in main) of the dotnet segment does this:

// Exit code 145 is a special indicator that dotnet
// ran, but the current project config settings specify
// use of an SDK that isn't installed.
var exerr *commandError
if errors.As(err, &exerr) && exerr.exitCode == 145 {
    d.unsupportedVersion = true
    return true
}

But runCommand does not return the exit code but only the error output. I wonder if the original code(withoutput) was slower than the current change?

[tillig]

@lnu The bad global.json is how you test it. That should yield both the info you saw and a 145 exit code.

[JanDeDobbeleer]

But runCommand does not return the exit code but only the error output

True. I'll align it with command error.

[lnu]

@lnu The bad global.json is how you test it. That should yield both the info you saw and a 145 exit code.

Ok then it seems something has changed, it returns this:

[tillig]

Unfortunately, I'm on vacation without any real access to a dev environment for another week. Right now I'm on my phone. This isn't something I'll be able to check or help troubleshoot before then.

[lnu]

But runCommand does not return the exit code but only the error output

True. I'll align it with command error.

Now the exit code is there but processtate ==nil and it always return -1 and not the real exit code of the external command.

[lnu]

Unfortunately, I'm on vacation without any real access to a dev environment for another week. Right now I'm on my phone. This isn't something I'll be able to check or help troubleshoot before then.

You've been already very helpful. Since I kept the exitCode check and all the tests are green, everything should be ok.
Happy new year and all the best!

[JanDeDobbeleer]

not the real exit code of the external command

I'll check what does contain the value, I did this rather swift in between taking care of the kids. I can have a look somewhere around lunch.

[lnu]

not the real exit code of the external command

I'll check what does contain the value, I did this rather swift in between taking care of the kids. I can have a look somewhere around lunch.

I think it's not so urgent :), it can wait until 2021.

Happy new year and all the best for you and your family.

[lnu]

Ok, I tried in ubuntu and yes the exit code is correct(you can see the issued with exit code on the first screenshot):

with the fixed zsh script:

So I confirm it works(with the old runCommand version calling output) well in Ubuntu, same as before. We only have to fix the runCommand and the dotnet segment will be ok.
We're getting closer 👍

[JanDeDobbeleer]

@lnu found a fix, validated it too. This should hopefully make this ready to merge. Would be cool if we can do this before OEY 😓

[lnu]

@lnu found a fix, validated it too. This should hopefully make this ready to merge. Would be cool if we can do this before OEY 😓

Cool! Sounds like a deadline? 😅

[lnu]

@lnu found a fix, validated it too. This should hopefully make this ready to merge. Would be cool if we can do this before OEY 😓

Ok so I rebased everything and tested. The exit code is ok and the perf are better. Still, the dotnet segment works correctly under ubuntu but not in windows. But it was already the case before, so we still have the same behaviour.

[JanDeDobbeleer]

Ill do some testing on this layer today too, that way we can merge with confidence.

[JanDeDobbeleer]

@lnu This works fine, I would just remove DisplayModeNever as that's a bit pointless. It's the same as not adding the segment. But I'll remove that rather than ask you to once again rebase :-)

[lnu]

Ill do some testing on this layer today too, that way we can merge with confidence.

@lnu This works fine, I would just remove DisplayModeNever as that's a bit pointless. It's the same as not adding the segment. But I'll remove that rather than ask you to once again rebase :-)

True, no idea what's the point of never displaying the segment. It's also harmless to let it now and clean it later.