Consolidate Xcode Project Setup

Consolidate Xcode Project Setup

1. Remove 'sources.xcodeproj'
2. Add a 'sources' target under products project for files indexing.
3. Delete xctests/ folder to fix duplicate xctest file copies.

BUG=679110
Review-Url: https://codereview.chromium.org/2623203004
Cr-Commit-Position: refs/heads/master@{#444469}
Committed: https://chromium.googlesource.com/chromium/src/+/de19976ca12bb4c42acd5bfb5a83a69047d6e7b4

[liaoyuke, 2017-01-12 23:32:55]

liaoyuke@chromium.org changed reviewers:
+ justincohen@chromium.org, sdefresne@chromium.org

[liaoyuke, 2017-01-12 23:32:55]

Hi Sylvain, Justin,

Please take a look!

Thank you very much!

[liaoyuke, 2017-01-12 23:33:23]

Description was changed from

==========
Consolidate Xcode project setup

1. Remove 'sources.xcodeproj' and 'all.xcworkspace'.
2. Add a 'sources' target under products project for files indexing.
3. Delete xctests/ folder to fix duplicate xctest file copies.

BUG=679110
==========

to

==========
Consolidate Xcode Project Setup

1. Remove 'sources.xcodeproj' and 'all.xcworkspace'.
2. Add a 'sources' target under products project for files indexing.
3. Delete xctests/ folder to fix duplicate xctest file copies.

BUG=679110
==========

[justincohen, 2017-01-12 23:38:23]

Hmm, one of the tentative plans for helping external developers work with iOS
Chromium involved adding a helper project to all.xcworkspace.  I wonder how that
might work with all.xcworkspace going away.

[justincohen, 2017-01-12 23:38:24]

Hmm, one of the tentative plans for helping external developers work with iOS
Chromium involved adding a helper project to all.xcworkspace.  I wonder how that
might work with all.xcworkspace going away.

[liaoyuke, 2017-01-12 23:44:01]

I didn't take that into consideration. Or we can keep the all.xcworkspace
and only remove the sources.xcodeproj. What do you think?

On Thu, Jan 12, 2017 at 3:38 PM <justincohen@chromium.org> wrote:

> Hmm, one of the tentative plans for helping external developers work with
> iOS
> Chromium involved adding a helper project to all.xcworkspace. I wonder how
> that
> might work with all.xcworkspace going away.
>
> https://codereview.chromium.org/2623203004/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[justincohen, 2017-01-12 23:45:17]

it should be very low overhead keeping around all, although it's certainly not
necessary today.  what do you think sdefrense@ ?

[liaoyuke, 2017-01-12 23:54:00]

I would stick to YAGNI to keep things simple, and add it back when
necessary, but let's wait for Sylvain's opinion.

On Thu, Jan 12, 2017 at 3:45 PM <justincohen@chromium.org> wrote:

> it should be very low overhead keeping around all, although it's certainly
> not
> necessary today. what do you think sdefrense@ ?
>
> https://codereview.chromium.org/2623203004/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[justincohen, 2017-01-13 01:10:04]

It may also be annoying telling users to open a different project every few
months if this changes...  but i'm fine with either approach.

[sdefresne, 2017-01-13 09:00:26]

On 2017/01/13 01:10:04, justincohen wrote:
> It may also be annoying telling users to open a different project every few
> months if this changes...  but i'm fine with either approach.

I'd prefer if we kept the all.xcworkspace.

[sdefresne, 2017-01-13 09:01:49]

On 2017/01/13 09:00:26, sdefresne wrote:
> On 2017/01/13 01:10:04, justincohen wrote:
> > It may also be annoying telling users to open a different project every few
> > months if this changes...  but i'm fine with either approach.
> 
> I'd prefer if we kept the all.xcworkspace.

Reason for that is that I don't want to tell people to switch to another file
(and then in six week explain again that it was called all.xcworkspace because
they checked out a branch that was created before this CL rolled). Better to
avoid moving too many pieces.

[liaoyuke, 2017-01-13 22:10:27]

Description was changed from

==========
Consolidate Xcode Project Setup

1. Remove 'sources.xcodeproj' and 'all.xcworkspace'.
2. Add a 'sources' target under products project for files indexing.
3. Delete xctests/ folder to fix duplicate xctest file copies.

BUG=679110
==========

to

==========
Consolidate Xcode Project Setup

1. Remove 'sources.xcodeproj'
2. Add a 'sources' target under products project for files indexing.
3. Delete xctests/ folder to fix duplicate xctest file copies.

BUG=679110
==========

[liaoyuke, 2017-01-13 22:11:22]

On 2017/01/13 09:01:49, sdefresne wrote:
> On 2017/01/13 09:00:26, sdefresne wrote:
> > On 2017/01/13 01:10:04, justincohen wrote:
> > > It may also be annoying telling users to open a different project every
few
> > > months if this changes...  but i'm fine with either approach.
> > 
> > I'd prefer if we kept the all.xcworkspace.
> 
> Reason for that is that I don't want to tell people to switch to another file
> (and then in six week explain again that it was called all.xcworkspace because
> they checked out a branch that was created before this CL rolled). Better to
> avoid moving too many pieces.

Thank you all for the comments! Now I think it makes more sense to keep
all.xcworkspace.

PTAL!

[liaoyuke, 2017-01-17 05:01:35]

On 2017/01/13 22:11:22, liaoyuke wrote:
> On 2017/01/13 09:01:49, sdefresne wrote:
> > On 2017/01/13 09:00:26, sdefresne wrote:
> > > On 2017/01/13 01:10:04, justincohen wrote:
> > > > It may also be annoying telling users to open a different project every
> few
> > > > months if this changes...  but i'm fine with either approach.
> > > 
> > > I'd prefer if we kept the all.xcworkspace.
> > 
> > Reason for that is that I don't want to tell people to switch to another
file
> > (and then in six week explain again that it was called all.xcworkspace
because
> > they checked out a branch that was created before this CL rolled). Better to
> > avoid moving too many pieces.
> 
> Thank you all for the comments! Now I think it makes more sense to keep
> all.xcworkspace.
> 
> PTAL!

PING?

[sdefresne, 2017-01-17 10:08:40]

https://codereview.chromium.org/2623203004/diff/20001/ios/build/tools/convert...
File ios/build/tools/convert_gn_xcodeproj.py (right):

https://codereview.chromium.org/2623203004/diff/20001/ios/build/tools/convert...
ios/build/tools/convert_gn_xcodeproj.py:170: # Copy sources project and all
workspace.
nit: Please update comment (as this no longer copy "sources" project).

https://codereview.chromium.org/2623203004/diff/20001/ios/build/tools/convert...
ios/build/tools/convert_gn_xcodeproj.py:178: if 'sources.xcodeproj' in
os.listdir(input_dir):
nit: I would just test whether the file "sources" exists:

  sources = os.path.join('sources.xcodeproj', 'project.pbxproj')
  if os.path.isfile(os.path.join(input_dir, sources)):
    CopyFileIfChanged(...)

https://codereview.chromium.org/2623203004/diff/20001/tools/gn/xcode_writer.cc
File tools/gn/xcode_writer.cc (right):

https://codereview.chromium.org/2623203004/diff/20001/tools/gn/xcode_writer.c...
tools/gn/xcode_writer.cc:499:
xctest_file_lists[std::find(xctest_module_targets.begin(),
nit: I find this line really confusing (I had to read it three time before
parsing it correctly), I suggest introducing a local variable to make it clearer

    auto iter = std::find(xctest_module_targets.begin(),
                          xctest_module_targets.end(), target);
    const size_t xctest_module_target_index = std::distance(
        xctest_module_targets.begin(), iter);
    const Target::FileList& xctest_file_lists[xctest_module_target_index];

Or maybe better, change xctest_file_lists to be a std::map<const Target*,
Target::FileList> so that you do not have to keep two std::vector<> indices in
sync.

[liaoyuke, 2017-01-17 18:35:19]

PTAL!

Thank you very much!

https://codereview.chromium.org/2623203004/diff/20001/ios/build/tools/convert...
File ios/build/tools/convert_gn_xcodeproj.py (right):

https://codereview.chromium.org/2623203004/diff/20001/ios/build/tools/convert...
ios/build/tools/convert_gn_xcodeproj.py:170: # Copy sources project and all
workspace.
On 2017/01/17 10:08:40, sdefresne wrote:
> nit: Please update comment (as this no longer copy "sources" project).

Done.

https://codereview.chromium.org/2623203004/diff/20001/ios/build/tools/convert...
ios/build/tools/convert_gn_xcodeproj.py:178: if 'sources.xcodeproj' in
os.listdir(input_dir):
On 2017/01/17 10:08:40, sdefresne wrote:
> nit: I would just test whether the file "sources" exists:
> 
>   sources = os.path.join('sources.xcodeproj', 'project.pbxproj')
>   if os.path.isfile(os.path.join(input_dir, sources)):
>     CopyFileIfChanged(...)

Done.

https://codereview.chromium.org/2623203004/diff/20001/tools/gn/xcode_writer.cc
File tools/gn/xcode_writer.cc (right):

https://codereview.chromium.org/2623203004/diff/20001/tools/gn/xcode_writer.c...
tools/gn/xcode_writer.cc:499:
xctest_file_lists[std::find(xctest_module_targets.begin(),
On 2017/01/17 10:08:40, sdefresne wrote:
> nit: I find this line really confusing (I had to read it three time before
> parsing it correctly), I suggest introducing a local variable to make it
clearer
> 
>     auto iter = std::find(xctest_module_targets.begin(),
>                           xctest_module_targets.end(), target);
>     const size_t xctest_module_target_index = std::distance(
>         xctest_module_targets.begin(), iter);
>     const Target::FileList& xctest_file_lists[xctest_module_target_index];
> 
> Or maybe better, change xctest_file_lists to be a std::map<const Target*,
> Target::FileList> so that you do not have to keep two std::vector<> indices in
> sync.

Done.

[sdefresne, 2017-01-18 09:42:42]

lgtm

[justincohen, 2017-01-18 16:41:39]

lgtm

[liaoyuke, 2017-01-18 17:29:07]

liaoyuke@chromium.org changed reviewers:
+ dpranke@chromium.org

[liaoyuke, 2017-01-18 17:30:34]

On 2017/01/18 16:41:39, justincohen wrote:
> lgtm

Thank you very much for reviewing!

[liaoyuke, 2017-01-18 17:31:07]

On 2017/01/18 17:30:34, liaoyuke wrote:
> On 2017/01/18 16:41:39, justincohen wrote:
> > lgtm
> 
> Thank you very much for reviewing!

Hello Dirk,

Could you please take a look for owner's approval?

[Dirk Pranke, 2017-01-18 20:20:21]

lgtm

[liaoyuke, 2017-01-18 20:21:22]

The CQ bit was checked by liaoyuke@chromium.org

[liaoyuke, 2017-01-18 20:21:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
sdefresne@chromium.org, justincohen@chromium.org
Link to the patchset: https://codereview.chromium.org/2623203004/#ps60001
(title: "Rebase")

[liaoyuke, 2017-01-18 20:21:27]

Thank you for the quick response :)

On Wed, Jan 18, 2017 at 12:20 PM <dpranke@chromium.org> wrote:

> lgtm
>
>
>
> https://codereview.chromium.org/2623203004/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2017-01-18 20:22:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-18 20:43:21]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484770882972850,
"parent_rev": "653edfe298e0b3ccda6a417befa060d1d8801876", "commit_rev":
"de19976ca12bb4c42acd5bfb5a83a69047d6e7b4"}

[commit-bot: I haz the power, 2017-01-18 20:43:48]

Description was changed from

==========
Consolidate Xcode Project Setup

1. Remove 'sources.xcodeproj'
2. Add a 'sources' target under products project for files indexing.
3. Delete xctests/ folder to fix duplicate xctest file copies.

BUG=679110
==========

to

==========
Consolidate Xcode Project Setup

1. Remove 'sources.xcodeproj'
2. Add a 'sources' target under products project for files indexing.
3. Delete xctests/ folder to fix duplicate xctest file copies.

BUG=679110

Review-Url: https://codereview.chromium.org/2623203004
Cr-Commit-Position: refs/heads/master@{#444469}
Committed:
https://chromium.googlesource.com/chromium/src/+/de19976ca12bb4c42acd5bfb5a83...
==========

[commit-bot: I haz the power, 2017-01-18 20:43:50]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/de19976ca12bb4c42acd5bfb5a83...