Skip to content

Simplify env.fish#508

Open
kkebo wants to merge 2 commits into
swiftlang:mainfrom
kkebo:refactor-env-fish
Open

Simplify env.fish#508
kkebo wants to merge 2 commits into
swiftlang:mainfrom
kkebo:refactor-env-fish

Conversation

@kkebo

@kkebo kkebo commented Feb 21, 2026

Copy link
Copy Markdown
Member

Summary

The current env.fish feels a bit complicated. I refactored it with fish's built-in commands.

The fish_add_path options I used:

  • -m: Move already-included directories to the place they would be added - by default they would be left in place and not added again.
  • -p (default): Add directories to the front of the variable (this is the default).
  • -P: Manipulate $PATH directly.

refs: https://fishshell.com/docs/current/cmds/fish_add_path.html

Test

I tested the generated env.fish locally with several PATH configurations, including cases where SWIFTLY_BIN_DIR is absent, already at the beginning, in the middle, or appears multiple times. I didn't commit this test since it's too platform-dependent and requires that all shells are installed.

    @Test(
        .testHome(),
        arguments: ["/bin/bash", "/bin/zsh", "/bin/fish"],
        [
            (
                path: ["/foo", "/bar"],
                expectedPath: ["/swiftly/bin", "/foo", "/bar"]
            ),
            (
                path: ["/swiftly/bin", "/foo", "/bar"],
                expectedPath: ["/swiftly/bin", "/foo", "/bar"]
            ),
            (
                path: ["/foo", "/swiftly/bin", "/bar"],
                expectedPath: ["/swiftly/bin", "/foo", "/bar"]
            ),
            (
                path: ["/foo", "/swiftly/bin", "/bar", "/swiftly/bin"],
                expectedPath: ["/swiftly/bin", "/foo", "/bar"]
            ),
        ]
    )
    func envPrependsSwiftlyBinDirAndRemovesDuplicates(
        shell: String,
        pathCase: (path: [String], expectedPath: [String])
    ) async throws {
        // GIVEN: a fresh user account without swiftly installed
        try? await fs.remove(atPath: Swiftly.currentPlatform.swiftlyConfigFile(SwiftlyTests.ctx))

        // AND: the user is using the bash shell
        var ctx = SwiftlyTests.ctx
        ctx.mockedShell = shell

        try await SwiftlyTests.$ctx.withValue(ctx) {
            let envScript: FilePath?
            if shell.hasSuffix("bash") || shell.hasSuffix("zsh") {
                envScript = Swiftly.currentPlatform.swiftlyHomeDir(SwiftlyTests.ctx) / "env.sh"
            } else if shell.hasSuffix("fish") {
                envScript = Swiftly.currentPlatform.swiftlyHomeDir(SwiftlyTests.ctx) / "env.fish"
            } else {
                envScript = nil
            }

            if let envScript {
                #expect(!(try await fs.exists(atPath: envScript)))
            }

            // WHEN: swiftly is invoked to init the user account and finish swiftly installation
            try await SwiftlyTests.runCommand(Init.self, ["init", "--assume-yes", "--skip-install"])

            // THEN: env script modifies $PATH correctly
            let swiftlyHomeDir = Swiftly.currentPlatform.swiftlyHomeDir(SwiftlyTests.ctx)
            let swiftlyBinDir = Swiftly.currentPlatform.swiftlyBinDir(SwiftlyTests.ctx)

            func replaceSwiftlyBinDir(in path: [String]) -> [String] {
                path.map { $0 == "/swiftly/bin" ? swiftlyBinDir.string : $0 }
            }

            let inputPath = replaceSwiftlyBinDir(in: pathCase.path)
            let expectedPath = replaceSwiftlyBinDir(in: pathCase.expectedPath)

            if let envScript {
                let command = if shell.hasSuffix("fish") {
                    """
                    set -gx PATH \(inputPath.joined(separator: " "))
                    source \(envScript.string)
                    string join : $PATH
                    """
                } else {
                    """
                    PATH='\(inputPath.joined(separator: ":"))'
                    . '\(envScript.string)'
                    printf '%s' "$PATH"
                    """
                }

                let result = try await Subprocess.run(
                    .path(FilePath(shell)),
                    arguments: ["-c", command],
                    output: .string(limit: 1024)
                )

                #expect(result.terminationStatus.isSuccess)

                let actualPath = result.standardOutput?
                    .replacing(/\n$/, with: "")
                    .split(separator: ":")
                    .map(String.init)

                #expect(actualPath == expectedPath)
            }
        }
    }
