Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DRAFT: Improved FSW symlink handling #3550

Closed

Conversation

UffeJakobsen
Copy link
Contributor

DRAFT: Improved FSW symlink handling

This is NOT a real PR - it is only meant for discussion

In continuation of our talk on issue #3544

I've for more than 2 years used/applied this patch on top of CodeLite

This patch enables me to use CodeLite on projects that have symlinks in the path leading up to the FSW project root.

There are several problems in respect to symlinks and the use of realpath() calls that this patch approaches:

  • Clangd LSP returns realpath() paths - this can be solved with the LSP solution outlined in issue Add new macro WorkspaceRealPath #3544
  • mainbook uses realpath() extensively to figure out if a file is already opened (in another tab).
  • CodeLite Find-In-Files uses realpath() extensively (and in the end - clicking on the found file opens it)
  • etc...

This patch have worked for me without any problems for more than 2 years.

It basically fakes the use of CLRealPath() calls - on most occasions just returning the unmodified path.
An optional argument to CLRealPath() (defaults to false) can force the realpath() conversion (this is only used in very few occasions in my patch)

For compatibility reasons (and easy testing) the original behavior of CLRealPath() can be reestablished/reenabled by starting CodeLite with the environment variable CL_REALPATH defined.

eg:

CL_REALPATH=xxx codelite

I've used this handle to do extensive testing of this feature - it makes it easy to compare behavior with or without the realpath manipulation - and hence evaluate the end-user experience of the functionality of this patch.

My plan was to make a CL GUI option within the FSW project configuration for "FSW symlink/realpath handling" - so that end-users would be able to enable/disable this feature based on their project setup (just for compatibilty - as there may be some scenarios that I haven't thought of)

Anyway - I've not yet come around to make as GUI option for this - mainly because the patch as-is solves all my personal issues with FSW projects and symlinks

This is just meant to inspire you to how FSW symlink handling can easily be improved - see it as a starting-point for a discussion

At some point I hope that this patch or similar functionality can be included into CodeLite :-)

Copy link
Owner

@eranif eranif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea. Like you said, we should probably make this a configuration somewhere in the UI. Also, maybe using FileUtils which is more recent than globals.cpp

@@ -937,10 +939,14 @@ wxFileName wxReadLink(const wxFileName& filename)
#endif
}

wxString CLRealPath(const wxString& filepath) // This is readlink on steroids: it also makes-absolute, and dereferences
wxString CLRealPath(const wxString& filepath, bool force) // This is readlink on steroids: it also makes-absolute, and dereferences
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably move this logic into FileUtils

// any symlinked dirs in the path
{
return FileUtils::RealPath(filepath);
if (force || pEnv_CL_RealPath) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making this a static variable inside FileUtils makes more sense

@UffeJakobsen UffeJakobsen force-pushed the master_uj_fsw_symlink_patch branch from 5237931 to 24e6479 Compare December 21, 2024 12:05
@UffeJakobsen UffeJakobsen marked this pull request as draft December 21, 2024 12:13
@UffeJakobsen
Copy link
Contributor Author

Thanks for your feed back - good to know :-)
I'll incorporate your comments and do some more work on this patch 👍

@UffeJakobsen UffeJakobsen force-pushed the master_uj_fsw_symlink_patch branch from 24e6479 to 70e6213 Compare December 21, 2024 12:23
@UffeJakobsen
Copy link
Contributor Author

Also, maybe using FileUtils which is more recent than globals.cpp

Just a question:
I took a look at the current status of the source - and currently it seems that about 95% percent or more RealPath calls are heading for CLRealPath()

Would you prefer an implementation that calls FileUtils::RealPath() (or similar) over CLRealPath() ?

@eranif
Copy link
Owner

eranif commented Dec 21, 2024

Would you prefer an implementation that calls FileUtils::RealPath() (or similar) over CLRealPath() ?

CLRealPath was the old code, in order to avoid massive change, it was modified to route calls to FileUtils. So we can keep it around, but I would like to avoid adding new logic into it

It's current logic (before your change) is simply this:

return FileUtils::RealPath(filepath);

@UffeJakobsen UffeJakobsen force-pushed the master_uj_fsw_symlink_patch branch 2 times, most recently from 059b203 to 253422f Compare December 26, 2024 12:36
@eranif
Copy link
Owner

eranif commented Dec 27, 2024

FYI: the CI is consistently failing with missing symbols. Maybe you forgot to add a file to git?

@UffeJakobsen UffeJakobsen force-pushed the master_uj_fsw_symlink_patch branch from 253422f to 299488d Compare December 30, 2024 11:30
@UffeJakobsen
Copy link
Contributor Author

FYI: the CI is consistently failing with missing symbols. Maybe you forgot to add a file to git?

Thanks - I'll close this draft while doing rework

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants