Bug 227579 - Bundle libraries for remote execution in run-jsc-benchmarks
Summary: Bundle libraries for remote execution in run-jsc-benchmarks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Angelos Oikonomopoulos
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-07-01 04:13 PDT by Angelos Oikonomopoulos
Modified: 2021-07-16 07:25 PDT (History)
11 users (show)

See Also:


Attachments
Patch (29.43 KB, patch)
2021-07-01 04:17 PDT, Angelos Oikonomopoulos
no flags Details | Formatted Diff | Diff
Patch (32.12 KB, patch)
2021-07-02 04:22 PDT, Angelos Oikonomopoulos
no flags Details | Formatted Diff | Diff
Patch (32.13 KB, patch)
2021-07-02 07:29 PDT, Angelos Oikonomopoulos
no flags Details | Formatted Diff | Diff
Patch (32.71 KB, patch)
2021-07-12 07:57 PDT, Angelos Oikonomopoulos
no flags Details | Formatted Diff | Diff
Patch (36.65 KB, patch)
2021-07-12 08:06 PDT, Angelos Oikonomopoulos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Angelos Oikonomopoulos 2021-07-01 04:13:55 PDT
Bundle libraries for remote execution in run-jsc-benchmarks
Comment 1 Angelos Oikonomopoulos 2021-07-01 04:17:46 PDT
Created attachment 432676 [details]
Patch
Comment 2 Angelos Oikonomopoulos 2021-07-02 04:22:06 PDT
Created attachment 432779 [details]
Patch
Comment 3 Angelos Oikonomopoulos 2021-07-02 07:29:46 PDT
Created attachment 432792 [details]
Patch
Comment 4 Radar WebKit Bug Importer 2021-07-08 04:14:17 PDT
<rdar://problem/80316541>
Comment 5 Carlos Alberto Lopez Perez 2021-07-08 14:19:55 PDT
Comment on attachment 432792 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=432792&action=review

The code looks fine, however when I tested it I found unexpected errors.. that is why I'm setting r-

It seems that generating the MiniBrowser bundle with generate-bundle is broken after this patch

$ Tools/Scripts/generate-bundle --platform wpe --release --bundle MiniBrowser 
Traceback (most recent call last):
  File "Tools/Scripts/generate-bundle", line 606, in <module>
    sys.exit(main())
  File "Tools/Scripts/generate-bundle", line 596, in main
    bundle_file_path = bundle_creator.create()
  File "Tools/Scripts/generate-bundle", line 254, in create
    self._create_bundle(bundle_binary)
  File "Tools/Scripts/generate-bundle", line 388, in _create_bundle
    gio_modules = self._get_gio_modules()
  File "Tools/Scripts/generate-bundle", line 327, in _get_gio_modules
    retcode, stdout, stderr = self._run_cmd_and_get_output(['pkg-config', '--variable=giomoduledir', 'gio-2.0'])
AttributeError: 'BundleCreator' object has no attribute '_run_cmd_and_get_output'

> Tools/Scripts/bundle-binary:1
> +#!/usr/bin/env python3

Please add the usual copyright boilerplate at the top of each new code file, is the policy of the project to do that.

> Tools/Scripts/webkitpy/binary_bundling/bundle.py:1
> +import logging

Missing copyright boilerplate also here

> Tools/Scripts/webkitpy/binary_bundling/ldd.py:1
> +import logging

Also missing copyright
Comment 6 Angelos Oikonomopoulos 2021-07-12 07:57:26 PDT
Created attachment 433317 [details]
Patch
Comment 7 Angelos Oikonomopoulos 2021-07-12 08:06:59 PDT
Created attachment 433319 [details]
Patch
Comment 8 Angelos Oikonomopoulos 2021-07-13 05:25:01 PDT
(In reply to Carlos Alberto Lopez Perez from comment #5)
> Comment on attachment 432792 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=432792&action=review
> 
> The code looks fine, however when I tested it I found unexpected errors..
> that is why I'm setting r-
> 
> It seems that generating the MiniBrowser bundle with generate-bundle is
> broken after this patch
> 
> $ Tools/Scripts/generate-bundle --platform wpe --release --bundle
> MiniBrowser 
> Traceback (most recent call last):
>   File "Tools/Scripts/generate-bundle", line 606, in <module>
>     sys.exit(main())
>   File "Tools/Scripts/generate-bundle", line 596, in main
>     bundle_file_path = bundle_creator.create()
>   File "Tools/Scripts/generate-bundle", line 254, in create
>     self._create_bundle(bundle_binary)
>   File "Tools/Scripts/generate-bundle", line 388, in _create_bundle
>     gio_modules = self._get_gio_modules()
>   File "Tools/Scripts/generate-bundle", line 327, in _get_gio_modules
>     retcode, stdout, stderr = self._run_cmd_and_get_output(['pkg-config',
> '--variable=giomoduledir', 'gio-2.0'])
> AttributeError: 'BundleCreator' object has no attribute
> '_run_cmd_and_get_output'

Oops. Did some more testing of the bundling code after fixing this. AFAICT the only difference that diff -Nur finds is the INTERPRETER= line being moved earlier in the generated script. Tested with the flatpak build with/without generate-install-script and with the jhbuild.

> > Tools/Scripts/bundle-binary:1
> > +#!/usr/bin/env python3
> 
> Please add the usual copyright boilerplate at the top of each new code file,
> is the policy of the project to do that.

Thanks, should be there now.
Comment 9 Carlos Alberto Lopez Perez 2021-07-13 14:33:20 PDT
Comment on attachment 433319 [details]
Patch

Patch looks fine! thanks!
BTW, a suggestion to make the bundle-binary script jhbuild/flatpak aware is this diff over your patch: http://sprunge.us/BB3T7i
It adds an optional platform parameter, that when enabled will try to detect jhbuild/flatpak detection on gtk/wpe ports.
So it looks for libraries inside the flatpak container or the jhbuild directories.
If platform is not passed (like when called from the run-jsc-benchmark script then it behaves like now.. doesn't try to detect jhbuild/flatpak which is not used on javascriptonly builds)
Comment 10 Angelos Oikonomopoulos 2021-07-16 07:20:59 PDT
(In reply to Carlos Alberto Lopez Perez from comment #9)
> Comment on attachment 433319 [details]
> Patch
> 
> Patch looks fine! thanks!
> BTW, a suggestion to make the bundle-binary script jhbuild/flatpak aware is
> this diff over your patch: http://sprunge.us/BB3T7i
> It adds an optional platform parameter, that when enabled will try to detect
> jhbuild/flatpak detection on gtk/wpe ports.
> So it looks for libraries inside the flatpak container or the jhbuild
> directories.
> If platform is not passed (like when called from the run-jsc-benchmark
> script then it behaves like now.. doesn't try to detect jhbuild/flatpak
> which is not used on javascriptonly builds)

Thanks for the suggestion! ATM (in this patch), `bundle-binary` is only used non-interactively from run-jsc-stress-tests and run-jsc-benchmarks. IIUC, it will work for e.g. a flatpak setup as long as it's run from within the flatpak container (as for most other scripts). So not sure that explicitly adding flatpak/jhbuild support is needed at this point. But open to adding such if there's some interactive use I'm unaware of.
Comment 11 EWS 2021-07-16 07:25:33 PDT
Committed r279984 (239727@main): <https://commits.webkit.org/239727@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 433319 [details].