Extend freestanding to support targeting Darwin platforms #40202

edymtt · 2021-11-16T18:49:55Z

add an option to add freestanding to the Darwin platform, so that
to get expected compile behaviours (e.g. setting the install name)
rework testing configuration to relax assumptions about freestanding
add a preset to test such configuration (at least for PR testing)

Addresses rdar://85465396

edymtt · 2021-11-16T18:51:14Z

@swift-ci please python lint

edymtt · 2021-11-16T18:51:39Z

preset=stdlib_S_standalone_minimal_macho_x86_64,build,test
@swift-ci please test with toolchain and preset

edymtt · 2021-11-16T18:52:25Z

preset=stdlib_S_standalone_darwin_x86_64,build,test
@swift-ci please test with toolchain and preset

compnerd · 2021-11-16T18:53:00Z

stdlib/cmake/modules/AddSwiftStdlib.cmake

@@ -1685,6 +1685,7 @@ function(add_swift_target_library name)
    set(SWIFTLIB_TARGET_SDKS ${SWIFT_SDKS})
  endif()
  list_replace(SWIFTLIB_TARGET_SDKS ALL_APPLE_PLATFORMS "${SWIFT_DARWIN_PLATFORMS}")
+  list(REMOVE_DUPLICATES SWIFTLIB_TARGET_SDKS)


Why is this part of this change? Is this because adding the FREESTANDING now results in duplicates?

Yes -- in particular FREESTANDING could be present in SWIFT_DARWIN_PLATFORMS and as an explicit target when building the Darwin overlay (as a result of https://github.com/apple/swift/blob/78809726080ce61a4f73c013d542c91d5da35f2e/stdlib/public/Platform/CMakeLists.txt#L21)

I could have added the deduplication in the file above, but this seemed something that could happen in other instances as well.

This needs a nice big comment here.

@edymtt is it possible to instead of adding in this deduplication, add in an assert that we never have duplicates and change the other code?

For now I addressed the issue in Platform/CMakeLists.txt (a8d57e2)

Adding the assert seems to require some changes to precondition_binary_op which may require further discussion a bit outside of this PR -- if time allows I will tackle that in another PR.

swift-ci · 2021-11-16T19:11:27Z

Build failed
Swift Test OS X Platform
Git Sha - 6fff7df049079ffe25226753472916bcb9f10fb7

test/lit.cfg

gottesmm · 2021-11-16T21:18:21Z

test/lit.cfg

@@ -1155,7 +1165,10 @@ if run_vendor == 'apple':
        'Running tests on %s version %s (%s)' %
        (sw_vers_name, sw_vers_vers, sw_vers_build))

-    config.target_sdk_name = xcrun_sdk_name
+    if "-freestanding" in config.variant_suffix and config.swift_freestanding_is_darwin:
+        config.target_sdk_name = "freestanding"


Can you add a comment here explaining why you are doing this.

Added a comment in bbde1af, also pulled the value from CMake in an attempt to show where that value comes from.

test/lit.site.cfg.in

validation-test/lit.site.cfg.in

edymtt · 2021-11-17T19:56:02Z

preset=stdlib_S_standalone_minimal_macho_x86_64,build,test
@swift-ci please test with toolchain and preset

swift-ci · 2021-11-17T20:38:17Z

Build failed
Swift Test OS X Platform
Git Sha - c843efb09df08b5e7dd4e3471a21c224fd4de893

edymtt · 2021-11-17T21:29:28Z

******************** TEST 'Swift(freestanding-x86_64) :: stdlib/symbols-depended-on-in-freestanding.darwin.test-sh' FAILED ********************
<snip>
Error: Unexpected dependency '_fflush'

This happens on main as well -- https://ci.swift.org/job/oss-swift-test-stdlib-with-toolchain-minimal/843/console

edymtt · 2021-11-17T21:29:54Z

preset=stdlib_S_standalone_darwin_x86_64,build,test
@swift-ci please test with toolchain and preset

* add an option to add freestanding to the Darwin platform, so that to get expected compile behaviours (e.g. setting the install name) * rework testing configuration to relax assumptions about freestanding * add a preset to test such configuration (at least for PR testing) Addresses rdar://85465396

Drop deduplication of such lists so to surface configuration errors earlier.

Add some more comments as well

Also take the chance to rename a function for clarity

edymtt · 2021-12-06T17:38:22Z

@swift-ci please python lint

edymtt · 2021-12-06T17:39:41Z

preset=stdlib_S_standalone_minimal_macho_x86_64,build,test
@swift-ci please test with toolchain and preset

edymtt · 2021-12-06T17:39:59Z

Testing the minimal preset before testing the one introduced with this PR

edymtt · 2021-12-06T18:23:55Z

The minimal preset succeeded, https://ci.swift.org/job/swift-PR-stdlib-with-toolchain-osx-preset/71/

edymtt · 2021-12-06T18:24:06Z

preset=stdlib_S_standalone_darwin_x86_64,build,test
@swift-ci please test with toolchain and preset

edymtt · 2021-12-06T18:41:13Z

@swift-ci please smoke test

edymtt · 2021-12-06T18:41:23Z

@swift-ci please test Apple Silicon

edymtt · 2021-12-06T18:41:48Z

@swift-ci please build toolchain

swift-ci · 2021-12-06T20:50:24Z

Linux Toolchain (Ubuntu 16.04)
Download Toolchain
Git Sha - 194fb21

