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.

Sign up for GitHub

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

Jump to bottom

enable static linking of the swift runtime libraries by default on supported platforms #3905

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

enable static linking of the swift runtime libraries by default on supported platforms #3905

wants to merge 5 commits into from
+195 −72

Conversation

tomerd
Copy link
Member

@tomerd tomerd commented Dec 3, 2021 •
edited

motivation: on some platforms (eg Linux) statically linking the swift runtime libraries makes more sense to allow better mobility of execuables

changes:

  • deprecate the opt-in "static-swift-stdlib" CLI flag
  • introduce an opt-out "disable-static-swift-runtime" CLI flag
  • change behavior to automatically build/link with "-static-stdlib" on supported platforms (Linux and WASI for now)
  • adjust tests

@tomerd tomerd changed the title enable static linking of the swift runtime libraries by default on su… enable static linking of the swift runtime libraries by default on supported platforms Dec 3, 2021
@tomerd
Copy link
Member Author

tomerd commented Dec 3, 2021

@neonichu @abertelrud @ktoso @weissi @compnerd @MaxDesiatov following on the discussion from https://forums.swift.org/t/pre-pitch-statically-linking-the-swift-runtime-libraries-by-default-on-linux/ this is a draft implementation. does this resonate with you? if so, I will make a formal pitch with a link to this PR as the impl

@tomerd tomerd force-pushed the feature/static-link-stdlib-linux branch 2 times, most recently from 89f8989 to cb4a28b Compare December 3, 2021 04:34
@MaxDesiatov
Copy link
Member

cc @kateinoigakukun

@tomerd tomerd self-assigned this Jan 13, 2022
@tomerd tomerd force-pushed the feature/static-link-stdlib-linux branch from 0e90fd4 to 45d8915 Compare February 14, 2022 19:25
tomerd
tomerd commented Feb 14, 2022
View reviewed changes
Tests/BuildTests/BuildPlanTests.swift
let testPathExtension = testBuildDescription.binary.extension
XCTAssertEqual(testPathExtension, "wasm")
}

Copy link
Member Author

Choose a reason for hiding this comment

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

@MaxDesiatov one of the key pieces of feedback to this proposal was that this should only apply in release mod (since if you are trying to debug you would need to have the toolchain installed by definition).

ptal at the changes to the WASI tests: expanded them to cover both debug and release modes

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, thanks!

tomerd
tomerd commented Feb 14, 2022
View reviewed changes
Tests/BuildTests/BuildPlanTests.swift
@@ -1959,7 +1952,7 @@ final class BuildPlanTests: XCTestCase {
XCTAssertMatch(executablePathExtension, "exe")
}