$ swift test --filter InitTests.envPrependsSwiftlyBinDirAndRemovesDuplicates
...
✔ Test envPrependsSwiftlyBinDirAndRemovesDuplicates(shell:pathCase:) with 12 test cases passed after 0.190 seconds.
✔ Suite InitTests passed after 0.191 seconds.
✔ Test run with 1 test in 1 suite passed after 0.191 seconds.

Environment:

$ uname -a
Linux Raspberry-beetle 6.18.34+rpt-rpi-2712 #1 SMP PREEMPT Debian 1:6.18.34-1+rpt1 (2026-06-09) aarch64 GNU/Linux
$ fish --version
fish, version 4.8.0
$ zsh --version
zsh 5.9 (aarch64-unknown-linux-gnu)
$ bash --version
GNU bash, version 5.2.37(1)-release (aarch64-unknown-linux-gnu)
Copyright (C) 2022 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>

This is free software; you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

@kkebo kkebo force-pushed the refactor-env-fish branch from 31bb16a to b6b28df Compare July 15, 2026 15:11
@kkebo

kkebo commented Jul 15, 2026

Copy link
Copy Markdown
Member Author

I understand that my script is not equivalent to the old one with the following test.

    @Test(
        .testHome(),
        arguments: ["/bin/bash", "/bin/zsh", "/bin/fish"],
        [
            (
                path: ["/foo", "/bar"],
                expectedPath: ["/swiftly/bin", "/foo", "/bar"]
            ),
            (
                path: ["/swiftly/bin", "/foo", "/bar"],
                expectedPath: ["/swiftly/bin", "/foo", "/bar"]
            ),
            (
                path: ["/foo", "/swiftly/bin", "/bar"],
                expectedPath: ["/swiftly/bin", "/foo", "/bar"]
            ),
            (
                path: ["/foo", "/swiftly/bin", "/bar", "/swiftly/bin"],
                expectedPath: ["/swiftly/bin", "/foo", "/bar"]
            ),
        ]
    )
    func envPrependsSwiftlyBinDirAndRemovesDuplicates(
        shell: String,
        pathCase: (path: [String], expectedPath: [String])
    ) async throws {
        // GIVEN: a fresh user account without swiftly installed
        try? await fs.remove(atPath: Swiftly.currentPlatform.swiftlyConfigFile(SwiftlyTests.ctx))

        // AND: the user is using the bash shell
        var ctx = SwiftlyTests.ctx
        ctx.mockedShell = shell

        try await SwiftlyTests.$ctx.withValue(ctx) {
            let envScript: FilePath?
            if shell.hasSuffix("bash") || shell.hasSuffix("zsh") {
                envScript = Swiftly.currentPlatform.swiftlyHomeDir(SwiftlyTests.ctx) / "env.sh"
            } else if shell.hasSuffix("fish") {
                envScript = Swiftly.currentPlatform.swiftlyHomeDir(SwiftlyTests.ctx) / "env.fish"
            } else {
                envScript = nil
            }

            if let envScript {
                #expect(!(try await fs.exists(atPath: envScript)))
            }

            // WHEN: swiftly is invoked to init the user account and finish swiftly installation
            try await SwiftlyTests.runCommand(Init.self, ["init", "--assume-yes", "--skip-install"])

            // THEN: env script modifies $PATH correctly
            let swiftlyHomeDir = Swiftly.currentPlatform.swiftlyHomeDir(SwiftlyTests.ctx)
            let swiftlyBinDir = Swiftly.currentPlatform.swiftlyBinDir(SwiftlyTests.ctx)

            func replaceSwiftlyBinDir(in path: [String]) -> [String] {
                path.map { $0 == "/swiftly/bin" ? swiftlyBinDir.string : $0 }
            }

            let inputPath = replaceSwiftlyBinDir(in: pathCase.path)
            let expectedPath = replaceSwiftlyBinDir(in: pathCase.expectedPath)

            if let envScript {
                let command = if shell.hasSuffix("fish") {
                    """
                    set -gx PATH \(inputPath.joined(separator: " "))
                    source \(envScript.string)
                    string join : $PATH
                    """
                } else {
                    """
                    PATH='\(inputPath.joined(separator: ":"))'
                    . '\(envScript.string)'
                    printf '%s' "$PATH"
                    """
                }

                let result = try await Subprocess.run(
                    .path(FilePath(shell)),
                    arguments: ["-c", command],
                    output: .string(limit: 1024)
                )

                #expect(result.terminationStatus.isSuccess)

                let actualPath = result.standardOutput?
                    .replacing(/\n$/, with: "")
                    .split(separator: ":")
                    .map(String.init)

                #expect(actualPath == expectedPath)
            }
        }
    }
