Swift: introduce Xcode autobuilder #10786

AlexDenisov · 2022-10-12T06:56:01Z

No description provided.

redsun82

some minor comments on the code itself, but one generic concern.

I'm not sold yet on the choice to use objective C++. While most of it looks like C++, the objective C-specific parts look alien enough to a C++ developer (like myself and probably a good part of current and future team members) that this might mean quite a maintenance cost down the line.

So on a general note, I think it might make sense to either:

try to keep it in C++ finding some off-the-shelf plist deserializer. But I'm really meh on this, as the top result on google is this project that has not seen updates since 9 years or so 🙁
A bit paradoxically, I think Swift might actually be a better choice, even if it's a language further away from C++, as Swift knowledge will always be part of what is necessary to maintain Swift language support. It could even be a good exercise for us 🙂

redsun82 · 2022-10-12T07:03:01Z

swift/xcode-autobuilder/BUILD.bazel

+swift_cc_binary(
+    name = "xcode-autobuilder",
+    srcs = glob([
+        "*.c",
+    ]),
+    visibility = ["//swift:__pkg__"],
+    deps = [
+        ":xcode-autobuilder-lib",
+    ],
+)


are we not using objc_binary here just to capture the settings made by swift_cc_binary?

I couldn't find objc_binary so went with the simplest available option https://bazel.build/reference/be/objective-c

redsun82 · 2022-10-12T07:11:32Z

swift/xcode-autobuilder/xcode-autobuilder.mm

+  std::unordered_set<std::string> allowedTargetTypes({Application, Framework});
+  targets.erase(
+      std::remove_if(std::begin(targets), std::end(targets),
+                     [&](Target& t) -> bool { return !allowedTargetTypes.count(t.type); }),
+      std::end(targets));


I'd probably do this filtering earlier, as early as already inside mapTargetsToWorkspace to avoid all the rest of the work on target information collection.

We still need to get all the targets so that we can calculate the files in the dependencies.

redsun82 · 2022-10-12T07:21:50Z

swift/xcode-autobuilder/xcode-autobuilder.mm