func testWASITarget() throws {
func testWASITargetDebug() throws {
Copy link
Member Author

Choose a reason for hiding this comment

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

@MaxDesiatov

tomerd
tomerd commented Feb 14, 2022
View reviewed changes
Tests/BuildTests/BuildPlanTests.swift
@@ -2049,6 +2041,94 @@ final class BuildPlanTests: XCTestCase {
XCTAssertEqual(testPathExtension, "wasm")
}

func testWASITargetRelease() throws {
Copy link
Member Author

Choose a reason for hiding this comment

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

@MaxDesiatov

@tomerd tomerd force-pushed the feature/static-link-stdlib-linux branch from 45d8915 to dd03aca Compare February 14, 2022 19:29
@tomerd
Copy link
Member Author

tomerd commented Feb 14, 2022

@swift-ci please smoke test

@tomerd tomerd added the ready Author believes the PR is ready to be merged & any feedback has been addressed label Feb 14, 2022
@tomerd
Copy link
Member Author

tomerd commented Feb 15, 2022

@swift-ci please smoke test

ktoso
ktoso approved these changes Feb 15, 2022
View reviewed changes
Copy link
Member

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

Wohoo :)

@tomerd
Copy link
Member Author

tomerd commented Feb 15, 2022

@swift-ci please smoke test

1 similar comment
@tomerd
Copy link
Member Author

tomerd commented Feb 17, 2022

@swift-ci please smoke test

@tomerd
Copy link
Member Author

tomerd commented Feb 24, 2022

@swift-ci please smoke test Linux

@tomerd
Copy link
Member Author

tomerd commented Feb 24, 2022

@swift-ci please smoke test

@tomerd tomerd force-pushed the feature/static-link-stdlib-linux branch from 50cbd94 to cda15a6 Compare March 1, 2022 01:02
@tomerd
Copy link
Member Author

tomerd commented Mar 1, 2022

@swift-ci please smoke test linux

@tomerd tomerd force-pushed the feature/static-link-stdlib-linux branch from 0a439b9 to 20cb939 Compare March 2, 2022 00:40
@tomerd
Copy link
Member Author

tomerd commented Mar 2, 2022

@swift-ci please smoke test

Kyle-Ye
Kyle-Ye reviewed Mar 7, 2022
View reviewed changes
Utilities/Docker/Dockerfile Outdated Show resolved Hide resolved
@tomerd tomerd force-pushed the feature/static-link-stdlib-linux branch from 20cb939 to 47b7691 Compare March 22, 2022 23:33
@tomerd
Copy link
Member Author

tomerd commented Mar 22, 2022

@swift-ci smoke test

@tomerd tomerd force-pushed the feature/static-link-stdlib-linux branch from 47b7691 to a2d598e Compare March 22, 2022 23:35
@tomerd
Copy link
Member Author

tomerd commented Mar 22, 2022

@swift-ci smoke test

@tomerd
enable static linking of the swift runtime libraries by default on su…
f613192 
…pported platforms

motivation: on some platforms (eg Linux) statically linking the swift runtime libraries makes more sense to allow better mobility of execuables

changes:
* deprecate the opt-in "static-swift-stdlib" CLI flag
* introduce an opt-out "disable-static-swift-runtime" CLI flag
* change behavior to automatically build/link with "-static-stdlib" in release mode on supported platforms (Linux and WASI)
* adjust tests
@tomerd
fixup
345b263
@tomerd
fixup
0787cae
@tomerd
fixup
e4afd4d
@tomerd tomerd force-pushed the feature/static-link-stdlib-linux branch from a2d598e to e4afd4d Compare March 25, 2022 23:30
@tomerd
Copy link
Member Author

tomerd commented Mar 25, 2022

@swift-ci please smoke test

tomerd
tomerd commented Mar 26, 2022
View reviewed changes
Utilities/Docker/Dockerfile Outdated Show resolved Hide resolved
@neonichu neonichu added next waiting for next merge window and removed pending evolution proposal ready Author believes the PR is ready to be merged & any feedback has been addressed labels Apr 11, 2022
@tomerd
Copy link
Member Author

tomerd commented Apr 12, 2022

@swift-ci please smoke test

@MaxDesiatov MaxDesiatov mentioned this pull request May 12, 2022
Closed
@kateinoigakukun kateinoigakukun mentioned this pull request Jun 14, 2022
Open
@MaxDesiatov MaxDesiatov added the WIP Work in progress label Oct 26, 2022
@tomerd
Copy link
Member Author

tomerd commented Oct 28, 2022

cc @MaxDesiatov

@MaxDesiatov MaxDesiatov assigned MaxDesiatov and unassigned tomerd Oct 28, 2022
@rauhul
Copy link
Contributor

rauhul commented Jan 14, 2024

This evolution proposal was accepted ~2 years ago; are we still planning on merging this change?

@MaxDesiatov
Copy link
Member

Yes, but the PR needs to be updated with conflicts resolved, tests enabled/fixed etc.

@kateinoigakukun kateinoigakukun mentioned this pull request Jan 19, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaxDesiatov MaxDesiatov MaxDesiatov left review comments

@Kyle-Ye Kyle-Ye Kyle-Ye left review comments

@neonichu neonichu neonichu approved these changes

@ktoso ktoso ktoso approved these changes

@abertelrud abertelrud Awaiting requested review from abertelrud

@elsh elsh Awaiting requested review from elsh

@compnerd compnerd Awaiting requested review from compnerd

Assignees

@MaxDesiatov MaxDesiatov

Labels
next waiting for next merge window WIP Work in progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@tomerd @MaxDesiatov @rauhul @neonichu @ktoso @Kyle-Ye