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

Improve finding executable on Windows #175

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

stevapple
Copy link

@stevapple stevapple commented Dec 21, 2020

There're three main parts of the patch:

  • Automatically add .exe suffix based on Windows' rule when finding executables;
  • Add default Windows search paths: System32, System and Windows;
  • Fix unexpected failure when dealing with empty path strings.

Sources/TSCBasic/Process.swift Outdated Show resolved Hide resolved
Sources/TSCBasic/Process.swift Outdated Show resolved Hide resolved
Sources/TSCBasic/misc.swift Outdated Show resolved Hide resolved
Sources/TSCBasic/misc.swift Outdated Show resolved Hide resolved
@stevapple stevapple requested a review from compnerd December 23, 2020 12:21
@stevapple stevapple force-pushed the windows-executable branch 2 times, most recently from 66e3883 to e4cda47 Compare December 23, 2020 12:32
@stevapple
Copy link
Author

Anyone to push forward this pull request?

@abertelrud
Copy link
Contributor

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!

@tomerd
Copy link
Contributor

tomerd commented Jan 10, 2021

@swift-ci please test

@stevapple stevapple requested a review from tomerd as a code owner December 6, 2021 11:18
@tomerd tomerd requested review from compnerd and removed request for neonichu, tomerd, friedbunny, aciidgh and abertelrud December 6, 2021 17:53
@tomerd
Copy link
Contributor

tomerd commented Dec 6, 2021

@compnerd could you review this one please

Copy link
Member

@compnerd compnerd left a 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.

Sources/TSCBasic/Process.swift Outdated Show resolved Hide resolved
@stevapple stevapple force-pushed the windows-executable branch 2 times, most recently from c8885fa to 8717f9c Compare December 7, 2021 06:26
@stevapple
Copy link
Author

I've changed the logic a little bit.

If you search for clang, it will now first look for exactly a clang in to search paths. If nothing was found, then search for clang.exe instead.

@stevapple stevapple force-pushed the windows-executable branch 2 times, most recently from 5d81333 to 5912df3 Compare December 7, 2021 22:34
@stevapple
Copy link
Author

ProcessTests are still failing either because they’re not tailored for Windows, or because supporting functions (like withCustomEnv) are broken. I’d like to address them in another PR.

@stevapple stevapple force-pushed the windows-executable branch 2 times, most recently from 8f521a0 to c85e6ef Compare December 8, 2021 00:22
@tomerd
Copy link
Contributor

tomerd commented Feb 2, 2022

@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 tomerd added the wip Work in progress label Mar 2, 2022
@stevapple
Copy link
Author

@tomerd I believe this PR is totally independent of the System integration work, and provides the correct behavior on Windows.

It is only blocked by review:(

@tomerd
Copy link
Contributor

tomerd commented Mar 9, 2022

@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?

@stevapple
Copy link
Author

@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 FilePath, then it should look like this:

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 FilePath integration is more of a kind of improvement. They should have different priorities, and actually have no dependency on each other.

Copy link
Member

@compnerd compnerd left a 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?

Sources/TSCBasic/Process.swift Show resolved Hide resolved
Sources/TSCBasic/misc.swift Outdated Show resolved Hide resolved
@stevapple stevapple requested a review from compnerd March 16, 2022 03:22
Sources/TSCBasic/misc.swift Outdated Show resolved Hide resolved
@stevapple stevapple force-pushed the windows-executable branch from 960a08c to 8a33393 Compare March 16, 2022 03:32
@tomerd
Copy link
Contributor

tomerd commented Mar 16, 2022

@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

@stevapple
Copy link
Author

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:)

@stevapple
Copy link
Author

aware this PR started before the integration with SwiftSystem was put in place, but now that we have it why add additional debt?

This PR is more of some bug-fixes, and most of these points still need to be taken extra care of even with System integration at this time. So I would like to land this first, and then replace part of it along with the other System stuffs.

@stevapple stevapple force-pushed the windows-executable branch from 8a33393 to 57b0196 Compare April 22, 2022 06:26
@stevapple
Copy link
Author

stevapple commented Apr 22, 2022

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

@stevapple stevapple force-pushed the windows-executable branch from 96664e4 to 283c833 Compare April 22, 2022 06:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wip Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants