Bug 210548 – [Flatpak SDK] Not fully hooked in BuildSlaveSupport

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Philippe Normand]

.

[Philippe Normand]

Created attachment 396536 [details]
Patch

[Carlos Alberto Lopez Perez]

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

[Philippe Normand]

Ahhh that's you meant about zip... Yeah, this won't work...

[Carlos Alberto Lopez Perez]

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

[Philippe Normand]

Created attachment 396545 [details]
Patch

[Philippe Normand]

Created attachment 396547 [details]
Patch

[Philippe Normand]

Created attachment 396548 [details]
Patch

[Carlos Alberto Lopez Perez]

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

[Carlos Alberto Lopez Perez]

(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.

[Philippe Normand]

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.

[Carlos Alberto Lopez Perez]

(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.

[Philippe Normand]

Created attachment 396633 [details]
Patch

[Philippe Normand]

Created attachment 396648 [details]
Patch

[Carlos Alberto Lopez Perez]

Comment on attachment 396648 [details]
Patch

nice! thanks :)

[EWS]

Committed r260240: <https://trac.webkit.org/changeset/260240>

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

[Radar WebKit Bug Importer]

<rdar://problem/61925692>