$ swift test --filter InitTests.envPrependsSwiftlyBinDirAndRemovesDuplicates
...
✘ Test envPrependsSwiftlyBinDirAndRemovesDuplicates(shell:pathCase:) recorded an issue with 2 arguments shell → "/bin/fish", pathCase → (path: ["/foo", "/swiftly/bin", "/bar", "/swiftly/bin"], expectedPath: ["/swiftly/bin", "/foo", "/bar"]) at InitTests.swift:217:17: Expectation failed: (actualPath → ["/tmp/swiftly-tests-testHome-3D0FC3FE-2A7A-4D3A-898B-2F26120A3FF6/bin", "/foo", "/bar", "/tmp/swiftly-tests-testHome-3D0FC3FE-2A7A-4D3A-898B-2F26120A3FF6/bin"]) == (expectedPath → ["/tmp/swiftly-tests-testHome-3D0FC3FE-2A7A-4D3A-898B-2F26120A3FF6/bin", "/foo", "/bar"])
✘ Test envPrependsSwiftlyBinDirAndRemovesDuplicates(shell:pathCase:) with 12 test cases failed after 0.185 seconds with 1 issue.
✘ Suite InitTests failed after 0.186 seconds with 1 issue.
✘ Test run with 1 test in 1 suite failed after 0.186 seconds with 1 issue.

This is because fish_add_path -mP does not remove all existing duplicates.

@kkebo

kkebo commented Jul 15, 2026

Copy link
Copy Markdown
Member Author

Fixed: 881c1ac (this PR)

@kkebo

kkebo commented Jul 15, 2026

Copy link
Copy Markdown
Member Author