+  std::unordered_set<std::string> allowedTargetTypes({Application, Framework});
+  targets.erase(
+      std::remove_if(std::begin(targets), std::end(targets),
+                     [&](Target& t) -> bool { return !allowedTargetTypes.count(t.type); }),


I think using a std::unordered_set to filter out two static values is a bit overkill 😅 return t.type != Application && t.type != Framework is ok to read and faster to execute.

AlexDenisov · 2022-10-12T07:50:16Z

Re: ObjC: I'll give Swift a try, but I think it will be even more messy/hard to read than the ObjC version.

redsun82

That's my first pass, I'd like to get into it again tomorrow 🙂

redsun82 · 2022-10-17T12:24:34Z

swift/BUILD.bazel

+pkg_files(
+    name = "autobuild",
+    srcs = ["tools/autobuild.sh"],
+    attributes = pkg_attributes(mode = "0755"),
+    prefix = "tools",
+)
+


nit: we can probably merge this with the qltest definition above into something like

pkg_files( name = "scripts", srcs = ["tools/qltest.sh", "tools/autobuild.sh"], attributes = pkg_attributes(mode = "0755"), prefix = "tools", )

swift/tools/autobuild.sh

redsun82 · 2022-10-17T12:37:30Z

swift/xcode-autobuilder/XcodeBuildRunner.cpp

+extern char** environ;
+
+static bool exec(const std::vector<std::string>& argv) {
+  const char** c_argv = (const char**)calloc(argv.size() + 1, sizeof(char*));


you can avoid using calloc and free and still pass the arguments to the underlying C exec API with

std::vector<const char*> c_argv; c_argv.reserve(argv.size() + 1); for (const auto& arg : argv) { c_argv.push_back(argv.c_str()); } c_argv.push_back(nullptr);

or alternatively

std::vector<const char*> c_argv{argv.size() + 1, nullptr}; for (size_t i = 0; i < argv.size(); i++) { c_argv[i] = argv[i].c_str(); }

and then pass c_argv.data() to posix_spawn.

Just tried it and it throws way more "incompatible const * char * const ***char types" errors than I'm ready to deal with 😄

redsun82 · 2022-10-17T12:40:22Z

swift/xcode-autobuilder/XcodeBuildRunner.cpp

+    std::string s;
+    for (auto& arg : argv) {
+      s += arg + " ";
+    }
+    std::cout << s << "\n";


nit: it's better to avoid building a string if we can directly stream.

Suggested change

std::string s;

for (auto& arg : argv) {

s += arg + " ";

}

std::cout << s << "\n";

for (auto& arg : argv) {

std::cout << arg << " ";

}

std::cout << "\n";

I'm sure I had a reason for using a string, but I cannot recall it anymore 😆
Anyways, applied your suggestion!

swift/xcode-autobuilder/XcodeProjectParser.cpp

redsun82 · 2022-10-17T12:47:38Z

swift/xcode-autobuilder/XcodeProjectParser.cpp

+};
+
+static std::string stringValue(CFDictionaryRef dict, CFStringRef key) {
+  auto cfValue = (CFStringRef)CFDictionaryGetValue(dict, key);


although it requires to type more characters, it's preferable to use static_cast over a C-style cast 🙂

https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es49-if-you-must-use-a-cast-use-a-named-cast

there are many, many other cases.

In any case as discussed on zoom, probably all these C-style casts should be extracted to functions performing a type sanity check (and aborting with an error if the type does not match)

redsun82 · 2022-10-17T12:52:39Z

swift/xcode-autobuilder/XcodeProjectParser.cpp

+static std::string stringValue(CFDictionaryRef dict, CFStringRef key) {
+  auto cfValue = (CFStringRef)CFDictionaryGetValue(dict, key);
+  if (cfValue) {
+    const int bufferSize = 256;


nit, and probably makes no real difference to the compiler, but you can replace const with constexpr

redsun82 · 2022-10-17T14:51:06Z

swift/xcode-autobuilder/XcodeBuildRunner.cpp

+  if (!WIFEXITED(status) || WEXITSTATUS(status) != 0) {
+    return false;
+  }
+  return true;


nit: maybe just

Suggested change

if (!WIFEXITED(status) || WEXITSTATUS(status) != 0) {

return false;

}

return true;

return WIFEXITED(status) && WEXITSTATUS(status) == 0;

redsun82 · 2022-10-17T15:01:17Z

swift/xcode-autobuilder/XcodeProjectParser.cpp

+  Targets targets;
+  Dependencies dependencies;
+  BuildFiles buildFiles;
+


there are three big loops in this function, would it maybe help readability to extract each of those into a separate function with a descriptive name like collectTargets, collectDependencies, collectBuildFiles?

Let me give it a try!

It's a bit interconnected, so splitting it into separate functions only makes navigation harder, so I'm not sure if it's actually better...

fair enough!

redsun82 · 2022-10-17T15:04:37Z

swift/xcode-autobuilder/XcodeProjectParser.cpp

+  }
+  std::ifstream ifs(pbxproj, std::ios::in);
+  std::string content((std::istreambuf_iterator<char>(ifs)), (std::istreambuf_iterator<char>()));
+  auto data = CFDataCreate(allocator, (UInt8*)content.data(), content.size());


use static_cast instead of C-style cast

It's char * to unsigned char * so I guess we can only use reinterpret_cast here?

right you are 🙂

redsun82 · 2022-10-18T07:02:07Z

swift/xcode-autobuilder/BUILD.bazel

+        "-lxml2",
+        "-framework CoreFoundation",
+    ],
+)


Suggested change

)

target_compatible_with = ["@platforms//os:macos"],

)

redsun82 · 2022-10-18T07:59:32Z

