-
Notifications
You must be signed in to change notification settings - Fork 43
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 a translateOnly mode that skips compilation-dependent tasks. #349
Conversation
cc: @gocursor |
9ad56b2
to
67f5bfb
Compare
I have tested the new "translateOnlyMode" and it works! Configuration is much cleaner now... Thank you again for your fast response! |
How often do you make releases? Or: when can I expect the "translateOnlyMode" flag in the official release? What is the preffered way to work with the lastest version on GutHub: to clone it locally and build it by yourself? |
0.5.0 is scheduled for Aug 31 but we don't have any set schedule. |
|
||
// Support translation-only mode. | ||
if (translateOnlyMode) { | ||
project.tasks.all { Task task -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can go in the project.tasks.all loop above, otherwise you're repeating the same loop twice. Also, the "|Objc" is redundant as all tasks are covered by previous terms. I've also expanded the J2objc|j2objc for clarity in the regex.
Here's a possible version:
boolean debugEnabled = Boolean.parseBoolean(Utils.getLocalProperty(project, 'debug.enabled', 'true'))
boolean releaseEnabled = Boolean.parseBoolean(Utils.getLocalProperty(project, 'release.enabled', 'true'))
project.tasks.all { Task task ->
String name = task.name
// Matches plugin-specific debug tasks beyond translation
if (name =~ /^j2objc(Assemble|PackLibraries|Test)Debug$/).matches()) {
if (translateOnly || ! debugEnabled) {
task.enabled = false
}
}
// Matches plugin-specific release tasks beyond translation
if (name =~ /^j2objc(Assemble|PackLibraries|Test)Release$/).matches()) {
if (translateOnly || ! releaseEnabled) {
task.enabled = false
}
}
// Matches all native-compilation tasks.
if ((name =~ /^.*((J2objcExecutable|j2objcStaticLibrary|j2objcSharedLibrary))$/).matches()) {
if (translateOnly) {
task.enabled = false
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I don't see how Objc suffix is redundant. when I do it without that, the compile steps for the test binary still run, even though nothing is linked. we want to eliminate compiling, linking and executable generation.
- I kept the loops separate because I think it is clearer to separate the debug/release functionality from translateOnly. However I'll combine the tasks.all loop without combining the regex/if statements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah i think i see the confusion, your patterns would work if gradle didn't link in the native build steps into the lifecycle assemble task automatically. but it does, so we have to disable those other debug and release tasks explicitly (which my code does)
67f5bfb
to
3127778
Compare
@brunobowden - from your comments I think there is some confusion over the intent of this code. I do not think it is correct to automatically detect Windows and have that turn on this flag. The spiritual equivalent would be for example, detecting no Android SDK is present on a computer and skipping the Android build steps instead of failing them. A user can indeed override the system to prevent a full build, but it is never the automatic behavior. |
@gocursor - it would be good to have your input on this as well. My thought is imagine an office that is evenly split between Windows and OS X developers. You want them to use the same process and same configuration as much as possible. With a translateOnly flag, the Windows developers will turn it on... and then naturally commit that change. When the OS X developer finds that breaks their iOS build, they revert that change and commit it as well. Now you've got a fight between them on whether translateOnly should be enabled as they each need something different. To require one half of the to edit that file before doing development would be painful and error prone to say the least. I agree with you that it's bad that someone can do a build and then have it be incomplete. My point is that is less painful compared to the scenario I describe above where I'm expecting the two teams to constantly fight with each other. To mitigate your concern, I think the flag name should be translateOnlyForLImitedPlatforms... or disableTasksForUnsupportedPlatforms, along with an appropriately strong warning of the implications. I'm open on the name but I think that's the functionality that the users will need. It does break the pureness that you speak of... but the name means that the user has explicitly recognized the trade off that they're making. @gocursor - please share your thoughts |
@brunobowden - I understand what you're saying. But I don't see how it is different from any other missing component in a toolchain. If my Mac machine were missing an up-to-date JDK, the build can and should fail, etc - and I shouldn't really have a way to pretend it succeeded... I'd find it confusing if a build built different things on different platforms. If the local environment needs to be taken into account, I think we should go with our existing environment variable and/or local.properties keys solution, just like j2objc.home and j2objc.debugEnabled. So if you set j2objc.translateOnlyMode to true in local.properties, this new config variable will also be set to true (within the finalConfigure call). I'll keep the translateOnlyMode as a per-project setting as well, since sometimes you really do only want the objective-c code (regardless of platform issues). I think everybody gets what they want with that solution. SG @brunobowden and @gocursor ? |
I'd be ok with that. |
3127778
to
06ab861
Compare
cool, commit updated. new logic: https://github.com/j2objc-contrib/j2objc-gradle/pull/349/files#diff-0822dd6efa78a21cdd40aff6cad5646bR524 |
06ab861
to
2748f54
Compare
Sorry for delay - I am in Europe and you discussed during the night in Europe - while I was sleeping... :-) I would go for "translateOnlyMode" because the semantics is the same on all platforms... I see two different options for developing in mixed Windows / OS X environment:
For the 2. option there are two solutions:
I would go for "translateOnlyMode" - it's cleaner... But if somebody wants "translateOnlyForLImitedPlatforms", we can add this one as well and have both options... Or we can add this one upon first request... |
thanks @gocursor - here is the solution I'm proposing that I think solves your use-case. j2objc.translateOnlyMode=true Will that work for you? EDIT: Your option (1) of having a separate project to build the objective-c binaries was something I looked at a long while ago, but it requires the developer to do a lot more work (no longer can you use a single plugin) - the j2objc plugin cannot support such a design though, as it requires instantiating a new project, something Gradle doesn't let you programmatically do inside a plugin. If you do in fact want to build the objective-c binaries as a separate Gradle project, you can hand write a native compilation Gradle project. |
@gocursor - I like the solution from @advayDev1. It requires that all the Windows developers configure translateOnly in their local.properties file (not committed to the repository) and then they can share the same build configuration. I don't like the idea of separate build configuration, which could accidentally diverge from each other. |
2748f54
to
37d3a0c
Compare
37d3a0c
to
69a834c
Compare
The solution with "j2objc.translateOnlyMode=true" in local.properties under Windows is great! I like it and vote for it! |
Advay - thanks for helping us find a good compromise here. On Sun, Aug 16, 2015, 2:41 AM Cursor [email protected] wrote:
|
LGTM P.S. Can you add something to the FAQ describing this. On Sun, Aug 16, 2015, 8:58 AM Bruno [email protected] wrote:
|
@brunobowden - this cl already has the faq change: |
Add a translateOnly mode that skips compilation-dependent tasks.
Thanks @advayDev1. I know this isn't your normal purview but it will be helpful to a small segment of our userbase. |
@brunobowden - I just didn't realize people actually wanted to do this. :) However, please do think about whether you want to change the FAQ to say whether Windows is officially supported for translation or not. It still says it is not (but that you can use translateOnlyMode as a workaround). |
I would be ok with lessening the FAQ. I'll send you a PR for discussion. On Sun, Aug 16, 2015 at 12:54 PM Advay Mengle [email protected]
|
Related to #345; Fixes #347