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

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

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