I'm wondering whether a very crude and quick set of C++ wrappers wouldn't help make the code more readable (and do the type sanity checks. I've jotted down this incomplete and totally untested code:

class Value {
  void* wrapped;

 public:
  Value(void* wrapped) : wrapped{wrapped} {}

  explicit operator bool() const { return wrapped != nullptr; }

  template <typename T>
  T as() const {
    return {wrapped};
  }
};

template <>
CFStringRef Value::as<CFStringRef>() const {
  CFStringRef ret = wrapped;
  // check actually string else abort
  return ret;
}

template <>
std::string Value::as<std::string>() const {
  auto cfValue = asCFStringRef();
  const int bufferSize = 256;
  char buf[bufferSize];
  if (CFStringGetCString(cfValue, buf, bufferSize, kCFStringEncodingUTF8)) {
    return {buf};
  }
  return {};
}

class Dictionary {
  CFDictionaryRef wrapped;
 public:
  Dictionary(CFDictionaryRef wrapped) : wrapped{wrapped} {
    // check actually a dictionary or abort
  }

  std::size_t size() const {
    return CFDictionaryGetCount(wrapped);
  }

  Value operator[](CFStringRef key) const {
    return {CFDictionaryGetValue(wrapped, key)}
  }

  std::vector<std::pair<Value, Value>> items() const {
    auto size = CFDictionaryGetCount(wrapped);
    std::vector<const void *> keys{size};
    std::vector<const void *> values{size};
    CFDictionaryGetKeysAndValues(dict, keys.data(), values.data());
    std::vector<std::pair<Value, Value>> ret;
    ret.reserve(size);
    for (auto i = 0u; i < size; ++i) {
      ret.emplace_back({keys[i]}, {values[i]});
    }
    return ret;
  }
};

class Array {
  CFArrayRef wrapped;
 public:
  Array(CFArrayRef wrapped) : wrapped{wrapped} {
    // check actually an array or abort
  }

  std::size_t size() const {
    return CFArrayGetCount(wrapped);
  }

  Value operator[](std::size_t i) const {
    return {CFArrayGetValueAtIndex(wrapped, i)};
  }
};

The idea would then to do for example

if (auto isa = object[CFSTR("isa")].as<CFStringRef>()) {
  ...
}

or

auto deps = targetObject[CFSTR("dependencies")].as<Array>();
for (auto i = 0u; i < deps.size(); ++i) {
  auto dependencyId = deps[i].as<CFStringRef>();
  auto dependency = objects[dependencyId].as<Dictionary>();
  auto targetId = dependency[CFSTR("target")].as<CFStringRef>();
  ...
}

Do you think it's worth it?

AlexDenisov · 2022-10-18T08:13:58Z

Do you think it's worth it?

I picked a bit simpler and less template-heavy approach, will push in a moment.

redsun82

As you already mentioned, the code is not super-easy to follow, but this is in part because of the intrinsic complexity and the APIs. While I'm sure we could come up with some beautiful C++ API wrapping the core foundation one, I also find it's not worth pursuing that for just this piece of code.

Let's , and if the code will hurt in the future we can iterate on it then 🙂

AlexDenisov · 2022-10-19T08:19:44Z

I just run it in a dry-run mode against a bigger sample:

Total: 2595
Crashed: 2
Errored: 341
Succeeded: 2252

Errored here means either there was no Xcode project/workspace found or there were no suitable targets (e.g., ObjC only project, etc.).
I will take a look at the crashes, but so far the result is pretty solid, so I'm confident we can merge it.

AlexDenisov requested a review from redsun82 October 12, 2022 06:56

github-actions bot added the Swift label Oct 12, 2022

redsun82 reviewed Oct 12, 2022

View reviewed changes

AlexDenisov force-pushed the alexdenisov/xcode-autobuilder branch from 00b643a to a7397e4 Compare October 14, 2022 10:45

Swift: introduce Xcode autobuilder

3040837

AlexDenisov force-pushed the alexdenisov/xcode-autobuilder branch from a7397e4 to 3040837 Compare October 14, 2022 10:45

AlexDenisov added 3 commits October 17, 2022 10:14

Swift: only run Xcode autobuilder on macOS

f3ed54e

Swift: split Xcode autobuild

0521855

Swift: do not rely on CFStringGetLength

dde51d3

redsun82 reviewed Oct 17, 2022

View reviewed changes

redsun82 reviewed Oct 18, 2022

View reviewed changes

AlexDenisov marked this pull request as ready for review October 18, 2022 07:04

AlexDenisov requested a review from a team as a code owner October 18, 2022 07:04

AlexDenisov added 4 commits October 18, 2022 09:12

Merge branch 'main' into alexdenisov/xcode-autobuilder

023fea6

Swift: do not use relative include paths

6d754c4

Swift: add CMake target for xcode-autobuilder

d97669f

Swift: make dealing with CF types typesafe

44c26be

AlexDenisov added 2 commits October 18, 2022 10:01

Swift: do not use C casts

a3b5f22

Swift: simplify Bazel a bit

eba7f1a

AlexDenisov added 2 commits October 18, 2022 10:15

Swift: stream directly instead of using intermediate string

ad9f5ef

Swift: %/\t/\s/

0c3fd9f

redsun82 approved these changes Oct 18, 2022

View reviewed changes

AlexDenisov merged commit 5c8fa84 into main Oct 19, 2022

AlexDenisov deleted the alexdenisov/xcode-autobuilder branch October 19, 2022 08:19

Swift: introduce Xcode autobuilder #10786

Swift: introduce Xcode autobuilder #10786

Uh oh!

Conversation

AlexDenisov commented Oct 12, 2022

Uh oh!

redsun82 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!

AlexDenisov commented Oct 12, 2022

Uh oh!

redsun82 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

AlexDenisov Oct 18, 2022 • 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!

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!

redsun82 commented Oct 18, 2022

Uh oh!

AlexDenisov commented Oct 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

redsun82 left a comment

Choose a reason for hiding this comment

Uh oh!

AlexDenisov commented Oct 19, 2022

Uh oh!

Uh oh!

AlexDenisov Oct 18, 2022 •

edited

Loading

AlexDenisov commented Oct 18, 2022 •

edited

Loading