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

Provide wrappers for tool binaries instead of symlink #52

Open
kateinoigakukun opened this issue Apr 12, 2023 · 10 comments
Open

Provide wrappers for tool binaries instead of symlink #52

kateinoigakukun opened this issue Apr 12, 2023 · 10 comments

Comments

@kateinoigakukun
Copy link
Contributor

The design document says:

The ~/.local/bin directory would include symlinks pointing to the bin directory of the "active" toolchain, if any.
This is all very similar to how rustup does things, but I figure there's no need to reinvent the wheel here.

https://github.com/swift-server/swiftly/blob/4d718c0621bdfd5835507565896ef9d28b390659/DESIGN.md#installation-of-a-swift-toolchain

But it looks like rustup does more things, known as Proxies, to select the active toolchain based on rust-toolchain.toml or cargo +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)

@adam-fowler
Copy link
Member

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.

@kateinoigakukun
Copy link
Contributor Author

kateinoigakukun commented Apr 13, 2023

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.

Sorry, I don't understand this. Could you elaborate "parse these shell scripts"?
As far as I know, having a wrapper for the initial launch of actual swift command won't be a problem as long as the actual binary path is not moved outside of the toolchain's usr/bin because all toolchain resources like stdlib swiftmodule/library binary/tools are resolved relatively from the driver executable file's path.

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.

@adam-fowler
Copy link
Member

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 ~/.local/bin/swift is a symbolic link I can follow the link to find the actual location of the swift executable and use this information alongside knowledge of the structure of the toolchain to get the location of the LLDB lib.

If ~/.local/bin/swift is a proxy (shell script or whatever) I do not have information about the real location of the swift executable.

@kateinoigakukun
Copy link
Contributor Author

kateinoigakukun commented Apr 14, 2023

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 .swift-version file, select a specific version locally (not globally) )

IMHO, for the liblldb issue, the proper way would be to ask swift command to provide the canonical toolchain directory by something like swift --print-toolchain-path instead of assuming the given swift executable should be or point the canonical toolchain binary. Note that the driver doesn't have the option now but we can have some workarounds for now by getting the current executable (lldb) by REPL mode (I think there are still some other ways):

$ echo "print(CommandLine.arguments[0])" | swift repl 2> /dev/null
/home/katei/.local/share/swiftly/toolchains/main-snapshot-2022-06-08/usr/bin/repl_swift

@kateinoigakukun
Copy link
Contributor Author

@patrickfreed Do you have any thoughts on this?

@patrickfreed
Copy link
Collaborator

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 .swift-version file or some such) or some alternative one-off syntax for selecting a version (similar to +nightly in cargo).

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 swift executable might not be the best idea (you can imagine users writing scripts that work with swiftly-installed toolchains but not manually downloaded ones). We might also run into issues supporting such syntax on macOS. Instead, I think it might be better to introduce a new command in swiftly itself for that kind of functionality (see #22), which would mean that shims wouldn't be required.

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!

@kateinoigakukun
Copy link
Contributor Author

Okay, it makes sense to me.

@stevapple
Copy link

@adam-fowler Typically it's the proxy/shim's job to show the toolchain being used to the user. This means with swiftly shim being adopted the VSCode extension can use something like swiftly show toolchain-path to get the real path if it really needs to.


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 swift and swiftc in $PATH for daily use, and should be able to use Swift with nothing in $PATH. The DEB and RPM packages already did it and it's appealing to make it the new default for packaging (including on Windows, where symlinks are not capable and a shim could be the only choice).

So from my point, we can start with a real bin/ directory with swift and swiftc symlinks pointing to the tools (or pointing to swift-driver directly). This is not so complicated but would largely ease the transition to using a shim.

@patrickfreed
Copy link
Collaborator

patrickfreed commented Feb 14, 2024

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.

@kkebo
Copy link
Contributor

kkebo commented Apr 25, 2024

FYI

apple/swift-driver#1583

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

No branches or pull requests

5 participants