Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2662513004:
Rewrite iOS FAT libraries build script in Python (Closed)
|
Created:
3 years, 10 months ago by oprypin_webrtc Modified:
3 years, 10 months ago CC:
webrtc-reviews_webrtc.org Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRewrite iOS FAT libraries build script in Python
BUG=webrtc:7049
Review-Url: https://codereview.webrtc.org/2662513004
Cr-Commit-Position: refs/heads/master@{#16453}
Committed: https://chromium.googlesource.com/external/webrtc/+/7a2d8ca9bc06f7cdc2cafeba02d57facd18614fc
Patch Set 1 #Patch Set 2 : Fix copying a directory tree (needs to support merging) #Patch Set 3 : Catch error when deleting a file (was it a mistake in the Bash script?) #
Total comments: 23
Patch Set 4 : Change according to feedback #
Total comments: 2
Patch Set 5 : Add verbose logging #Patch Set 6 : Drop the check for stray mobileprovision #Patch Set 7 : Rebase #Messages
Total messages: 29 (16 generated)
Description was changed from ========== Rewrite iOS FAT libraries build script in Python BUG=webrtc:7049 ========== to ========== Rewrite iOS FAT libraries build script in Python BUG=webrtc:7049 ==========
oprypin@webrtc.org changed reviewers: + kjellander@webrtc.org
kjellander@webrtc.org changed reviewers: + ehmaldonado@webrtc.org
Edward: isn't this trybot error the same we got last week? It builds all the platforms but fails on the last step when it's merging the libraries into a FAT one.
Phew, I'm unable to spot what may be causing the problem. Maybe my suggestion for logging the check_call invocations will help. Otherwise I hope you'll be able to figure it out when you got your Mac. https://codereview.webrtc.org/2662513004/diff/40001/tools-webrtc/ios/build_io... File tools-webrtc/ios/build_ios_libs.py (right): https://codereview.webrtc.org/2662513004/diff/40001/tools-webrtc/ios/build_io... tools-webrtc/ios/build_ios_libs.py:60: shutil.rmtree(output_dir, True) I'm not sure we want to ignore errors here. Since this is run only on Mac, we shouldn't really get errors when deleting the dir, and if we do I think we might be interested in seeing them as an exception. https://codereview.webrtc.org/2662513004/diff/40001/tools-webrtc/ios/build_io... tools-webrtc/ios/build_ios_libs.py:65: custom_gn_options=[]): empty lists as a default parameters in Python are scary, let's avoid it: http://stackoverflow.com/questions/366422/what-is-the-pythonic-way-to-avoid-d... https://codereview.webrtc.org/2662513004/diff/40001/tools-webrtc/ios/build_io... tools-webrtc/ios/build_ios_libs.py:70: # Add flavor option. I know you're just preserving the behavior of the old script here, but let's improve the new one by introducing a new flag instead of this hardcoded constant: parser.add_argument('--build_config', default='release', choices=['debug', 'release'], help='The build config. Can be "debug" or "release". ' 'Defaults to "release".') Then you can shorten this to something like gn_args.append('is_debug=%s' % 'true' if flavor == 'debug' else 'false') and skip the ValueError (I assume parse_args() deals with incorrect values?). One could argue all of the constants should be flags, but let's wait with that at least. https://codereview.webrtc.org/2662513004/diff/40001/tools-webrtc/ios/build_io... tools-webrtc/ios/build_ios_libs.py:78: # Add the specified architecture. I know these comments were there in the bash script, but I'd prefer just removing this one and the ones below (lines 78-92) as they don't really tell anything just reading the code doesn't reveal. https://codereview.webrtc.org/2662513004/diff/40001/tools-webrtc/ios/build_io... tools-webrtc/ios/build_ios_libs.py:108: subprocess.check_call(cmd) For debugging purposes, I find it useful to wrap subprocess.check_call into a helper function so it's convenient to log what it's executing. Can you do that? I suggest making something like _RunCommand which is similar to the _RunGN function at https://chromium.googlesource.com/external/webrtc/+/master/tools-webrtc/andro... (since we only invoke each of these tools once I think just a generic function is needed for this script). https://codereview.webrtc.org/2662513004/diff/40001/tools-webrtc/ios/build_io... tools-webrtc/ios/build_ios_libs.py:167: os.remove(os.path.join(args.output_dir, dylib_path)) Use shutil.rmtree instead and check with os.path.isdir first to avoid errors if it doesn't exist. https://codereview.webrtc.org/2662513004/diff/40001/tools-webrtc/ios/build_io... tools-webrtc/ios/build_ios_libs.py:178: # See https://codereview.chromium.org/2397433002. This was rolled in long time ago. Can you make a separate CL to remove it from the bash script first, so we don't make that change in this CL? (but we will of course remove it from here as well if that is not breaking anything). Send the review of that CL to me+tkchin, who added it in https://chromium.googlesource.com/external/webrtc/+/dd0e1e007092a6a87a3abf994... https://codereview.webrtc.org/2662513004/diff/40001/tools-webrtc/ios/build_io... tools-webrtc/ios/build_ios_libs.py:187: dsym_path = 'WebRTC.dSYM/Contents/Resources/DWARF/WebRTC' since we use os.path.join everywhere else, we should do that here too. https://codereview.webrtc.org/2662513004/diff/40001/tools-webrtc/ios/build_io... File tools-webrtc/ios/build_ios_libs.sh (left): https://codereview.webrtc.org/2662513004/diff/40001/tools-webrtc/ios/build_io... tools-webrtc/ios/build_ios_libs.sh:17: export PATH=/usr/libexec:$PATH We don't have anything making it possible to find /usr/libexec for our Python script. I don't think it's what's causing the errors you're seeing, but I think it breaks the script for some users (IIRC an external suggested we added that one).
On 2017/01/31 21:39:54, kjellander_webrtc wrote:
> Phew, I'm unable to spot what may be causing the problem.
> Maybe my suggestion for logging the check_call invocations will help.
Otherwise
> I hope you'll be able to figure it out when you got your Mac.
>
The issue here is that distutils.dir_util.copy_tree(...) differs from the 'cp -R
...' shell command in that it copies the source directory contents only, not the
source directory itself into the destination. To match that aspect, we need the
following two changes:
distutils.dir_util.copy_tree(
os.path.join(arm64_lib_path, SDK_FRAMEWORK_NAME),
os.path.join(args.output_dir,SDK_FRAMEWORK_NAME))
distutils.dir_util.copy_tree(os.path.join(arm64_lib_path, 'WebRTC.dSYM'),
os.path.join(args.output_dir,'WebRTC.dSYM'))
Also we need to wrap one more os.remove into try-catch:
try:
os.remove(os.path.join(args.output_dir, dsym_path))
except OSError:
pass
On 2017/02/03 04:33:59, VladimirTechMan wrote: > On 2017/01/31 21:39:54, kjellander_webrtc wrote: > > Phew, I'm unable to spot what may be causing the problem. > > Maybe my suggestion for logging the check_call invocations will help. > Otherwise > > I hope you'll be able to figure it out when you got your Mac. > > > > The issue here is that distutils.dir_util.copy_tree(...) differs from the 'cp -R > ...' shell command in that it copies the source directory contents only, not the > source directory itself into the destination. To match that aspect, we need the > following two changes: > > ... > > Also we need to wrap one more os.remove into try-catch: > ... And then, also, match the effect of the 'export PATH=/usr/libexec:$PATH' part in the original shell script, as it was already pointed above. After that, the "things just work". :-) At least, on my Mac they do now, trialing this new script.
On 2017/02/03 04:40:16, VladimirTechMan wrote: > On 2017/02/03 04:33:59, VladimirTechMan wrote: > > On 2017/01/31 21:39:54, kjellander_webrtc wrote: > > > Phew, I'm unable to spot what may be causing the problem. > > > Maybe my suggestion for logging the check_call invocations will help. > > Otherwise > > > I hope you'll be able to figure it out when you got your Mac. > > > > > > > The issue here is that distutils.dir_util.copy_tree(...) differs from the 'cp > -R > > ...' shell command in that it copies the source directory contents only, not > the > > source directory itself into the destination. To match that aspect, we need > the > > following two changes: > > > > ... > > > > Also we need to wrap one more os.remove into try-catch: > > ... > > And then, also, match the effect of the 'export PATH=/usr/libexec:$PATH' part in > the original shell script, as it was already pointed above. After that, the > "things just work". :-) At least, on my Mac they do now, trialing this new > script. This script seems to just move the bash calls to python instead? What's the value proposition here? Also arguably this script shouldn't really have to exist, it's largely only here because of previous gaps in GN/GYP (which may have already been fixed today). In an ideal world, we'd just pass parameters to GN and everything gets generated for us.
On 2017/02/03 06:29:14, tkchin_webrtc wrote: > On 2017/02/03 04:40:16, VladimirTechMan wrote: > > On 2017/02/03 04:33:59, VladimirTechMan wrote: > > > On 2017/01/31 21:39:54, kjellander_webrtc wrote: > > > > Phew, I'm unable to spot what may be causing the problem. > > > > Maybe my suggestion for logging the check_call invocations will help. > > > Otherwise > > > > I hope you'll be able to figure it out when you got your Mac. > > > > > > > > > > The issue here is that distutils.dir_util.copy_tree(...) differs from the > 'cp > > -R > > > ...' shell command in that it copies the source directory contents only, not > > the > > > source directory itself into the destination. To match that aspect, we need > > the > > > following two changes: > > > > > > ... > > > > > > Also we need to wrap one more os.remove into try-catch: > > > ... > > > > And then, also, match the effect of the 'export PATH=/usr/libexec:$PATH' part > in > > the original shell script, as it was already pointed above. After that, the > > "things just work". :-) At least, on my Mac they do now, trialing this new > > script. > > This script seems to just move the bash calls to python instead? What's the > value proposition here? Zeke: I initiated this task since 1) I dislike Bash scripts since they're hard to debug and test and inflexible. 2) It was a good, reasonably simple starting task for Oleh who recently joined. > Also arguably this script shouldn't really have to exist, it's largely only here > because of previous gaps in GN/GYP (which may have already been fixed today). > In an ideal world, we'd just pass parameters to GN and everything gets generated > for us. That's a valid point, however if we're going to move closer to integrating this into GN, we'll probably end up writing an action at some point, in which is also favorable to have the logic in Python (since that's what you invoke from a GN action, see https://codereview.webrtc.org/2673573003/ for an excellent example of that). So with that in mind this rewrite is also a benefit. Finally, the Android counterpart is already written in Python as well (https://chromium.googlesource.com/external/webrtc/+/master/tools-webrtc/andro...).
https://codereview.webrtc.org/2662513004/diff/40001/tools-webrtc/ios/build_io... File tools-webrtc/ios/build_ios_libs.py (right): https://codereview.webrtc.org/2662513004/diff/40001/tools-webrtc/ios/build_io... tools-webrtc/ios/build_ios_libs.py:60: shutil.rmtree(output_dir, True) On 2017/01/31 21:39:54, kjellander_webrtc wrote: > I'm not sure we want to ignore errors here. Since this is run only on Mac, we > shouldn't really get errors when deleting the dir, and if we do I think we might > be interested in seeing them as an exception. Done. https://codereview.webrtc.org/2662513004/diff/40001/tools-webrtc/ios/build_io... tools-webrtc/ios/build_ios_libs.py:65: custom_gn_options=[]): On 2017/01/31 21:39:53, kjellander_webrtc wrote: > empty lists as a default parameters in Python are scary, let's avoid it: > http://stackoverflow.com/questions/366422/what-is-the-pythonic-way-to-avoid-d... Done. https://codereview.webrtc.org/2662513004/diff/40001/tools-webrtc/ios/build_io... tools-webrtc/ios/build_ios_libs.py:70: # Add flavor option. On 2017/01/31 21:39:54, kjellander_webrtc wrote: > I know you're just preserving the behavior of the old script here, but let's > improve the new one by introducing a new flag instead of this hardcoded > constant: > > parser.add_argument('--build_config', default='release', > choices=['debug', 'release'], > help='The build config. Can be "debug" or "release". ' > 'Defaults to "release".') > > Then you can shorten this to something like > gn_args.append('is_debug=%s' % 'true' if flavor == 'debug' else 'false') > and skip the ValueError (I assume parse_args() deals with incorrect values?). > > One could argue all of the constants should be flags, but let's wait with that > at least. I made the change to use a flag but I feel like the check inside the function is still nice to have, so we don't have to rely on a check that is done outside of it and might not always apply. https://codereview.webrtc.org/2662513004/diff/40001/tools-webrtc/ios/build_io... tools-webrtc/ios/build_ios_libs.py:78: # Add the specified architecture. On 2017/01/31 21:39:53, kjellander_webrtc wrote: > I know these comments were there in the bash script, but I'd prefer just > removing this one and the ones below (lines 78-92) as they don't really tell > anything just reading the code doesn't reveal. Done. https://codereview.webrtc.org/2662513004/diff/40001/tools-webrtc/ios/build_io... tools-webrtc/ios/build_ios_libs.py:108: subprocess.check_call(cmd) On 2017/01/31 21:39:54, kjellander_webrtc wrote: > For debugging purposes, I find it useful to wrap subprocess.check_call into a > helper function so it's convenient to log what it's executing. Can you do that? > I suggest making something like _RunCommand which is similar to the _RunGN > function at > https://chromium.googlesource.com/external/webrtc/+/master/tools-webrtc/andro... > (since we only invoke each of these tools once I think just a generic function > is needed for this script). Done. https://codereview.webrtc.org/2662513004/diff/40001/tools-webrtc/ios/build_io... tools-webrtc/ios/build_ios_libs.py:167: os.remove(os.path.join(args.output_dir, dylib_path)) On 2017/01/31 21:39:54, kjellander_webrtc wrote: > Use shutil.rmtree instead and check with os.path.isdir first to avoid errors if > it doesn't exist. Not sure what you mean here. Seems like we are deleting a file, not a directory tree here. https://codereview.webrtc.org/2662513004/diff/40001/tools-webrtc/ios/build_io... tools-webrtc/ios/build_ios_libs.py:178: # See https://codereview.chromium.org/2397433002. On 2017/01/31 21:39:54, kjellander_webrtc wrote: > This was rolled in long time ago. Can you make a separate CL to remove it from > the bash script first, so we don't make that change in this CL? (but we will of > course remove it from here as well if that is not breaking anything). > > Send the review of that CL to me+tkchin, who added it in > https://chromium.googlesource.com/external/webrtc/+/dd0e1e007092a6a87a3abf994... https://codereview.webrtc.org/2676233002/ https://codereview.webrtc.org/2662513004/diff/40001/tools-webrtc/ios/build_io... tools-webrtc/ios/build_ios_libs.py:187: dsym_path = 'WebRTC.dSYM/Contents/Resources/DWARF/WebRTC' On 2017/01/31 21:39:53, kjellander_webrtc wrote: > since we use os.path.join everywhere else, we should do that here too. Done. https://codereview.webrtc.org/2662513004/diff/40001/tools-webrtc/ios/build_io... File tools-webrtc/ios/build_ios_libs.sh (left): https://codereview.webrtc.org/2662513004/diff/40001/tools-webrtc/ios/build_io... tools-webrtc/ios/build_ios_libs.sh:17: export PATH=/usr/libexec:$PATH On 2017/01/31 21:39:54, kjellander_webrtc wrote: > We don't have anything making it possible to find /usr/libexec for our Python > script. I don't think it's what's causing the errors you're seeing, but I think > it breaks the script for some users (IIRC an external suggested we added that > one). Done.
https://codereview.webrtc.org/2662513004/diff/40001/tools-webrtc/ios/build_io... File tools-webrtc/ios/build_ios_libs.py (right): https://codereview.webrtc.org/2662513004/diff/40001/tools-webrtc/ios/build_io... tools-webrtc/ios/build_ios_libs.py:70: # Add flavor option. On 2017/02/06 08:39:14, oprypin_webrtc wrote: > On 2017/01/31 21:39:54, kjellander_webrtc wrote: > > I know you're just preserving the behavior of the old script here, but let's > > improve the new one by introducing a new flag instead of this hardcoded > > constant: > > > > parser.add_argument('--build_config', default='release', > > choices=['debug', 'release'], > > help='The build config. Can be "debug" or "release". ' > > 'Defaults to "release".') > > > > Then you can shorten this to something like > > gn_args.append('is_debug=%s' % 'true' if flavor == 'debug' else 'false') > > and skip the ValueError (I assume parse_args() deals with incorrect values?). > > > > One could argue all of the constants should be flags, but let's wait with that > > at least. > > I made the change to use a flag but I feel like the check inside the function is > still nice to have, so we don't have to rely on a check that is done outside of > it and might not always apply. Fair enough. https://codereview.webrtc.org/2662513004/diff/40001/tools-webrtc/ios/build_io... tools-webrtc/ios/build_ios_libs.py:167: os.remove(os.path.join(args.output_dir, dylib_path)) On 2017/02/06 08:39:14, oprypin_webrtc wrote: > On 2017/01/31 21:39:54, kjellander_webrtc wrote: > > Use shutil.rmtree instead and check with os.path.isdir first to avoid errors > if > > it doesn't exist. > > Not sure what you mean here. Seems like we are deleting a file, not a directory > tree here. Ah, I thought it was a dir. Still nicer to check if the file is there and only try to delete it if so, instead of catching an exception and do nothing. https://codereview.webrtc.org/2662513004/diff/40001/tools-webrtc/ios/build_io... tools-webrtc/ios/build_ios_libs.py:178: # See https://codereview.chromium.org/2397433002. On 2017/02/06 08:39:14, oprypin_webrtc wrote: > On 2017/01/31 21:39:54, kjellander_webrtc wrote: > > This was rolled in long time ago. Can you make a separate CL to remove it from > > the bash script first, so we don't make that change in this CL? (but we will > of > > course remove it from here as well if that is not breaking anything). > > > > Send the review of that CL to me+tkchin, who added it in > > > https://chromium.googlesource.com/external/webrtc/+/dd0e1e007092a6a87a3abf994... > > https://codereview.webrtc.org/2676233002/ Acknowledged. https://codereview.webrtc.org/2662513004/diff/60001/tools-webrtc/ios/build_io... File tools-webrtc/ios/build_ios_libs.py (right): https://codereview.webrtc.org/2662513004/diff/60001/tools-webrtc/ios/build_io... tools-webrtc/ios/build_ios_libs.py:130: logging.basicConfig(level=logging.INFO) Since we have several debug logging statements now I think we should also add a --verbose or --debug flag to the script, so we can get them printed (I think it makes sense to have that enabled by default on the bots as an example).
https://codereview.webrtc.org/2662513004/diff/40001/tools-webrtc/ios/build_io... File tools-webrtc/ios/build_ios_libs.py (right): https://codereview.webrtc.org/2662513004/diff/40001/tools-webrtc/ios/build_io... tools-webrtc/ios/build_ios_libs.py:167: os.remove(os.path.join(args.output_dir, dylib_path)) On 2017/02/06 08:58:24, kjellander_webrtc wrote: > On 2017/02/06 08:39:14, oprypin_webrtc wrote: > > On 2017/01/31 21:39:54, kjellander_webrtc wrote: > > > Use shutil.rmtree instead and check with os.path.isdir first to avoid errors > > if > > > it doesn't exist. > > > > Not sure what you mean here. Seems like we are deleting a file, not a > directory > > tree here. > > Ah, I thought it was a dir. Still nicer to check if the file is there and only > try to delete it if so, instead of catching an exception and do nothing. Just applying the EAFP principle. Aside from how it looks, this prevents the hypothetical race condition where the file gets deleted elsewhere after the check, causing a "file not found" error when deleting it in the program. https://codereview.webrtc.org/2662513004/diff/60001/tools-webrtc/ios/build_io... File tools-webrtc/ios/build_ios_libs.py (right): https://codereview.webrtc.org/2662513004/diff/60001/tools-webrtc/ios/build_io... tools-webrtc/ios/build_ios_libs.py:130: logging.basicConfig(level=logging.INFO) On 2017/02/06 08:58:24, kjellander_webrtc wrote: > Since we have several debug logging statements now I think we should also add a > --verbose or --debug flag to the script, so we can get them printed (I think it > makes sense to have that enabled by default on the bots as an example). Done.
lgtm https://codereview.webrtc.org/2662513004/diff/40001/tools-webrtc/ios/build_io... File tools-webrtc/ios/build_ios_libs.py (right): https://codereview.webrtc.org/2662513004/diff/40001/tools-webrtc/ios/build_io... tools-webrtc/ios/build_ios_libs.py:167: os.remove(os.path.join(args.output_dir, dylib_path)) On 2017/02/06 09:19:50, oprypin_webrtc wrote: > On 2017/02/06 08:58:24, kjellander_webrtc wrote: > > On 2017/02/06 08:39:14, oprypin_webrtc wrote: > > > On 2017/01/31 21:39:54, kjellander_webrtc wrote: > > > > Use shutil.rmtree instead and check with os.path.isdir first to avoid > errors > > > if > > > > it doesn't exist. > > > > > > Not sure what you mean here. Seems like we are deleting a file, not a > > directory > > > tree here. > > > > Ah, I thought it was a dir. Still nicer to check if the file is there and only > > try to delete it if so, instead of catching an exception and do nothing. > > Just applying the EAFP principle. Aside from how it looks, this prevents the > hypothetical race condition where the file gets deleted elsewhere after the > check, causing a "file not found" error when deleting it in the program. You're right. This is fine.
The CQ bit was checked by oprypin@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...) ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...) ios_api_framework on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_api_framework/build...) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/22425)
The CQ bit was checked by oprypin@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by oprypin@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/2662513004/#ps120001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1486396260476990,
"parent_rev": "1134b7b9184bb2422a81dbdbeaf1f011ecffa184", "commit_rev":
"7a2d8ca9bc06f7cdc2cafeba02d57facd18614fc"}
Message was sent while issue was closed.
Description was changed from ========== Rewrite iOS FAT libraries build script in Python BUG=webrtc:7049 ========== to ========== Rewrite iOS FAT libraries build script in Python BUG=webrtc:7049 Review-Url: https://codereview.webrtc.org/2662513004 Cr-Commit-Position: refs/heads/master@{#16453} Committed: https://chromium.googlesource.com/external/webrtc/+/7a2d8ca9bc06f7cdc2cafeba0... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/external/webrtc/+/7a2d8ca9bc06f7cdc2cafeba0...
Message was sent while issue was closed.
Patchset #8 (id:140001) has been deleted |
