Skip to content
This repository was archived by the owner on May 29, 2025. It is now read-only.

Conversation

@jamesrochabrun
Copy link
Contributor

@jamesrochabrun jamesrochabrun commented Mar 7, 2025

Summary

Aims to resolve #16

Tested this with current ClientChatDemo project Using Brave search

Details

Copy link
Owner

@gsabran gsabran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, some comments

let process = Process()
// In MacOS, zsh is the default since macOS Catalina 10.15.7. We can safely assume it is available.
process.launchPath = "/bin/zsh"

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we keep the initial condition? If the executable is found no need to load the env

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!


// MARK: Public

/// Creates a new `Transport` by launching the given executable with the specified arguments and attaching to its standard IO.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep?


private static func loadZshEnvironment(userEnv: [String: String]? = nil) throws -> [String: String] {
// Load shell environment as base
let shellProcess = Process()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we add userEnv to the process here, does it get merged automatically when calling printenv ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so? environment variables assigned to a process don't automatically merge with the shell's environment when running commands. I updated the code to do so, what do you think?

// Load shell environment as base
let shellProcess = Process()
shellProcess.executableURL = URL(fileURLWithPath: "/bin/zsh")
shellProcess.arguments = ["-ilc", "printenv"]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

@jamesrochabrun jamesrochabrun requested a review from gsabran March 10, 2025 06:58
Comment on lines 221 to 222
// When userEnv exists, we need to explicitly export these variables in the shell
// before running printenv so they're included in the environment output
if let userEnv = userEnv, !userEnv.isEmpty {
var exportCommands = userEnv.map { key, value in
"export \(key)=\(value.replacingOccurrences(of: "\"", with: "\\\""))"
}.joined(separator: "; ")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't shellProcess.environment = userEnv do the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disclaimer: with assistance of Claude

My understanding is that setting shellProcess.environment = userEnv wouldn't have the same effect because:

We're using zsh with -il flags which loads configuration files that might override environment variables
The explicit export commands ensure the variables are definitely set within the shell session where printenv runs
This approach guarantees the user variables appear in the printenv output, regardless of how zsh loads its environment.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't trust those AI
Screenshot 2025-03-10 at 11 07 44 AM

Copy link
Owner

@gsabran gsabran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Two nit comments. Can you rebase before merging? I simplified the linting and as there's no pre-merge checks not rebasing would lead to issues on main.

(swiftformat --config rules.swiftformat .)

@jamesrochabrun jamesrochabrun force-pushed the jroch-environment-issue branch from f996a6d to 8f03d26 Compare March 10, 2025 17:45
@jamesrochabrun jamesrochabrun requested a review from gsabran March 10, 2025 18:35
Comment on lines 218 to 228
// before running printenv so they're included in the environment output
if let userEnv, !userEnv.isEmpty {
var exportCommands = userEnv.map { key, value in
"export \(key)=\(value.replacingOccurrences(of: "\"", with: "\\\""))"
}.joined(separator: "; ")

if let path = env?.split(separator: "\n").filter({ $0.starts(with: "PATH=") }).last {
return ["PATH": String(path.dropFirst("PATH=".count))]
exportCommands += "; printenv"
shellProcess.arguments = ["-ilc", exportCommands]
} else {
shellProcess.arguments = ["-ilc", "printenv"]
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shellProcess.environment = userEnv.isEmpty ? ProcessInfo.processInfo.environment : userEnv
shellProcess.arguments = ["-ilc", "printenv"]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  private static func loadZshEnvironment(userEnv: [String: String]? = nil) throws -> [String: String] {
    // Load shell environment as base
    let shellProcess = Process()
    shellProcess.executableURL = URL(fileURLWithPath: "/bin/zsh")

    // Set process environment - either use userEnv if it exists and isn't empty, or use system environment
    if let env = userEnv, !env.isEmpty {
      shellProcess.environment = env
    } else {
      shellProcess.environment = ProcessInfo.processInfo.environment
    }

    shellProcess.arguments = ["-ilc", "printenv"]

    let outputPipe = Pipe()
    shellProcess.standardOutput = outputPipe
    shellProcess.standardError = Pipe()

    try shellProcess.run()
    shellProcess.waitUntilExit()

    let data = outputPipe.fileHandleForReading.readDataToEndOfFile()
    guard let outputString = String(data: data, encoding: .utf8) else {
      logger.error("Failed to read environment from shell.")
      return ProcessInfo.processInfo.environment
    }

    // Parse shell environment
    return outputString
      .split(separator: "\n")
      .reduce(into: [String: String]()) { result, line in
        let components = line.split(separator: "=", maxSplits: 1)
        guard components.count == 2 else { return }
        result[String(components[0])] = String(components[1])
      }
  }

@jamesrochabrun jamesrochabrun requested a review from gsabran March 12, 2025 04:31
Copy link
Owner

@gsabran gsabran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot!

@gsabran gsabran merged commit 24e83c1 into gsabran:main Mar 12, 2025
2 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The Environment Variable sent in the initializer are not added to the process.

2 participants