-
Notifications
You must be signed in to change notification settings - Fork 11
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
Provide wrappers for tool binaries instead of symlink #52
Comments
I would rather we did not use proxy shell scripts. The swiftly swift install is non-standard in that the binaries are referenced from a folder outside the actual swift toolchain folder. With symbolic links it is very easy to find the true bin folder (and thus the toolchain folder) for the current installation by following the symbolic links. If we change this to wrapper shell scripts this process becomes a lot harder and more brittle as we have to parse these shell scripts and rely on them following a certain structure. Being able to reach the actual true swift installation is needed by many tools that makes use of the swift toolchain eg the VSCode swift extension. |
Sorry, I don't understand this. Could you elaborate "parse these shell scripts"? Also, from tooling perspective, it won't change anything from the current situation because they just find a proxy executable instead of a symbolic link and those two work in the same way. |
Sorry maybe I wasn't clear enough. I'll give you an example The VSCode swift extension needs to know where the LLDB lib liblldb.so is. It needs to pass it the vscode-lldb extension to ensure it uses the swift version of LLDB. If If |
Okay, I understand the problem. Thank you for elaborating clearly! However, I think avoiding proxy loses a lot of opportunities to provide practical features (e.g. select version based on IMHO, for the
|
@patrickfreed Do you have any thoughts on this? |
I think the idea of using proxies rather than simple symlinks is very interesting (and TIL rustup uses them, that was an oversight by me in the design), but I'm hesitant to introduce them until we have a specific feature in mind that we want. I think you've pointed out the two main use-cases, namely a local specification of the in-use toolchain (e.g. a Regarding the former, it was an explicit decision to omit that feature for now for and to see if it turned out to be something that's important to users. Regarding the latter, swiftly won't be as ubiquitous in the Swift ecosystem as rustup is in the Rust ecosystem, so I think introducing new syntax to the So in summary, I think its an interesting idea that we might want to consider pursuing in the future, but I'd rather let it be done as part of implementing some other user-visible feature rather than in anticipation of such a feature. I will admit that switching from symlinks to shims in a later release of swiftly will require a bit more work from us (it should be invisible to the user at least), but I think I'm okay with that. Thanks for bringing this up! |
Okay, it makes sense to me. |
@adam-fowler Typically it's the proxy/shim's job to show the toolchain being used to the user. This means with Too easy access to the toolchain installation has been and is still a big problem for Swift (especially on Linux and Windows). Ideally we need only So from my point, we can start with a real |
If apple/swift#70932 (#92) doesn't get resolved, we may need to consider doing this to ensure swiftly continues to work with the new driver. Ideally, we won't be forced to do anything here though--I agree with @adam-fowler's assessment that this really is a bug. |
The design document says:
https://github.com/swift-server/swiftly/blob/4d718c0621bdfd5835507565896ef9d28b390659/DESIGN.md#installation-of-a-swift-toolchain
But it looks like
rustup
does more things, known asProxies
, to select the active toolchain based onrust-toolchain.toml
orcargo +nightly
style at the actual command invocation time.Of course, I think it makes a lot of sense to start by limiting the toolchain selection only globally by creating symlinks at installation time (as swiftly does now).
But I'd like to raise up the future direction discussion of shims just for the record. (Sorry if it's already discussed somewhere)
The text was updated successfully, but these errors were encountered: