Remove Foundation dependency #39216

salinas-miguel · 2021-09-08T23:41:32Z

This removes the Swift project's dependency on Foundation on macOS.

resolves rdar://82458394

CodaFi · 2021-09-09T20:33:20Z

tools/swift-stdlib-tool/CMakeLists.txt

  SWIFT_COMPONENT compiler)

-find_library(FOUNDATION NAMES Foundation)
-target_link_libraries(swift-stdlib-tool PRIVATE ${FOUNDATION})
+set(CMAKE_CXX_STANDARD 17)


This means the entire project now requires a C++17-enabled compiler. Please mention this change on the Swift forums.

To be clear, I'm not opposed to such a change, but we have a wide variety of adopters with a wide variety of toolchains and we must be careful to make sure we're advertising the correct minimum requirements.

Do all of those toolchains need this tool, or is it limited to certain platforms like Darwin?

Right now, this tool is built to ship specifically for macOS hosts in Darwin toolchains.

That should dramatically reduce impact, because there is only one variety of toolchain we need to worry about compatibility with, then, right?

I can mention this in the forums. Xcode 13 beta 4 is currently required on macOS and includes a C++17 enabled version of Clang so I thought it would be ok to use C++17 here.

That should dramatically reduce impact, because there is only one variety of toolchain we need to worry about compatibility with, then, right?

Yes.

so I thought it would be ok to use C++17 here.

It is okay to use. I just want our adopters to be aware of this change, that's all.

Is there a reason to use CMAKE_CXX_STANDARD rather than setting CXX_STANDARD on just this target? I know you don't want to diverge too much across the project, but it seems like if you don't have a C++17-capable stdlib, you could just skip building this one target.

Ah, I'm not very familiar with CMake and didn't know this was possible. I'll use CXX_STANDARD on just this target instead. Thank you.

CodaFi · 2021-09-09T20:37:46Z

tools/swift-stdlib-tool/swift-stdlib-tool.cpp

-                NSUUID *uuid = (NSUUID *)
-                    CFUUIDCreateFromUUIDBytes(nil, uuid_bytes);
+                uuid_t uuid;
+                memcpy(uuid, uuid_cmd->uuid(), 16);


Please replace this magic number with a sizeof.

CodaFi · 2021-09-09T20:42:58Z

tools/swift-stdlib-tool/swift-stdlib-tool.cpp

    uint32_t len = 0;
-    _NSGetExecutablePath(nil, &len);
+    _NSGetExecutablePath(NULL, &len);
    std::vector<char> buffer;
    buffer.reserve(len);
    _NSGetExecutablePath(buffer.data(), &len);


Will _NSGetExecutablePath fill in the nul byte at the end? Even if so, I would feel more comfortable if we constructed a std::span with the len here and converted that to a std::filesystem::path.

It seems like std::span was introduced in C++20. _NSGetExecutablePath returns a string with a size of MAXPATHLEN at most so I was able to remove the first call. It does fill the nul byte at the end but I'm zeroing anyways.

CodaFi · 2021-09-09T20:49:05Z

tools/swift-stdlib-tool/swift-stdlib-tool.cpp

+
+    auto const stdOutData = readToEOF(outPipe[0]);
+
+    dispatch_semaphore_wait(gotStdErr, DISPATCH_TIME_FOREVER);


Waiting forever doesn't seem like the right default. But it's also not your bug so we can deal with this later.

CodaFi · 2021-09-09T20:51:01Z

tools/swift-stdlib-tool/swift-stdlib-tool.cpp

-    for (NSString *lib in libs) @autoreleasepool {
-        NSString *src = [src_dir stringByAppendingPathComponent:lib]; 
-        NSString *dst = [dst_dir stringByAppendingPathComponent:lib];
+    for (auto const &pair : libs) {


As long as we're using C++17:

Suggested change

for (auto const &pair : libs) {

for (auto const &[lib, srcUUIDs] : libs) {

Thanks, I didn't know this was possible in C++

CodaFi · 2021-09-09T20:52:52Z

tools/swift-stdlib-tool/swift-stdlib-tool.cpp

+        fail_usage("At least one of --source-libraries and --platform "
+                   "must be set.");
+    }
+    else if (src_dir.empty()) {


Nit: else if consistent with the others please.

CodaFi · 2021-09-09T21:01:45Z

tools/swift-stdlib-tool/swift-stdlib-tool.cpp

+    const char *launchPath = "/usr/bin/xcrun";
+
+    // Tell xcrun to search our toolchain first.
+    const char *arguments[commandAndArguments.size() + 5];


Using std::vector here would avoid a lot of the magic math.

CodaFi · 2021-09-09T21:06:42Z

tools/swift-stdlib-tool/swift-stdlib-tool.cpp

 }();

+std::vector<uint8_t> readToEOF(int fd) {


I think we can replace this function with dispatch_read in a loop.

I'm also using this function to read synchronously on line 741 so I think this is probably still needed.

CodaFi · 2021-09-09T21:09:46Z

tools/swift-stdlib-tool/swift-stdlib-tool.cpp

-                   NSMutableDictionary *libs, bool stripBitcode)
+void copyLibraries(std::filesystem::path src_dir, std::filesystem::path dst_dir, 
+                   std::unordered_map<std::string,
+                   std::unordered_set<std::string>> libs,


This is equivalent to std::unordered_multimap

Does this routine mutate the map? It doesn't look like it, so take it by const ref please.

I tried using a std::unordered_multimap but iterating through it in the same way and comparing to dstUUIDs below was a little messier.

salinas-miguel · 2021-09-20T19:56:35Z

@CodaFi I addressed your feedback and also made the C++17 requirement specific to this target as @belkadan suggested. Let me know what you think.

CodaFi

LGTM

Thank you for your patience here.

@swift-ci test

drodriguez · 2021-09-23T18:21:17Z

tools/swift-stdlib-tool/CMakeLists.txt

+  CXX_STANDARD 17
+  CXX_STANDARD_REQUIRED ON)
+
+target_link_libraries(swift-stdlib-tool PRIVATE)


This line doesn't seem to do anything anymore. If there's no need for linking libraries, let's remove it completely.

drodriguez · 2021-09-23T18:22:41Z

tools/swift-stdlib-tool/swift-stdlib-tool.cpp

@@ -418,8 +424,8 @@ ssize_t pread_all(int fd, void *buf, size_t count, off_t offset)

 template <typename T> 
 int parse_macho(int fd, uint32_t offset, uint32_t size, 
-                void (^dylibVisitor)(NSString *path), 
-                void (^uuidVisitor)(NSUUID *uuid))
+void (^dylibVisitor)(std::filesystem::path const &path),


Nit: seems that this line is not correctly indented with the surroundings.

drodriguez · 2021-09-23T18:31:41Z

tools/swift-stdlib-tool/swift-stdlib-tool.cpp

+                  std::back_inserter(retData));
+    }
+    return retData;
+}

 // Runs a tool with `xcrun`. 
 // Returns NSTask.terminationStatus.


Outdated comment

drodriguez · 2021-09-23T18:49:51Z

tools/swift-stdlib-tool/swift-stdlib-tool.cpp