Install command
tar zxf swift-PR-40202-739-ubuntu16.04.tar.gz
More info

swift-ci · 2021-12-07T01:32:27Z

macOS Toolchain
Download Toolchain
Git Sha - 194fb21

Install command
tar -zxf swift-PR-40202-1234-osx.tar.gz --directory ~/

gottesmm · 2021-12-07T21:43:27Z

test/lit.cfg

@@ -972,6 +980,8 @@ if run_vendor == 'apple':
        symlink_if_not_exists("clang", "clang")
        symlink_if_not_exists("shims", "shims")
        symlink_if_not_exists("freestanding", "macosx")
+        if config.swift_freestanding_is_darwin:


Can you add a comment here explaining why we are doing this?

gottesmm · 2021-12-07T21:44:09Z

test/lit.cfg


-        swift_execution_tests_extra_flags += ' -experimental-hermetic-seal-at-link -lto=llvm-full'
+        if not config.swift_freestanding_is_darwin:


I think this is fine for now, but the hermetic seal at link thing feels like it should have its own feature option sort of thing.

But that being said, I think this is good incremental work. Maybe put in a TODO.

gottesmm

LGTM. Can you maybe add some comments in a follow on commit.

Tackles feedback of the review of swiftlang#40202

edymtt requested review from compnerd, gottesmm and kubamracek November 16, 2021 18:49

edymtt force-pushed the extend-freestanding-to-darwin branch from da1374d to 6fff7df Compare November 16, 2021 18:51

compnerd reviewed Nov 16, 2021

View reviewed changes

gottesmm reviewed Nov 16, 2021

View reviewed changes

test/lit.cfg Outdated Show resolved Hide resolved

gottesmm reviewed Nov 16, 2021

View reviewed changes

test/lit.site.cfg.in Outdated Show resolved Hide resolved

gottesmm reviewed Nov 16, 2021

View reviewed changes

validation-test/lit.site.cfg.in Outdated Show resolved Hide resolved

edymtt added 5 commits December 6, 2021 07:44

Remove unnecessary parenthesis

3fb78aa

Normalize CMake flags passes to lit.site.cfg.in

3465354

Fix root cause of duplicated SDK targets

a8d57e2

Drop deduplication of such lists so to surface configuration errors earlier.

Pull sdk name for freestanding from CMake

bbde1af

Add some more comments as well

edymtt force-pushed the extend-freestanding-to-darwin branch from c843efb to bbde1af Compare December 6, 2021 16:17

Reimplement test for new parameter as Python test

194fb21

Also take the chance to rename a function for clarity

gottesmm reviewed Dec 7, 2021

View reviewed changes

gottesmm approved these changes Dec 7, 2021

View reviewed changes

edymtt merged commit 403ccd4 into swiftlang:main Dec 8, 2021

edymtt added a commit to edymtt/swift that referenced this pull request Dec 8, 2021

Add explanatory comments around freestanding test configuration

0e01344

Tackles feedback of the review of swiftlang#40202

edymtt mentioned this pull request Dec 8, 2021

Add explanatory comments around freestanding test configuration #40472

Merged

edymtt deleted the extend-freestanding-to-darwin branch December 8, 2021 15:21


		swift_execution_tests_extra_flags += ' -experimental-hermetic-seal-at-link -lto=llvm-full'
		if not config.swift_freestanding_is_darwin:

Extend freestanding to support targeting Darwin platforms #40202

Extend freestanding to support targeting Darwin platforms #40202

Uh oh!

Conversation

edymtt commented Nov 16, 2021

Uh oh!

edymtt commented Nov 16, 2021

Uh oh!

edymtt commented Nov 16, 2021

Uh oh!

edymtt commented Nov 16, 2021

Uh oh!

compnerd Nov 16, 2021

Choose a reason for hiding this comment

Uh oh!

edymtt Nov 16, 2021

Choose a reason for hiding this comment

Uh oh!

gottesmm Nov 16, 2021

Choose a reason for hiding this comment

Uh oh!

gottesmm Nov 16, 2021

Choose a reason for hiding this comment

Uh oh!

edymtt Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swift-ci commented Nov 16, 2021

Uh oh!

Uh oh!

gottesmm Nov 16, 2021

Choose a reason for hiding this comment

Uh oh!

edymtt Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

edymtt commented Nov 17, 2021

Uh oh!

swift-ci commented Nov 17, 2021

Uh oh!

edymtt commented Nov 17, 2021

Uh oh!

edymtt commented Nov 17, 2021

Uh oh!

edymtt commented Dec 6, 2021

Uh oh!

edymtt commented Dec 6, 2021

Uh oh!

edymtt commented Dec 6, 2021

Uh oh!

edymtt commented Dec 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edymtt commented Dec 6, 2021

Uh oh!

edymtt commented Dec 6, 2021

Uh oh!

edymtt commented Dec 6, 2021

Uh oh!

edymtt commented Dec 6, 2021

Uh oh!

swift-ci commented Dec 6, 2021

Uh oh!

swift-ci commented Dec 7, 2021

Uh oh!

gottesmm Dec 7, 2021

Choose a reason for hiding this comment

Uh oh!

gottesmm Dec 7, 2021

Choose a reason for hiding this comment

Uh oh!

gottesmm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

edymtt Nov 17, 2021 •

edited

Loading

edymtt Nov 17, 2021 •

edited

Loading

edymtt commented Dec 6, 2021 •

edited

Loading