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

Fix path failure preventing trace from opening #184

Merged

Conversation

dleclairbb
Copy link
Contributor

@dleclairbb dleclairbb commented Oct 25, 2023

When doing some testing on an internal extension, I noticed that we could no longer use the Open With TraceViewer command on Windows due to an invalid path failing (worked on Linux).

I was able to trace the cause of this it to some code that uses .path instead of .fsPath, which is intended for use with the filesystem and scrubs out improper path values (which is what caused our error!) I also fixed this in other areas of the code.

This fixes a critical issue which prevents users from opening traces in some cases.

@ngondalia
Copy link
Contributor

ngondalia commented Oct 25, 2023

@PatrickTasse @bhufmann please review asap. Opening traces using the 'traces.openTraceFile' command directly does not work on windows without this patch! Thank you :)

@dleclairbb dleclairbb force-pushed the fix-trace-paths-windows branch from cb950a6 to 32f2b75 Compare October 26, 2023 16:18
Copy link
Collaborator

@bhufmann bhufmann left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the fix!

I tested it on Linux and it looks good. However, the commit message has a reference to what I believe some bug report tool "QVSC-760: fix basename calculation". Could you please remove it?

When doing some testing on an internal extension relying, I
noticed that we could no longer use the Open With TraceViewer
command on Windows due to an invalid path failing (worked on Linux).

I was able to trace the cause of this it to some code that uses
.path instead of .fsPath, which is intended for use with the
filesystem and scrubs out improper path values  (which is
what caused our error!). I also changed some other path calculations to
be less error prone across platforms by using the path API.

This fixes a critical issue which prevents users from opening traces
in some cases.

Signed-off-by: Dylan Leclair <[email protected]>
@dleclairbb dleclairbb force-pushed the fix-trace-paths-windows branch from 32f2b75 to 84e4944 Compare October 27, 2023 20:31
@dleclairbb
Copy link
Contributor Author

I must have missed that when I was squashing the other changes in - sorry about that! Thank you :)

@dleclairbb dleclairbb requested a review from bhufmann October 27, 2023 20:45
Copy link
Contributor

@marcdumais-work marcdumais-work left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for this contribution!
Note: I do not have a Windows machine to confirm the bug previous to this contribution, and that it's now fixed. Tested on Linux and the changes seem to work fine still.

Copy link
Collaborator

@bhufmann bhufmann left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for the fix.

@bhufmann bhufmann merged commit f0df578 into eclipse-cdt-cloud:master Oct 27, 2023
3 checks passed
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.

5 participants