-                 src.fileSystemRepresentation, 
-                 dst.fileSystemRepresentation, 
-                 nserr.localizedFailureReason.UTF8String);
+        if (rename(src.c_str(), dst.c_str()) != 0) {


rename is different than a copy. Is this an intentional change?

Thanks for finding this mistake

drodriguez · 2021-09-23T19:10:10Z

tools/swift-stdlib-tool/swift-stdlib-tool.cpp

+        auto const &lib = worklist.back();
+        worklist.pop_back();


You cannot grab a reference to the back() and immediately pop_back(). It will invalidate the reference. It probably works today because of luck, but it is not guaranteed to work. It is probably safer to remove the reference and pay the price of the copy.

drodriguez · 2021-09-23T19:20:39Z

tools/swift-stdlib-tool/swift-stdlib-tool.cpp

+            std::move(otherCodesignFlags.begin(), otherCodesignFlags.end(),
+                      std::back_inserter(commandAndArguments));


I am not sure that moving in every loop from otherCodesignFlags is the correct thing. Why not std::copy which should be clearly safe?

drodriguez · 2021-09-27T17:19:37Z

@swift-ci please test

swift-ci · 2021-09-27T17:45:17Z

Build failed
Swift Test OS X Platform
Git Sha - de73874

salinas-miguel · 2021-09-29T21:40:42Z

It looks like std::filesystem is only available on macOS 10.15 and later. Apologies for not realizing this sooner. I'll remove its usage so that we can keep the current deployment target (macOS 10.9).

drodriguez · 2021-10-05T23:15:05Z

@swift-ci please test

(I haven't looked at the code yet. Thanks for your patience)

salinas-miguel · 2021-10-07T22:59:27Z

@drodriguez it looks like the CI was not triggered for some reason

drodriguez · 2021-10-08T00:02:49Z

@swift-ci please test

salinas-miguel · 2021-10-09T01:51:23Z

I think the Windows failure may be unrelated since this target is not built for Windows.

salinas-miguel · 2021-10-15T16:39:32Z

@CodaFi does this look good to merge? I removed the C++17 requirement since I was only using it for the std::filesystem API which is not available before macOS 10.15.

CodaFi · 2021-11-02T18:32:37Z

tools/swift-stdlib-tool/swift-stdlib-tool.cpp

+        close(outPipe[0]);
+        close(errPipe[0]);
+
+        execv(launchPath, (char *const *)arguments.data());


const_cast<char **> here please.

CodaFi · 2021-11-02T18:33:27Z

tools/swift-stdlib-tool/swift-stdlib-tool.cpp

@@ -418,8 +426,8 @@ ssize_t pread_all(int fd, void *buf, size_t count, off_t offset)

 template <typename T> 
 int parse_macho(int fd, uint32_t offset, uint32_t size, 
-                void (^dylibVisitor)(NSString *path), 
-                void (^uuidVisitor)(NSUUID *uuid))
+                void (^dylibVisitor)(std::string const &path),


Nit: LLVM style is west-const.

CodaFi · 2021-11-02T18:33:36Z

tools/swift-stdlib-tool/swift-stdlib-tool.cpp

@@ -487,8 +487,8 @@ int parse_macho(int fd, uint32_t offset, uint32_t size,


 int parse_macho(int fd, uint32_t offset, uint32_t size, 
-              void (^dylibVisitor)(NSString *path), 
-              void (^uuidVisitor)(NSUUID *uuid))
+              void (^dylibVisitor)(std::string const &path),


CodaFi · 2021-11-02T18:33:45Z

tools/swift-stdlib-tool/swift-stdlib-tool.cpp

@@ -515,8 +515,8 @@ int parse_macho(int fd, uint32_t offset, uint32_t size,


 int parse_fat(int fd, off_t fsize, char *buffer, size_t size, 
-              void (^dylibVisitor)(NSString *path), 
-              void (^uuidVisitor)(NSUUID *uuid))
+              void (^dylibVisitor)(std::string const &path),


CodaFi

In general, the spacing and brace style in this file do not match the conventions used in this codebase. Please run clang-format against this file.

salinas-miguel · 2021-11-03T23:33:30Z

Pushed a change to reflect the changes in #39996. @CodaFi

CodaFi · 2021-11-04T23:31:42Z

Looks pretty faithful to #39996 to me.

@swift-ci test

CodaFi · 2021-11-04T23:34:00Z

I'm going to squash-merge this when it finally goes in because the history is a bit messy here and I don't want you to have to rebase which will kill the test run.

swift-ci · 2021-11-05T03:05:15Z

Build failed
Swift Test Linux Platform
Git Sha - 1e94613

salinas-miguel · 2021-11-05T16:33:28Z

The linux failure seems unrelated:
/home/buildnode/jenkins/workspace/swift-PR-Linux/branch-main/swift-docc/Tests/SwiftDocCUtilitiesTests/PreviewServer/PreviewServerTests.swift:43: error: PreviewServerTests.testPreviewServerBeforeStarted : failed - The operation could not be completed. (NIO.SocketAddressError error 3.)

CodaFi · 2021-11-05T17:22:30Z

@swift-ci test Linux platform

When used without passing `--source-libraries`, this tool fetches its current path and uses that to find the directories of the swift libraries near it. Since #39216 it was searching the nearby directories recursively, and only for files, instead of just looking at 1 level, only for directories. Because of this it would never discover `path/to/lib/swift-5.0/...` because it was only looking for files. This fixes this issue with a new function, and renames the previous function to be more explicit for the other use case that does want that behavior. (cherry picked from commit d1eabf1)

Remove Foundation dependency

2fdd5e6

CodaFi reviewed Sep 9, 2021

View reviewed changes

address feedback

f995005

mackoj mentioned this pull request Sep 12, 2021

[194] Issue #194 - September 23, 2021 SwiftWeekly/swiftweekly.github.io#679

Closed

salinas-miguel and others added 2 commits September 13, 2021 09:43

only require C++17 for swift-stdlib-tool

6e8c59a

fix typo

815b784

CodaFi approved these changes Sep 20, 2021

View reviewed changes

drodriguez reviewed Sep 23, 2021

View reviewed changes

salinas-miguel added 2 commits September 24, 2021 15:40

fix bugs and address feedback

b075697

Merge branch 'main' of github.com:salinas-miguel/swift

de73874

salinas-miguel added 3 commits October 1, 2021 16:16

remove std::filesystem usage

808d7f3

fix directory traversal

786e906

fix fd leaks

2e8028c

salinas-miguel requested review from drodriguez and CodaFi October 26, 2021 17:36

CodaFi reviewed Nov 2, 2021

View reviewed changes

salinas-miguel added 2 commits November 2, 2021 12:10

fix formatting issues

3c8bb5d

Merge branch 'main'

1e94613

CodaFi merged commit eab562f into swiftlang:main Nov 6, 2021

keith mentioned this pull request Nov 8, 2021

[swift-stdlib-tool] use correct paths when codesigning #40093

Merged

keith mentioned this pull request Jan 31, 2022

[swift-stdlib-tool] Fix finding sibling directories #41110

Merged

keith mentioned this pull request Feb 8, 2022

[5.6][swift-stdlib-tool] Fix finding sibling directories #41262

Merged


		auto const stdOutData = readToEOF(outPipe[0]);

		dispatch_semaphore_wait(gotStdErr, DISPATCH_TIME_FOREVER);

	for (auto const &pair : libs) {
	for (auto const &[lib, srcUUIDs] : libs) {

		std::move(otherCodesignFlags.begin(), otherCodesignFlags.end(),
		std::back_inserter(commandAndArguments));

Remove Foundation dependency #39216

Remove Foundation dependency #39216

Uh oh!

Conversation

salinas-miguel commented Sep 8, 2021

Uh oh!

CodaFi Sep 9, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

salinas-miguel commented Sep 20, 2021

Uh oh!

CodaFi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drodriguez commented Sep 27, 2021

Uh oh!

swift-ci commented Sep 27, 2021

Uh oh!

salinas-miguel commented Sep 29, 2021

Uh oh!

drodriguez commented Oct 5, 2021

CodaFi Sep 9, 2021 •

edited

Loading