Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Merge iproj and xproj? #5942

Closed
wants to merge 2 commits into from
Closed

Merge iproj and xproj? #5942

wants to merge 2 commits into from

Conversation

kkaefer
Copy link
Contributor

@kkaefer kkaefer commented Aug 17, 2016

#5727 is showing that it’s difficult to keep track of shared sources between the iOS and macOS SDKs. Should we combine ios.xcworkspace and macos.xcworkspace (which also hosts the Node schemes) into one workspace that contains both ios.xcodeproj and macos.xcodeproj? Should we go a step further and combine the .xcodeproj files, so that a file can directly be targeted at either platform or both platforms as needed? We do this in the Mapbox Swift library projects without any notable problems. It would also make it much easier to stand up a tvOS SDK in the future (#2340 #2778).

/cc @kkaefer @frederoni @friedbunny @boundsj @incanus @mikemorris

@1ec5 1ec5 added iOS Mapbox Maps SDK for iOS build macOS Mapbox Maps SDK for macOS Node.js node-mapbox-gl-native labels Aug 11, 2016
@friedbunny
Copy link
Contributor

friedbunny commented Aug 11, 2016

I think I would be for this from a development perspective, but I don’t have much of an idea of how it would complicate the build system.

make xproj also seems to have special powers that make iproj does not — when tests don’t run or a dependency isn’t installed properly, make xproj tends to mend things.

@1ec5
Copy link
Contributor Author

1ec5 commented Aug 11, 2016

I don’t expect this to complicate much on the Xcode-managed side, where there’s currently a lot of duplication. In CMake, there may be some awkwardness from having the mbgl project target both platforms with the same sources. Mason is a total unknown for me – it’s very dependent on the current platform.

when tests don’t run or a dependency isn’t installed properly, make xproj tends to mend things.

That may be because macos.xcworkspace also contains the Node sources and schemes, so CMake has to set up a bit more in order to prepare the workspace for development.

@kkaefer
Copy link
Contributor

kkaefer commented Aug 12, 2016

In CMake, there may be some awkwardness from having the mbgl project target both platforms with the same sources.

What in a project/settings makes Xcode compile something for iOS vs. for macOS?

Mason is a total unknown for me – it’s very dependent on the current platform.

Mason is platform-dependent, but since we control all of it, we can change it so that we can install for both platforms. iOS only has one compiled dependency (geojson); all of the rest is header only (and thus shared with macOS).

@1ec5
Copy link
Contributor Author

1ec5 commented Aug 12, 2016

It may be instructive to look at MapboxGeocoder.swift, which has one project that supports all four Apple platforms (iOS, macOS, tvOS, watchOS). MapboxGeocoder.xcodeproj contains a framework and test target for each platform (other than watchOS, which can’t be unit tested at the moment). A file is a member of one or more of these targets. If it’s in multiple targets, it’ll be listed multiple times in the project file, under each target’s list of buildables. For example, this sources list enumerates all the source files needed for the target.

It’s also possible to conditionally compile code using compiler macros, as we already do in a few places.

@kkaefer
Copy link
Contributor

kkaefer commented Aug 15, 2016

Looked into this a little further:

  • CMake doesn't have explicit support for building multiple platforms in one go. That means we'd have to manually merge both builds, e.g. by includeing the *.cmake files twice, but with different variables set so they generate separate targets for macOS and iOS. This is doable, but complicates the process a bit.
  • We'd need to do the same thing for Mason: Invoke everything twice, once for iOS and once for macOS.
  • We'd need to manually set the build output path, since those paths are currently not versioned per platform (build/macos/Debug is where all files, iOS and macOS would end up in).

Overall, I think this is not worth implementing unless there are real benefits, but it doesn't seem like it? We'd still have to have separate targets for iOS vs. macOS; they're just bundled in one Xcode project? Instead of merging this on a CMake level, we could retain the two generated CMake projects, and instead abandon the ios.xcworkspace and merge all of these projects into the macos.xcworkspace so they all appear in one project. However, apart from not having to switch windows, it's unclear what the benefit of that would be.

@1ec5
Copy link
Contributor Author

1ec5 commented Aug 15, 2016

Yes, we'd still have multiple targets, one per platform. The advantage to merging the workspaces is better maintainability at the SDK level: because we manually add each file to the Xcode project, we have to remember to do so in the other workspace as well.

What if we combine the iOS and macOS .xcodeproj's, but have the iOS target depend on the existing CMake-generated iOS libraries and the macOS target depend on the CMake-generated macOS libraries? That way we don't have to do any merging in CMake, and we get all the benefits of merging projects in the SDK, where it matters. The only change we'd need to make at the CMake level is to rename each mbgl.xcodeproj to be unique. (Optional, but probably a good idea to keep confusion to a minimum.)

@kkaefer
Copy link
Contributor

kkaefer commented Aug 15, 2016

What if we combine the iOS and macOS .xcodeproj's, but have the iOS target depend on the existing CMake-generated iOS libraries and the macOS target depend on the CMake-generated macOS libraries? That way we don't have to do any merging in CMake, and we get all the benefits of merging projects in the SDK, where it matters.

That's what I was proposing in #5942 (comment). I tried it, and it works (even with non-unique names for mbgl, but the resulting Dropdown is pretty unwieldily.

If you have to manually add files, you have to add them to both the iOS and the macOS version of the target, correct? Is there anything shared among these?

@1ec5
Copy link
Contributor Author

1ec5 commented Aug 15, 2016

the resulting Dropdown is pretty unwieldily.

We should come up with more descriptive names for each scheme. Scheme names can contain spaces, parentheses, and (yep) even emoji.

Also, Xcode lets you hide schemes you aren't interested in (in Manage Schemes). Your preference should persist across invocations of make xproj; if not, that should be pretty simple to fix.

If you have to manually add files, you have to add them to both the iOS and the macOS version of the target, correct? Is there anything shared among these?

Not currently. If we merge the two projects, it'd be much easier to factor out a static library for the "Darwin" SDK code shared between these two platforms.

But even adding to both targets manually would be fine: right now, when you add a file to the iOS project, you're presented with a list of candidate targets that includes the dynamic and static targets for iOS (you have to check both). Merging the projects together would add a third candidate target for macOS to the list. The key is having them all in the same place when making that decision.

@kkaefer
Copy link
Contributor

kkaefer commented Aug 15, 2016

Seems like we can check in the disabling of "Autocreate Schemes":

diff --git a/platform/macos/macos.xcworkspace/xcshareddata/WorkspaceSettings.xcsettings b/platform/macos/macos.xcworkspace/xcshareddata/WorkspaceSettings.xcsettings
new file mode 100644
index 0000000..08de0be
--- /dev/null
+++ b/platform/macos/macos.xcworkspace/xcshareddata/WorkspaceSettings.xcsettings
@@ -0,0 +1,8 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
+<plist version="1.0">
+<dict>
+       <key>IDEWorkspaceSharedSettings_AutocreateContextsIfNeeded</key>
+       <false/>
+</dict>
+</plist>

Then we'd only create the schemes we actually want to show and can use correct names for them (the create_scheme.sh script already supports arbitrary names).

Not currently. If we merge the two projects, it'd be much easier to factor out a static library for the "Darwin" SDK code shared between these two platforms.

Yeah, but how would be build that target for all the platforms?

@1ec5
Copy link
Contributor Author

1ec5 commented Aug 15, 2016

Autocreated schemes are named after the target by default. We should give the targets more descriptive names, too, while we’re at it.

I don’t recall why we opted to rely on autocreated schemes, but turning it off might allow us to omit the unused ones that come with the KIF project.

Not currently. If we merge the two projects, it'd be much easier to factor out a static library for the "Darwin" SDK code shared between these two platforms.

Yeah, but how would be build that target for all the platforms?

Ah, right, that library would still require two targets. 😄

@mention-bot
Copy link

@kkaefer, thanks for your PR! By analyzing this pull request, we identified @1ec5, @boundsj and @jfirebaugh to be potential reviewers.

@kkaefer
Copy link
Contributor

kkaefer commented Aug 17, 2016

@1ec5 I added an attempt at merging the projects, but I'm still not convinced it is a good idea since the workspace becomes pretty big; it takes a long time to have Xcode index everything and I'm unsure about the benefits (remember to add files in both projects IMHO is a weak reason).