I tested the generated env.fish locally with several PATH configurations, including cases where SWIFTLY_BIN_DIR is absent, already at the beginning, in the middle, or appears multiple times. The behavior was unchanged with this refactoring. I didn't commit this test since it's too platform-dependent and requires that all shells are installed.

    @Test(
        .testHome(),
        arguments: ["/bin/bash", "/bin/zsh", "/bin/fish"],
        [
            (
                path: ["/foo", "/bar"],
                expectedPath: ["/swiftly/bin", "/foo", "/bar"]
            ),
            (
                path: ["/swiftly/bin", "/foo", "/bar"],
                expectedPath: ["/swiftly/bin", "/foo", "/bar"]
            ),
            (
                path: ["/foo", "/swiftly/bin", "/bar"],
                expectedPath: ["/swiftly/bin", "/foo", "/bar"]
            ),
            (
                path: ["/foo", "/swiftly/bin", "/bar", "/swiftly/bin"],
                expectedPath: ["/swiftly/bin", "/foo", "/bar"]
            ),
        ]
    )
    func envPrependsSwiftlyBinDirAndRemovesDuplicates(
        shell: String,
        pathCase: (path: [String], expectedPath: [String])
    ) async throws {
        // GIVEN: a fresh user account without swiftly installed
        try? await fs.remove(atPath: Swiftly.currentPlatform.swiftlyConfigFile(SwiftlyTests.ctx))

        // AND: the user is using the bash shell
        var ctx = SwiftlyTests.ctx
        ctx.mockedShell = shell

        try await SwiftlyTests.$ctx.withValue(ctx) {
            let envScript: FilePath?
            if shell.hasSuffix("bash") || shell.hasSuffix("zsh") {
                envScript = Swiftly.currentPlatform.swiftlyHomeDir(SwiftlyTests.ctx) / "env.sh"
            } else if shell.hasSuffix("fish") {
                envScript = Swiftly.currentPlatform.swiftlyHomeDir(SwiftlyTests.ctx) / "env.fish"
            } else {
                envScript = nil
            }

            if let envScript {
                #expect(!(try await fs.exists(atPath: envScript)))
            }

            // WHEN: swiftly is invoked to init the user account and finish swiftly installation
            try await SwiftlyTests.runCommand(Init.self, ["init", "--assume-yes", "--skip-install"])

            // THEN: env script modifies $PATH correctly
            let swiftlyHomeDir = Swiftly.currentPlatform.swiftlyHomeDir(SwiftlyTests.ctx)
            let swiftlyBinDir = Swiftly.currentPlatform.swiftlyBinDir(SwiftlyTests.ctx)

            func replaceSwiftlyBinDir(in path: [String]) -> [String] {
                path.map { $0 == "/swiftly/bin" ? swiftlyBinDir.string : $0 }
            }

            let inputPath = replaceSwiftlyBinDir(in: pathCase.path)
            let expectedPath = replaceSwiftlyBinDir(in: pathCase.expectedPath)

            if let envScript {
                let command = if shell.hasSuffix("fish") {
                    """
                    set -gx PATH \(inputPath.joined(separator: " "))
                    source \(envScript.string)
                    string join : $PATH
                    """
                } else {
                    """
                    PATH='\(inputPath.joined(separator: ":"))'
                    . '\(envScript.string)'
                    printf '%s' "$PATH"
                    """
                }

                let result = try await Subprocess.run(
                    .path(FilePath(shell)),
                    arguments: ["-c", command],
                    output: .string(limit: 1024)
                )

                #expect(result.terminationStatus.isSuccess)

                let actualPath = result.standardOutput?
                    .replacing(/\n$/, with: "")
                    .split(separator: ":")
                    .map(String.init)

                #expect(actualPath == expectedPath)
            }
        }
    }
$ swift test --filter InitTests.envPrependsSwiftlyBinDirAndRemovesDuplicates
...
✔ Test envPrependsSwiftlyBinDirAndRemovesDuplicates(shell:pathCase:) with 12 test cases passed after 0.190 seconds.
✔ Suite InitTests passed after 0.191 seconds.
✔ Test run with 1 test in 1 suite passed after 0.191 seconds.

Environment:

$ uname -a
Linux Raspberry-beetle 6.18.34+rpt-rpi-2712 #1 SMP PREEMPT Debian 1:6.18.34-1+rpt1 (2026-06-09) aarch64 GNU/Linux
$ fish --version
fish, version 4.8.0
$ zsh --version
zsh 5.9 (aarch64-unknown-linux-gnu)
$ bash --version
GNU bash, version 5.2.37(1)-release (aarch64-unknown-linux-gnu)
Copyright (C) 2022 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>

This is free software; you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

@cmcgee1024 Since you originally worked on this logic, would you mind taking a look when you have a chance? I’d especially appreciate your thoughts on whether there are any edge cases I may have missed.

@kkebo kkebo force-pushed the refactor-env-fish branch from 881c1ac to ea49a9f Compare July 15, 2026 16:39
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

Successfully merging this pull request may close these issues.

1 participant