lhopps / grit-i18n

Automatically exported from code.google.com/p/grit-i18n
BSD 2-Clause "Simplified" License
0 stars 0 forks source link

grit should differentiate host and target os #10

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
grit should follow the same convention as gyp,
which is that iOS and Mac are different.

https://code.google.com/searchframe#OAMlx_jo-ck/src/tools/grit/grit/node/base.py
&exact_package=chromium&q=is_macosx&l=467

The specific case is for messages that are iOS only.
Current workaround is to test for pp_ifdef('ios').

Original discussion:
https://codereview.chromium.org/11966004/

Original issue reported on code.google.com by bul...@chromium.org on 18 Jan 2013 at 1:31

GoogleCodeExporter commented 9 years ago
Specifically, the problem is that sys.platform is reading the host, and iOS 
builds don't have the host == target property that the desktop builds do.

Original comment by stuartmorgan@chromium.org on 18 Jan 2013 at 2:01

GoogleCodeExporter commented 9 years ago
I have no objection to this behavior being changed, assuming it affects only 
builds targeting iOS.  I'm happy to look at patches that fix this bug.

For completeness of information in the bug, where should the target OS property 
come from?  A command-line option, an environment variable, or what?

Original comment by joi@chromium.org on 21 Jan 2013 at 10:37

GoogleCodeExporter commented 9 years ago
We already pass "-D ios" to grit, so using that seems like the easiest option.

Original comment by stuartmorgan@chromium.org on 21 Jan 2013 at 10:57

GoogleCodeExporter commented 9 years ago
I'm not sure I think it's a good idea to use "-D ios" as the basis of a generic 
solution.  It seems that we either:

a) Do something for iOS only, in which case there doesn't seem to be a need to 
make any change (you are already passing "-D ios" and checking that in your 
<if> conditions); or

b) Do something generic for all platforms, in which case we should have, in 
addition to the current is_linux, is_win etc. variables available in the <if> 
statement, something like target_is_linux, target_is_win, etc. and have some 
generic flag (not a -D flag, and not one flag per platform, but rather 
something like --target=xyz) that sets the target platform for GRIT.

WDYT?

Original comment by joi@chromium.org on 21 Jan 2013 at 1:35

GoogleCodeExporter commented 9 years ago
Sure, I'm fine with an optional --target flag that overrides the 
auto-detection. I'm not clear on why we'd need is_foo and target_is_foo though; 
in a grd file wouldn't you only ever care about the target? I'd think the logic 
would be to set the existing variables based on the flag if present, and the 
host if not.

Original comment by stuartmorgan@chromium.org on 21 Jan 2013 at 2:16

GoogleCodeExporter commented 9 years ago
Good point, agreed the flag should simply change the behavior of the existing 
is_xyz variables.

I need to fix another bug in GRIT today, and since this looks simple I may fix 
it while I'm in there.

Original comment by joi@chromium.org on 21 Jan 2013 at 2:36

GoogleCodeExporter commented 9 years ago
There's a related bug for Android at 
https://code.google.com/p/chromium/issues/detail?id=239434 and I have a CL to 
fix it. I didn't see a need to split the host and target, as stuartmorgan 
suggests in #5, so I just added an is_android condition and have chromium 
passing -t android instead of -D android.

iOS could do the same thing. I don't particularly want to add this to my CL 
right now because I need this fixed fairly urgently to unbreak the WebView and 
I don't have the environment to test building iOS, but feel free to copy the 
approach once that lands :)

Original comment by torne@chromium.org on 14 May 2013 at 11:40

GoogleCodeExporter commented 9 years ago
OK, I've landed the android changes for this. I suggest that someone from the 
iOS side does exactly the same thing as the changes I made for 
https://code.google.com/p/chromium/issues/detail?id=239434 :)

Original comment by torne@chromium.org on 21 May 2013 at 3:43

GoogleCodeExporter commented 9 years ago
is_ios was landed at r123. We can now use it instead of pp_ifdef('ios').

Original comment by tfar...@chromium.org on 21 Jun 2013 at 2:18

GoogleCodeExporter commented 9 years ago
I believe this is fixed, right? Marking fixed, torne@/stuartmorgan@/bulach@ 
please reopen if I'm wrong.

Original comment by joi@chromium.org on 28 Jun 2013 at 10:33

GoogleCodeExporter commented 9 years ago
Yep, I just need to change the existing uses now.

Original comment by stuartmorgan@chromium.org on 28 Jun 2013 at 10:38