Bug 210548

Summary: [Flatpak SDK] Not fully hooked in BuildSlaveSupport
Product: WebKit Reporter: Philippe Normand <pnormand>
Component: Tools / TestsAssignee: Philippe Normand <pnormand>
Status: RESOLVED FIXED    
Severity: Normal CC: aakash_jain, clopez, ews-watchlist, jbedard, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Philippe Normand 2020-04-15 07:57:57 PDT
.
Comment 1 Philippe Normand 2020-04-15 08:07:39 PDT
Created attachment 396536 [details]
Patch
Comment 2 Carlos Alberto Lopez Perez 2020-04-15 08:30:35 PDT
Comment on attachment 396536 [details]
Patch

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

> Tools/BuildSlaveSupport/built-product-archive:295
> +    if sys.platform.startswith("linux"):
> +        top_level_directory = os.path.normpath(os.path.join(os.path.dirname(__file__), '..'))
> +        sys.path.insert(0, os.path.join(top_level_directory, 'flatpak'))
> +        sys.path.append(os.path.join(top_level_directory, 'Scripts'))
> +        import flatpakutils
> +        flatpakutils.run_in_sandbox_if_available(sys.argv)

I think this its not going to work for our use case.
Because the bots have a modified PATH and instead of the real "zip" command they run a wrapper script that does some magic to instead of uploading the real build-products only uploading a fake zip with a README with a pointer to the built-product on a local server (so they avoid uploading/downloading hundreds of MBs on each build to internet).
And if we make this script run inside the flatpak environment, then it will use the real "zip" command instead of the zipwrapper script.
Let me test this, but i suspect it won't work
Comment 3 Philippe Normand 2020-04-15 08:40:06 PDT
Ahhh that's you meant about zip... Yeah, this won't work...
Comment 4 Carlos Alberto Lopez Perez 2020-04-15 09:20:11 PDT
Comment on attachment 396536 [details]
Patch

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

>> Tools/BuildSlaveSupport/built-product-archive:295
>> +        flatpakutils.run_in_sandbox_if_available(sys.argv)
> 
> I think this its not going to work for our use case.
> Because the bots have a modified PATH and instead of the real "zip" command they run a wrapper script that does some magic to instead of uploading the real build-products only uploading a fake zip with a README with a pointer to the built-product on a local server (so they avoid uploading/downloading hundreds of MBs on each build to internet).
> And if we make this script run inside the flatpak environment, then it will use the real "zip" command instead of the zipwrapper script.
> Let me test this, but i suspect it won't work

Tested it, it doesn't work.

Instead of running this script inside flatpak I think we need to change the path it uses for the build-directory from WebKitBuild/$Configuration to WebKitBuild/$PORT/$Configuration

See determineWebKitBuildDirectories() in Tools/BuildSlaveSupport/built-product-archive

And only do that when the third-party userFlatpak directory its present and WEBKIT_JHBUILD env variable its not defined
Comment 5 Philippe Normand 2020-04-15 10:03:11 PDT
Created attachment 396545 [details]
Patch
Comment 6 Philippe Normand 2020-04-15 10:26:39 PDT
Created attachment 396547 [details]
Patch
Comment 7 Philippe Normand 2020-04-15 10:27:50 PDT
Created attachment 396548 [details]
Patch
Comment 8 Carlos Alberto Lopez Perez 2020-04-15 11:22:25 PDT
Comment on attachment 396548 [details]
Patch

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

> Tools/BuildSlaveSupport/built-product-archive:124
> +        prefixDirectory = os.path.join(archiveDir, platform.upper(), configuration.title())
> +        if os.environ.get("WEBKIT_JHBUILD", "0") == "0" and os.path.isdir(prefixDirectory):

This won't work on the first run for the test bot with flatpak because WebKitBuild/$PORT/$Config doesn't exist yet.
Remember that the test bot doesn't build, it just runs the update-flatpak step and then downloads the built product and runs the tests.
We have to check here for the userflatpak directory, not for the build one.

> Tools/BuildSlaveSupport/built-product-archive:253
> -        if createZipFromList(neededDirectories, configuration, excludePattern='*.o'):
> +        if createZipFromList(neededDirectories, platform, configuration, excludePattern='*.o'):

I don't think patching the createZipFromList() function its the best approach.
Instead of that I think we need to fix the function webkitBuildDirectoryForConfigurationAndPlatform() to output the right directories.
This its also specially important because we need to fix also the directory for the extract command. See:

This command should create a zip file from the build directory (its what the build-only bot runs)
python Tools/BuildSlaveSupport/built-product-archive --platform=gtk --release archive


And this other should _wipe_ the build directory and then uncompress the contents of the previous zip file there (its what the test-only bot runs after downloading the zip in a previous step)
python Tools/BuildSlaveSupport/built-product-archive --platform=gtk --release extract
Comment 9 Carlos Alberto Lopez Perez 2020-04-15 11:25:09 PDT
(In reply to Carlos Alberto Lopez Perez from comment #8)
> Instead of that I think we need to fix the function
> webkitBuildDirectoryForConfigurationAndPlatform() to output the right
> directories.


I see also that this webkitBuildDirectoryForConfigurationAndPlatform() function its getting that directories by simply calling other (perl! yay!) script (webkit-build-directory) to get the paths (toplevel and configuration)

$ Tools/Scripts/webkit-build-directory --gtk --release
/home/clopez/webkit/webkit/WebKitBuild
/home/clopez/webkit/webkit/WebKitBuild/Release

And with a few greps I also see that this script webkit-build-directory its used on more scripts, for example the one to order a clean build (clean-build)

Maybe a better approach to fix this starts with patching this webkit-build-directory perl script to output the right build directory for when flatpak its in use.
Comment 10 Philippe Normand 2020-04-15 11:29:15 PDT
Of course I thought about fixing webkit-build-directory but then the result depends on wether we run in the sandbox or not and it becomes yet another mess.
Comment 11 Carlos Alberto Lopez Perez 2020-04-15 11:39:40 PDT
(In reply to Philippe Normand from comment #10)
> Of course I thought about fixing webkit-build-directory but then the result
> depends on wether we run in the sandbox or not and it becomes yet another
> mess.

Checking if we are inside of the sandbox or not seems easy to me, for example you can just check if the directory /app/webkit exists or not.
Comment 12 Philippe Normand 2020-04-16 04:41:22 PDT
Created attachment 396633 [details]
Patch
Comment 13 Philippe Normand 2020-04-16 07:44:04 PDT
Created attachment 396648 [details]
Patch
Comment 14 Carlos Alberto Lopez Perez 2020-04-16 13:21:53 PDT
Comment on attachment 396648 [details]
Patch

nice! thanks :)
Comment 15 EWS 2020-04-17 01:37:42 PDT
Committed r260240: <https://trac.webkit.org/changeset/260240>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396648 [details].
Comment 16 Radar WebKit Bug Importer 2020-04-17 01:38:17 PDT
<rdar://problem/61925692>