-
Notifications
You must be signed in to change notification settings - Fork 126
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
Improve finding executable on Windows #175
base: main
Are you sure you want to change the base?
Conversation
fe54c4c
to
740edef
Compare
06b423a
to
7a61443
Compare
66e3883
to
e4cda47
Compare
Anyone to push forward this pull request? |
@compnerd Would you be able to take a second look here? A lot of the swift-tools-support-core contributors don't have much Windows experience. Thanks! |
@swift-ci please test |
4534de9
to
0866f42
Compare
@compnerd could you review this one please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previously mentioned items are still a concern, I think that this was just rebased.
c8885fa
to
8717f9c
Compare
I've changed the logic a little bit. If you search for |
5d81333
to
5912df3
Compare
|
8f521a0
to
c85e6ef
Compare
@stevapple @compnerd we now have swift-system integrated into the main branch of TSC and can start making use of it / reduce code to help support windows paths etc better |
@tomerd I believe this PR is totally independent of the It is only blocked by review:( |
@compnerd to review @stevapple aren't there APIs in SwiftSystem that can help reduce boilerplate code here, for example the isPath computation should ideally be something that SystemSystem provides? |
If we want to reimplement this check in if let _ = FilePath.Component(value) {
// additional logic
} It's somewhat clearer, but shouldn't a blocking issue here. As I've mentioned many times before, this PR is a bug fix, while the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add more context on the motivation behind this change?
960a08c
to
8a33393
Compare
@stevapple @compnerd one thing that is not clear to me about this PR is why we are not leaning more heavily on SwiftSystem and rolling our own path utilities. aware this PR started before the integration with SwiftSystem was put in place, but now that we have it why add additional debt? cc @milseman |
I wish we could, but sadly no one has reviewed #219 so far. It is ready now, I would expect someone to give me some feedback:) |
This PR is more of some bug-fixes, and most of these points still need to be taken extra care of even with |
8a33393
to
57b0196
Compare
I recently discovered a new bug where Swift Driver and SwiftPM fail to locate themselves. There’re a bunch of bugs related to unaligned executable searching logic with Windows system. I really don’t understand why a bugfix cannot be merged, and should instead wait for unnecessary refactoring, only meant to replace a small part of its implementation detail? @tomerd @compnerd |
96664e4
to
283c833
Compare
There're three main parts of the patch:
.exe
suffix based on Windows' rule when finding executables;System32
,System
andWindows
;