Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add destDirTest option to save away test objc files. #56

Merged
merged 1 commit into from
May 13, 2015

Conversation

advayDev1
Copy link
Contributor

(Please first do PR #54 and #53 for easy merging)

You can make this a different directory or the same as destDir - it
will work either way.

This allows a convenient way to setup objc source sets
just like your java ones (think:
src-gen/main/objc
src-gen/test/objc
etc.) for easy gradle compilation.

TESTED=yes

if (destTestDir != destDir) {
// If we want main source and test source in one directory, then don't
// re-delete the main directory where we just put files!
prepDestDir destTestDir, 'destTestDir'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it easier to understand when it's function style rather than groovy. This is helpful for other engineers making changes:

prepDestDir(destTestDir, 'destTestDir')

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed PTAL

@advayDev1
Copy link
Contributor Author

@brunobowden - currently j2objc.gradle has a mix of groovy-style top-level method calls and java-style ones. for example, near the lines you commented on there was 'project.delete destDir'. also your readme uses groovy style in its example of how to configure the plugin. do you want to actively go towards java style top-level method calls? thanks!

@@ -739,6 +775,25 @@ class J2objcCopyTask extends DefaultTask {
File srcDir
// TODO: declare @OutputXXX so gradle knows if task is up to date

private def prepDestDir(File destDir, String name) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename to clearDestDirWithChecks as that makes it purpose clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed PTAL

@brunobowden
Copy link
Contributor

Hi @advayDev1 - I think the Java style function calls is better as that makes it clearer for other developers proposing changes. I realize that there's multiple places where that needs to be fixed up.

Please rebase / merge this with master and I'll take another look. Also, is the include / exclude classes still needed?

@advayDev1
Copy link
Contributor Author

@brunobowden - the include/exclude classes is from PR #54 (that commit is in both this branch and that one). i do use that code in my project as it is more convenient than making regexes for each class I exclude, and an example is provided on the declaration.

also, I will repair the java-style calls affected by this change. thanks for the feedback!

@advayDev1 advayDev1 force-pushed the testDir branch 2 times, most recently from a454a68 to 65a5940 Compare May 13, 2015 06:16
@advayDev1
Copy link
Contributor Author

rebased a new version for all comments except PR #54 and the open comment re: https://github.com/brunobowden/j2objc-gradle/pull/56#discussion_r30204264 - will modify further if you make a decision on the other PR and want to see the latter added complexity.

// Where to copy generated test files (excludes executable)
// If null, generated test files are discarded for final output.
// Can be the same directory as destDir.
String destTestDir = null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename to 'destDirTest' as that signifies closer association with 'destDir'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@brunobowden brunobowden changed the title Add destTestDir option to save away test objc files. Add destDirTest option to save away test objc files. May 13, 2015
You can make this a different directory or the same as destDir - it
will work either way.

This allows a convenient way to setup objc source sets
just like your java ones (think:
src-gen/main/objc
src-gen/test/objc
etc.) for easy gradle compilation.

TESTED=yes
@brunobowden
Copy link
Contributor

Just add the TODO and then I'll merge. I'm working on some changes with confile early tomorrow morning, so I'd like to get this in first.

@advayDev1
Copy link
Contributor Author

Per https://github.com/brunobowden/j2objc-gradle/pull/56#discussion_r30206228, TODO is no longer needed (cleaner this way anyway).

brunobowden added a commit that referenced this pull request May 13, 2015
Add destDirTest option to save away test objc files.
@brunobowden brunobowden merged commit cd675ce into j2objc-contrib:master May 13, 2015
@brunobowden
Copy link
Contributor

I merged in #54, thinking that you needed that for now ;-) Never mind, we'll clean this up later.

@advayDev1
Copy link
Contributor Author

Yeah actually there's no hurry in PRs on this repo I guess. I have various alterations to this plugin in my proprietary repo, and am extricating my changes (to skip private or thoroughly entangled changes) into my public fork and eventually your source repo.

@brunobowden
Copy link
Contributor

In the future, I think of this as not just a tool for shared build but also
for partial migration. Some of which you may be already dealing with.

On Wed, May 13, 2015 at 12:06 AM Advay Mengle notifications@github.com
wrote:

Yeah actually there's no hurry in PRs on this repo I guess. I have various
alterations to this plugin in my proprietary repo, and am extricating my
changes (to skip private or thoroughly entangled changes) into my public
fork and eventually your source repo.


Reply to this email directly or view it on GitHub
https://github.com/brunobowden/j2objc-gradle/pull/56#issuecomment-101542498
.

@advayDev1
Copy link
Contributor Author

Yep, I see what you mean. I see that @confile is working on making this easier to use with Xcode projects with integratej2objc. My workflow is a bit different in that I'm trying to use the xcode toolchain via Gradle[1] and with no prebuilts or checked-in generated source, as I do not want to deal with Xcode's workspace management for many of the reasons that make integratej2objc necessary or with denormalized iOS source (I have a single codebase that I want to run './gradlew build' from the root and have it build all my custom Groovy toolchain, my common Java libraries, my Android apk, generated objc, and my iOS app all at once in that order.) Also working on getting this plugin to work with ObjC++. I'll file issues so we can talk over things in an organized manner.

Edit: I know this kind of one codebase build would be more suited for bazel [2], but we'll have to wait until Google decides to make bazel the Recommended way to do Android app builds instead of Android Studio.

Also, wishing github PRs had batched/atomic code review comments/revisions right now =)

[1] https://gradle.org/docs/current/userguide/nativeBinaries.html
[2] https://github.com/google/bazel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants