Skip to content

feat: expand SWIFTFLAGS with full paths#74

Merged
soumyamahunt merged 2 commits into
SwiftyLab:mainfrom
stevapple:expand-flags
Sep 15, 2024
Merged

feat: expand SWIFTFLAGS with full paths#74
soumyamahunt merged 2 commits into
SwiftyLab:mainfrom
stevapple:expand-flags

Conversation

@stevapple

Copy link
Copy Markdown
Contributor

This will ensure compatibility with both CMD and PowerShell.

@codecov

codecov Bot commented Sep 26, 2023

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.79%. Comparing base (b939b08) to head (c371ddb).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #74   +/-   ##
=======================================
  Coverage   92.79%   92.79%           
=======================================
  Files          34       34           
  Lines        1082     1083    +1     
  Branches      157      157           
=======================================
+ Hits         1004     1005    +1     
  Misses         77       77           
  Partials        1        1           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment thread src/installer/windows/index.ts Outdated
Comment thread src/installer/windows/index.ts Outdated
@soumyamahunt

Copy link
Copy Markdown
Contributor

@stevapple is it possible to add validation of this feature in CI?

@stevapple

Copy link
Copy Markdown
Contributor Author

@stevapple is it possible to add validation of this feature in CI?

I think this environment variable is not quite useful for modern Swift on Windows toolchains, and its only current usage is for LLDB and REPL, which are uncommon for CI/CD.

Since it's already guaranteed by unit testing, I think it's okay...?

@soumyamahunt

Copy link
Copy Markdown
Contributor

@stevapple is it possible to add validation of this feature in CI?

I think this environment variable is not quite useful for modern Swift on Windows toolchains, and its only current usage is for LLDB and REPL, which are uncommon for CI/CD.

Since it's already guaranteed by unit testing, I think it's okay...?

While LLDB probably would never be used in CI, some may use REPL. Adding a simple hello world in integration test should be fine.

swift -e "print(\"Hello from REPL\")"

@stevapple

Copy link
Copy Markdown
Contributor Author

While LLDB probably would never be used in CI, some may use REPL. Adding a simple hello world in integration test should be fine.

swift -e "print(\"Hello from REPL\")"

Well, the interpreter had never worked as expected on Windows. These flags are mostly legacy companions for swiftc on older Swift releases. We should actually set it only for Swift 5.6 and older versions.

@stevapple

stevapple commented Sep 27, 2023

Copy link
Copy Markdown
Contributor Author

We should actually set it only for Swift 5.6 and older versions.

Doing this (as well as eliminating support file setup mentioned in #72) requires us to pass/parse the installed version in the post-installation process, which would be explored in a later patch.

@soumyamahunt

Copy link
Copy Markdown
Contributor

We should actually set it only for Swift 5.6 and older versions.

Doing this (as well as eliminating support file setup mentioned in #72) requires us to pass/parse the installed version in the post-installation process, which would be explored in a later patch.

Please add comment here as well, so it's not missed

@soumyamahunt

Copy link
Copy Markdown
Contributor

@stevapple can you rebase this PR, I am thinking of adding this as part of next release.

@stevapple

Copy link
Copy Markdown
Contributor Author

@stevapple can you rebase this PR, I am thinking of adding this as part of next release.

Done.

@soumyamahunt

Copy link
Copy Markdown
Contributor

@stevapple thanks for the quick response, can you add following data to SETUPSWIFT_CUSTOM_TOOLCHAINS variables in your fork:

{
    "ubuntu2204": "https://github.com/swiftwasm/swift/releases/download/swift-wasm-5.10-SNAPSHOT-2024-03-30-a/swift-wasm-5.10-SNAPSHOT-2024-03-30-a-ubuntu22.04_x86_64.tar.gz",
    "xcode": "https://github.com/swiftwasm/swift/releases/download/swift-wasm-5.10-SNAPSHOT-2024-03-30-a/swift-wasm-5.10-SNAPSHOT-2024-03-30-a-macos_x86_64.pkg",
    "windows10": ""
}
Screenshot 2024-04-04 at 6 50 37 PM

The CI expects this data to test custom toolchain installations, and hence failing since not present.

@soumyamahunt soumyamahunt enabled auto-merge (squash) September 15, 2024 16:37
@soumyamahunt soumyamahunt merged commit 8573d06 into SwiftyLab:main Sep 15, 2024
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.

2 participants