@kkaefer
Copy link
Contributor

kkaefer commented Aug 17, 2016

Also, once we merged iOS and macOS, should we also merge in the remaining separate Qt project?

@1ec5
Copy link
Contributor Author

1ec5 commented Aug 17, 2016

We add files very frequently to the iOS and macOS projects. Because we check in the projects instead of generating them with CMake, the usual workflow is to use Xcode's Add Files feature.

Remembering to add files is indeed a weak argument, but we've had several incidents lately where a file was missing from either project. CI can't always catch these issues because headers can still be found via the header search path.

If the indexing time would become a burden for folks not working on the iOS or macOS SDK, I suppose we can table this work and see if the missing file reference issue subsides after the runtime styling work draws down.

should we also merge in the remaining separate Qt project?

If there's a desire for that, then sure, I guess. But keeping Qt separate doesn't have the same downsides as keeping the iOS and macOS projects separate, does it?

@1ec5
Copy link
Contributor Author

1ec5 commented Mar 13, 2017

it takes a long time to have Xcode index everything

If I’m not mistaken, this is mostly because the same workspace contains two copies of mbgl.xcodeproj and thus two sets of file references. Instead, the workspace should contain only one copy of mbgl.xcodeproj, and that project should have two copies of the mbgl-core target, one with iOS as the base SDK and one with macOS as the base SDK.

Similarly, we’d want to combine macos.xcodeproj and ios.xcodeproj into a single darwin.xcodeproj to avoid each Darwin file having two references.

With only one set of references, a particular scheme will have the same number of file references that it does on master, so indexing will take roughly the same amount of time.

@1ec5 1ec5 force-pushed the 5942-merge-iproj-and-xproj branch 3 times, most recently from a25c7a6 to 64ad3a8 Compare March 14, 2017 05:10
@1ec5
Copy link
Contributor Author

1ec5 commented Mar 14, 2017

Instead, the workspace should contain only one copy of mbgl.xcodeproj, and that project should have two copies of the mbgl-core target, one with iOS as the base SDK and one with macOS as the base SDK.

This would only reduce the number of file references if we’re able to deduplicate file references; that is, if we’re able to have a single file be a member of multiple targets. Apparently CMake doesn’t support that. I wonder if this is something we could fix up after the fact using the xcodeproj gem or xcode npm module.

/cc @incanus

@1ec5 1ec5 force-pushed the 5942-merge-iproj-and-xproj branch from 1120ed0 to eb1ebe0 Compare March 18, 2017 07:10
@1ec5
Copy link
Contributor Author

1ec5 commented Jul 11, 2018

This would only reduce the number of file references if we’re able to deduplicate file references; that is, if we’re able to have a single file be a member of multiple targets. Apparently CMake doesn’t support that. I wonder if this is something we could fix up after the fact using the xcodeproj gem or xcode npm module.

This gist claims to duplicate a target using the xcodeproj gem. I’m not sure if it works as well as the Duplicate command in Xcode, but if so, that would be very useful for merging the iOS and macOS projects without burdening everyone with additional indexing. It would be even more useful for #9319.

@stale
Copy link

stale bot commented Oct 24, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the archived Archived because of inactivity label Oct 24, 2018
@stale
Copy link

stale bot commented Oct 26, 2018

This pull request has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions.

@stale stale bot closed this Oct 26, 2018
@1ec5
Copy link
Contributor Author

1ec5 commented Oct 28, 2018

This was originally an issue that got commandeered for a pull request. The pull request has stalled, but I’m hopeful the work in #9319 will get us closer to a merged iOS/macOS project.

@1ec5 1ec5 reopened this Oct 28, 2018
@stale stale bot removed the archived Archived because of inactivity label Oct 28, 2018
@stale stale bot added the archived Archived because of inactivity label Dec 27, 2018
@stale
Copy link

stale bot commented Dec 27, 2018

This pull request has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions.

@stale stale bot closed this Dec 27, 2018
@tmpsantos tmpsantos deleted the 5942-merge-iproj-and-xproj branch April 24, 2020 20:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived Archived because of inactivity build iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS Node.js node-mapbox-gl-native
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants