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

Allow all local.properties flags to be overridden by environment #361

Merged
merged 1 commit into from
Aug 20, 2015

Conversation

advayDev1
Copy link
Contributor

Fixes #360

BREAKING API CHANGE:
J2OBJC_HOME environment value will now override j2objc.home
in local.properties instead of the other way around.

@advayDev1
Copy link
Contributor Author

@brunobowden ptal. please do #358 first

@brunobowden
Copy link
Contributor

Is there a precedence for this precedence? E.g. that the JVM or other tools have similar behaviour?

I've tried searching online but didn't get a good answer for this. The only example I could find seemed to suggest that local properties took precedence over the environment. I'm think that a system that has gone through battle testing on this front would have sound reasons for doing it that way. We should be careful about doing something difference.

http://www.javacodegeeks.com/2012/02/properties-with-spring.html

4.1 Properties Search Precedence
By default, in Spring 3.1, local properties are search last, after all environment property sources, including properties files. This behavior can be overridden via the localOverride property of the PropertySourcesPlaceholderConfigurer, which can be set to true to allow local properties to override file properties.

In Spring 3.0 and before, the old PropertyPlaceholderConfigurer also attempted to look for properties both in the manually defined sources as well as in the System properties. The lookup precedence was also customizable via the systemPropertiesMode property of the configurer:
never – Never check system properties

fallback (default) – Check system properties if not resolvable in the specified properties files
override – Check system properties first, before trying the specified properties files. This allows system properties to override any other property source.

Finally, note that in case a property is defined in two or more files defined via @propertysource – the last definition will win and override the previous ones. This makes the exact property value hard to predict, so if overriding is important, the PropertySource API can be used instead.

@advayDev1
Copy link
Contributor Author

So does Android I guess:
https://android.googlesource.com/platform/tools/build/+/master/gradle/src/main/groovy/com/android/build/gradle/internal/Sdk.groovy

alright, I'll swap the priority order back to properties overrides environment

@brunobowden
Copy link
Contributor

Another issue we'll have for the future... is when build.gradle allows you
to specific the J2ObjC version, we'll likely want that to be the preferred
configuration system.

On Wed, Aug 19, 2015 at 11:08 AM Advay Mengle [email protected]
wrote:

So does Android I guess:

https://android.googlesource.com/platform/tools/build/+/master/gradle/src/main/groovy/com/android/build/gradle/internal/Sdk.groovy

alright, I'll swap the priority order back to properties overrides
environment


Reply to this email directly or view it on GitHub
#361 (comment)
.

@advayDev1
Copy link
Contributor Author

ptal @brunobowden - put the order back to the way it was


This is helpful for a tight modify-compile-test loop using only debug binaries.
You can also do this for `j2objc.debug.enabled`.

If you'd rather just disable release builds for a particular run of the command line:
Copy link
Contributor

Choose a reason for hiding this comment

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

Empty line afterwards

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
Copy link
Contributor

LGTM - it would be nice to add a test for Utils.groovy for loading a property from an environment variable.

@advayDev1
Copy link
Contributor Author

filed a bug for mocking System.getenv - not sure how to at the moment

@brunobowden
Copy link
Contributor

Just add a TODO for now.

Nice update from Travis:

screen shot 2015-08-20 at 10 19 41 am

@advayDev1
Copy link
Contributor Author

Actually, it is baffling as it is a failure in JDK 8.0 only. JDK 7.0 is
working. Digging into it...

On Thu, Aug 20, 2015 at 10:20 AM Bruno Bowden [email protected]
wrote:

Just add a TODO for now.

Nice update from Travis:

[image: screen shot 2015-08-20 at 10 19 41 am]
https://cloud.githubusercontent.com/assets/679339/9390276/03c7074e-4725-11e5-8c2d-6687176c5bfd.png


Reply to this email directly or view it on GitHub
#361 (comment)
.

Fixes j2objc-contrib#360

local.properties values will override those in environment
@madvayApiAccess
Copy link
Contributor

well the tests are passing now. looks like we are flaky on JDK8 - i reran various commits on Travis over and over and JDK8 sometimes fails, sometimes passes.

i'm sending a PR to provide more info on such failures. right now it just tells you to look at the testreport.html which obviously we can't.

@brunobowden
Copy link
Contributor

It would be helpful to have a more detailed record, including the
testreport.html. Can that be maintained for a brief period, e.g. 24 hours?

On Thu, Aug 20, 2015 at 11:08 AM madvay public repo API account <
[email protected]> wrote:

well the tests are passing now. looks like we are flaky on JDK8 - i reran
various commits on Travis over and over and JDK8 sometimes fails, sometimes
passes.

i'm sending a PR to provide more info on such failures. right now it just
tells you to look at the testreport.html which obviously we can't.


Reply to this email directly or view it on GitHub
#361 (comment)
.

@advayDev1
Copy link
Contributor Author

We can't see the HTML file, but we can get the exception details:
http://mrhaki.blogspot.com/2013/05/gradle-goodness-show-more-information.html

see PR #373

advayDev1 added a commit that referenced this pull request Aug 20, 2015
Allow all local.properties flags to be overridden by environment
@advayDev1 advayDev1 merged commit fb04e92 into j2objc-contrib:master Aug 20, 2015
@advayDev1 advayDev1 deleted the env branch August 20, 2015 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants