-
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
Enabled architectures, fixes #357 #358
Conversation
@brunobowden ptal; |
ba3d364
to
8eb7da5
Compare
void testSupportedAndEnabledArchs_SubsetEnabledArchsSpecified() { | ||
// Multiple assignment not supported with static-typing. | ||
Object[] projObjs = TestingUtils.setupProject( | ||
new TestingUtils.ProjectConfig(createJ2objcConfig: true, |
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.
Add method that returns only the j2objcConfig from setting up a project. Something like:
J2objcConfig j2objcConfig = TestingUtils.setupProjectJ2objcConfig(
new TestingUtils.ProjectConfig(createJ2objcConfig: true,
extraLocalProperties: ['j2objc.enabledArchs=ios_armv7,ios_armv7s']))
That'd be much neater than the temp projObjs array.
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.
done
I like different items in the stack to be more closely related to each other. For example, the name of supportedArchs and enabledArchs should be the same. Given the local.properties configuration, do we still need to have a build.gradle setting? Perhaps this is all better converted to a disableArch setting? That way, the impact of the j2objcConfig and local.properties is cumulative. It would also have a consistent name and clear behaviour. Please have a ponder about this. |
supportedArchs in build.gradl is necessary. The project may just want to exist on later iOS platforms, or perhaps just x64 for OSX, etc. It is a property of the project (in source control) of what platforms it supports. What subset of those platforms you want to build at any given time I think should still be environment-specific. I think enabledArchs is better than disabledArchs, because if you want to just do a debug loop on one device/simulator/etc, you just want to enable one architecture. I agree though on the confusion between names, let me think about that a bit more. |
Alright here is my suggestion:
Why? This is consistent with XCode from what I can tell. But in Xcode your only options are having 1 or all architectures that your project supports being active in the build. This is also consistent with our own names like 'debug' or 'release' /enabled/, being the environment-specific setting. Updated appropriately. |
Similar to configuring debug, release, and translateOnly on a per-environment basis for developer convenience, permit enabling architectures in the environment via local.properties. This is separate and in addition to setting the /supported/ architectures in the build file itself. The final architectures built are the intersection of the supported architectures and the per-environment enabled ones (if any).
8eb7da5
to
2ae4450
Compare
ptal |
LGTM - I prefer it differently but this is fine |
Enabled architectures, fixes #357
Similar to configuring debug, release, and translateOnly on a per-environment basis for developer convenience, permit enabling architectures in the environment via local.properties.
This is separate and in addition to setting the /supported/ architectures in the build file itself. The final architectures built are the intersection of the supported architectures and the per-environment enabled ones (if any).