search

github / vscode-codeql

An extension for Visual Studio Code that adds rich language support for CodeQL
https://marketplace.visualstudio.com/items?itemName=GitHub.vscode-codeql
MIT License
421 stars 184 forks source link

Remove handling of 8.3 filenames #3638

Closed robertbrignull closed 3 months ago

robertbrignull commented 3 months ago

See also https://github.com/github/vscode-codeql/pull/3637 which removed the tests which were failing on main. This PR removes the rest of the code for this feature.

In https://github.com/actions/runner-images/commit/5ecfd27e8fd0a1b4e8c88072772f9bbab9c82b83 the actions team disables 8.3 filename on the windows actions runners. I believe this means we no longer have to handle them because the feature was only here because these types of path sometimes get used during CI runs. See https://github.com/github/vscode-codeql/pull/3233 where the feature was introduced.

@dbartol, @aeisenberg, what do you think? Are we ok to delete this code, or do we need to keep it to handle the filenames if we do happen to encounter them? The feature is now disabled on the windows-latest actions runner image, and as far as I can tell it's also disabled for windows-2019.

Checklist

robertbrignull commented 3 months ago

I've definitely broken things with this though I'm not sure why. I think it must be because the expandShortPaths method would always make the path absolute and normalised, even if there were no short paths to expand. I'll try adding that back in.

aeisenberg commented 3 months ago

I wonder if there still are some 8.3 shenanigans going on. The test failure https://github.com/github/vscode-codeql/actions/runs/9500094833/job/26182388764?pr=3638#step:8:967 Has RUNNER~1.

From https://github.com/github/vscode-codeql/pull/3233:

The TEMP environment variable uses the 8.3 (C:\Users\runner~1) version of the path.

I'm not really sure if this is related to the error we are seeing.

dbartol commented 3 months ago

It appears that the current defaults for Windows 11 are to enable 8.3 filename generation by default for the C: (system) drive, but to disable by default for all other drives. Unfortunately, the default location for user profiles and the temp directory are on the system drive, so most users will still see an 8.3 component in their user profile path if their username is longer than 8 characters.

As much as I'd like to remove this feature in the extension, I don't think we can.

dbartol commented 3 months ago

Oh, the motivation to remove the code was that 8.3 generation is disabled on Actions runners, so we have no easy way to test it in CI ☹️. Maybe we can enable the feature explicitly, and then make sure our test uses a path that is generated after the feature is enabled?

robertbrignull commented 3 months ago

Oh, the motivation to remove the code was that 8.3 generation is disabled on Actions runners, so we have no easy way to test it in CI ☹️. Maybe we can enable the feature explicitly, and then make sure our test uses a path that is generated after the feature is enabled?

We could try that. Essentially run the code from https://github.com/actions/runner-images/commit/5ecfd27e8fd0a1b4e8c88072772f9bbab9c82b83 at the start of our windows workflows.

We can also then revert https://github.com/github/vscode-codeql/pull/3637 so this feature is being tested.

I can try doing that and see if it works.

robertbrignull commented 3 months ago

I've closed this PR in favour of https://github.com/github/vscode-codeql/pull/3643