Issue #169 — Reorganize project as an app bundle by ahtierney · Pull Request #204 · SwiftGen/SwiftGen

ahtierney · 2016-11-19T01:15:20Z

This pull request covers the remainder of issue #169 involving the conversion to Swift 3 and project restructuring. The last 3 commits are regarding the app bundle / project restructure.

NOTE: This is built on top of PR #201 and should be merged after or instead of that request.

djbe · 2016-11-20T12:42:53Z

Good! Only thing I can note is your changelog entries' format:

* Description of the change.••
  [Author Name](GH profile link)
  [#PRNum](link to PR)

With • to be replaced by a space (ensures that the author and PR links are on a new line).

AliSoftware

I haven't reviewed this in detail, but I'm gonna mark it as rejected: don't take it the wrong way, it's probably good, but it's gonna be super-hard to review the diff given how heavy it is from containing both the changes from #201 and the changes just to make it an app bundle, so it's just to keep in mind that we'll have to wait for 201 to be ready an merged, then rebase that on top of it so that this PR only contains the relevant changes to make it an app bundle, which would make it way easier to review and cleaner to merge 😉

AliSoftware · 2016-11-25T23:22:40Z

Time to rework this PR now that #201 has been modified then merged! 🎉

Could you please rebase this on top of master so that it now only include th commits related to the "app bundle migration" change?

Thanks!

ahtierney · 2016-11-28T04:46:32Z

Okay! so this should be ready for a look.

Rebased off master
.app is runnable and buildable in xcode
- you can pass args by editing the scheme (we may want to leave some in there as they can be toggled)
- note I had to disable nsdocumentrevisionsdebugmode (was parsed as command line args)
Rake now builds a more .app like Resources structure

Let me know what you think.

AliSoftware · 2016-11-28T22:05:45Z

Rakefile

  bindir = args.bindir.nil? || args.bindir.empty? ? Pathname.new('./build/swiftgen/bin') : Pathname.new(args.bindir)
  fmkdir = args.fmkdir.nil? || args.fmkdir.empty? ? bindir + '../lib' : Pathname.new(args.fmkdir)
-  tpldir = args.tpldir.nil? || args.tpldir.empty? ? bindir + '../templates' : Pathname.new(args.tpldir)
+  tpldir = args.tpldir.nil? || args.tpldir.empty? ? bindir + '../Resources/templates' : Pathname.new(args.tpldir)


I'm not sure I get why you changed this?
The point of the Rakefile is to distribute SwiftGen as a command line tool in the format expected by homebrew and structured like other executables you typically have in /use/local/bin or similar non-app-bundled tools, which don't have a Resources folder

@AliSoftware Great question, my thinking was it'd be best to keep as much parity as makes sense between the debug and the packaged version. Based on the rake install step I assumed that adding the default value would be the lowest touch way to keep things harmonious (if the debug version requires default path Resources/templates running install will overwrite the main.swift file and require reset). However I certainly understand that the .app is a secondary feature. Some other options I considered were:

A build script to package the .app more like the cli tool (discussed here)

Using a different relative path value depending on the target.

Resetting the path value in main.swift after the install step finishes in the rake file.

Let me know if any of these options appeal to you and I'd be happy to implement it (or if I'm off in the wrong direction all together 😬)

Ah, I see.

But I think even without this PR, the main.swift was modified by the Rakefile anyway, as it can always be installed in any place you like depending on the rake task arguments…

And I'd still prefer keeping the structure of the final binary untouched, first because it's more standard, second because changing it would mean adapting the Homebrew's formula too, and because the final product is more important and the .app is just a convenience structure for development, not for distribution.

So I think the best option is to:

Keep the templates in Resources for the development mode (.app bundle) — We could even point the templatesRelativePath to NSBundle.main.resourcesDirectory to make it more dynamic? — because that's what .app bundles expect and the place where Xcode will put them

Keep the Rakefile changing that value to $tpldir, which by default if not specified when calling rake, would be ../templates to match what Homebrew and CLI binaries expect

Don't git reset main.swift at the end of rake. The Rakefile already did change the main.swift without reverting it before that PR, and I think that's OK:

now that the project will be organized as an app bundle, we should only have to use the Rakefile to do a release, so not that often anyway

I prefer to have a dirty git working copy after the rake install and handle the reset manually than having rake do a git checkout main.swift to reset it… and potentially accidentally also revert other changes that we might have forgotten to commit before calling rake install (another option would be to ensure that everything has been committed to git before authorizing to continue to rake install but ¯_(ツ)_/¯ )

So, in summary, commit the value of the relativePath that matches the relative path when the .app bundle stores the templates once compiled, so that a Cmd-R (and Cmd-U) works out of the box without changes from Xcode, and keep the Rakefile behavior as before, still letting it change the templateRelativePath during the rake install to match where the templates are asked to be installed according to the rake install[…] parameters, which would still default to ../templates.

ahtierney · 2016-11-30T05:59:38Z

@AliSoftware incorporated your feedback and rebased again 👍

AliSoftware · 2016-12-01T13:11:39Z

swiftgen-cli/main.swift


-let templatesRelativePath = "../Resources/templates"
-
+let templatesRelativePath = (Bundle.main.resourcePath ?? "") + "/templates"


I'd use ?? "." instead. Because in case Bundle.main.resourcePath returns nil, we would end up pointing to the root of the filesystem, which seems dangerous 😉

AliSoftware · 2016-12-01T13:13:45Z

Just a small change requested and I think (at least from what I read from GitHub) that we're good to go.

Maybe if I have more time later I'll pull it and test and run on my Mac in Xcode (or if anyone can do it too to validate that in practice) before we (finally!) merge.

AliSoftware · 2016-12-01T13:22:45Z

Wait… I think at some point I disabled the EMBEDDED_CONTENT_CONTAINS_SWIFT build settings back in the day when I configured SwiftGen as a command-line target (because Xcode doesn't know how to embed the Swift dylibs if the build product isn't packaged as a bundle). Btw, this caused an execution of pod install or pod update to generate a warning, because the project overwrote this build settings (while we shouldn't, or at least should $(inherit))

I'm surprised not to see that Build Setting being changed in this PR's diff? Shouldn't this (well, more likely the new ALWAYS_EMBED_SWIFT_STANDARD_LIBRARIES replacing it) be present again now?

🤔 I definitely will have to pull this on my Mac and inspect/test this. Diff'ing a pbxproj directly on GitHub isn't easy ^^

djbe · 2016-12-01T21:55:19Z

I can confirm that ALWAYS_EMBED_SWIFT_STANDARD_LIBRARIES is set to Yes.

It runs from within Xcode, and also runs when moving the produced app (and deleting the other produced content such as frameworks), so there are definitely no dependencies left.

- Copying templates to /Resources/templates - Modified rake to package accordingly

- added bundle resources to path - clean up swiftlint whitespace errs

ahtierney · 2016-12-02T05:08:12Z

@AliSoftware Good catch on the root, I've rebased and pushed the fix. Re: the .pbxproj file, for some reason it's been collapsed in this pull request (maybe this is what you're referring to?) there are changes and ALWAYS_EMBED_SWIFT_STANDARD_LIBRARIES is flagged set to yes. Let me know if you'd like it to $(inherit) (I believe there is still a warning when you pod update/install).

AliSoftware · 2016-12-02T11:26:49Z

Indeed, I think we should be sure to get rid of the pod update/install warning.
Probably no need to set the value to $(inherited), just remove the value completely from the Target level so that we just don't override the one set on the Config.File level.

ahtierney · 2016-12-02T15:57:53Z

@AliSoftware agree! pushed the fix.

AliSoftware

Still haven't have time to git pull that branch to test it in practice in Xcode, but if you assure me that it now doesn't warn upon pod install / pod update anymore now that ALWAYS_EMBED_SWIFT_LIBRARIES isn't overridden, then we should be good to go!

djbe · 2016-12-02T16:50:22Z

This is the output I'm getting with the latest code:

~/D/L/SwiftGen2 ❯❯❯ pod update feature/ahtierney/app-bundle
Update all pods
Updating local specs repositories
Analyzing dependencies
Fetching podspec for GenumKit from GenumKit
Downloading dependencies
Using Commander (0.6.0)
Using GenumKit (4.0.0)
Using PathKit (0.7.0)
Using Stencil (0.6.0)
Generating Pods project
Integrating client project
Sending stats
Pod installation complete! There are 4 dependencies from the Podfile and 4 total pods installed.
~/D/L/SwiftGen2 ❯❯❯

So it's good to go!

AliSoftware · 2016-12-02T17:03:49Z

I've finally got some time to pull and test in Xcode. Great work!

AliSoftware · 2016-12-02T17:09:13Z

Just opened a separate issue #221, as testing #204 in Xcode makes me realise that the ANSI color codes are printed in the Xcode console even if Xcode console doesn't support colors (RIP XcodeColors plugin in Xcode 8 😢 ), but that's a separate issue.

…e toggled ON/OFF easily

AliSoftware · 2016-12-02T17:22:50Z

I've remove the XcodeColors env var (as we now use Xcode 8 so no more XcodeColors) and added some sample launch arguments to make it easy to toggle them and test the various scenarios from Xcode.

[EDIT] It seems that I managed to push my commit anyway somehow, even with the error I had on my git push… not sure how it worked 😹

AliSoftware · 2016-12-03T07:55:02Z

🎉

AliSoftware mentioned this pull request Nov 20, 2016

Convert project to Swift 3 and app bundle #169

Closed

2 tasks

AliSoftware changed the title ~~Issue #169~~ Issue #169 — Reorganize project as an app bundle Nov 20, 2016

AliSoftware mentioned this pull request Nov 20, 2016

Cut a release to 3.0.2 #196

Closed

AliSoftware requested changes Nov 20, 2016

View reviewed changes

djbe modified the milestone: Next patch (4.0.1) Nov 20, 2016

AliSoftware added the type: internal label Nov 25, 2016

ahtierney mentioned this pull request Nov 25, 2016

Update project to Swift 3 #201

Merged

3 tasks

AliSoftware added the type: enhancement label Nov 25, 2016

ahtierney force-pushed the feature/ahtierney/app-bundle branch 2 times, most recently from 1e9adde to 4079605 Compare November 28, 2016 04:36

AliSoftware reviewed Nov 28, 2016

View reviewed changes

ahtierney force-pushed the feature/ahtierney/app-bundle branch from 4079605 to 62b0cee Compare November 30, 2016 05:54

AliSoftware mentioned this pull request Dec 1, 2016

Images: dot syntax #206

Merged

AliSoftware requested changes Dec 1, 2016

View reviewed changes

ahtierney added 4 commits December 1, 2016 20:29

- Restructure project to build .app

54d26a9

- Copying templates to /Resources/templates - Modified rake to package accordingly

- Changelog

bfa312a

- revert rakefile

67a7a88

- added bundle resources to path - clean up swiftlint whitespace errs

- Change fallback path to current directory "."

4f6e181

ahtierney force-pushed the feature/ahtierney/app-bundle branch from 62b0cee to 4f6e181 Compare December 2, 2016 05:05

- reset swift embed flag

9069cb9

AliSoftware approved these changes Dec 2, 2016

View reviewed changes

djbe added the status: ready to merge (waiting for CI) label Dec 2, 2016

Merge branch 'master' into feature/ahtierney/app-bundle

b57016f

AliSoftware mentioned this pull request Dec 2, 2016

Only generate ANSI color codes if terminal supports it #221

Closed

Added some default launch arguments in the swiftgen-cli.xcscheme to b…

e2e9dd4

…e toggled ON/OFF easily

djbe merged commit e9eaf7b into SwiftGen:master Dec 3, 2016

AliSoftware mentioned this pull request Mar 2, 2017

Configure CircleCI #269

Merged

djbe removed the status: ready to merge (waiting for CI) label Jun 24, 2020


		let templatesRelativePath = "../Resources/templates"

		let templatesRelativePath = (Bundle.main.resourcePath ?? "") + "/templates"

Conversation

ahtierney commented Nov 19, 2016

Uh oh!

djbe commented Nov 20, 2016

Uh oh!

AliSoftware left a comment

Choose a reason for hiding this comment

Uh oh!

AliSoftware commented Nov 25, 2016

Uh oh!

ahtierney commented Nov 28, 2016

Uh oh!

AliSoftware Nov 28, 2016

Choose a reason for hiding this comment

Uh oh!

ahtierney Nov 29, 2016

Choose a reason for hiding this comment

Uh oh!

AliSoftware Nov 29, 2016

Choose a reason for hiding this comment

Uh oh!

ahtierney commented Nov 30, 2016

Uh oh!

AliSoftware Dec 1, 2016

Choose a reason for hiding this comment

Uh oh!

AliSoftware commented Dec 1, 2016

Uh oh!

AliSoftware commented Dec 1, 2016

Uh oh!

djbe commented Dec 1, 2016

Uh oh!

ahtierney commented Dec 2, 2016

Uh oh!

AliSoftware commented Dec 2, 2016

Uh oh!

ahtierney commented Dec 2, 2016

Uh oh!

AliSoftware left a comment

Choose a reason for hiding this comment

Uh oh!

djbe commented Dec 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AliSoftware commented Dec 2, 2016

Uh oh!

AliSoftware commented Dec 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AliSoftware commented Dec 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AliSoftware commented Dec 3, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

djbe commented Dec 2, 2016 •

edited

Loading

AliSoftware commented Dec 2, 2016 •

edited

Loading

AliSoftware commented Dec 2, 2016 •

edited

Loading