Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Use prebuilt skia static link to reduce build time #57

Closed
wants to merge 1 commit into from

Conversation

Brooooooklyn
Copy link
Member

@Brooooooklyn Brooooooklyn commented Mar 20, 2019

Don't know why I can build and run in my Macbook pro, but all failure in CI.
Do you have any idea? @pragmatrix

@pragmatrix
Copy link
Member

@Brooooooklyn These linker errors on the CI seem to occur for C++ methods that are defined in the header files. Looking at the build script in the Skia repository, I was missing the flag that prevents inlining, so I think you need to generate the prebuilt libs with extra_cflags=["-fno-inline-functions"].

I am not sure why it works on your notebook though.

@Brooooooklyn
Copy link
Member Author

Windows build still failed... I couldn't find reason

@pragmatrix
Copy link
Member

@Brooooooklyn Regarding Windows: Add a RUST_BACKTRACE=1 like suggested to show the failing line in build.rs, I assume that neither wget nor/or tar is available on the Windows build VM. To make build.rs independent of such external tools, we should probably use rust libraries instead.

Several notes about this PR:

  • We need to extend build.rs to support the following:
    • The possibility to pull in prebuilt binaries if the platform, compiler, and feature configuration matches (this may be even more nuanced, see below).
    • The possibility to build Skia if the prebuilt binaries are not available. This is required if we want to support feature dependent builds, or platform specific builds we don't have prebuilt binaries for. Also I would prefer to keep the actual Skia repository as a submodule reference in the rust-skia repository.
    • The possibility to deploy pre-built binaries (this is for us, and a logical consequence of the two above, if we don't want to duplicate code).

Regarding the prebuilt-binaries, I think we need to compute a key string or a hash over such a string for each platform to match & locate them. This key should include:

  • The version of the rust compiler (+beta / +nightly) (because of skiabinding).
  • The complete platform string.
  • The current feature configuration.
  • The repository hash of rust-skia.

So effectively I am asking for a merge of the repository and the possibilities you use to build the Skia binaries with this one.

For a first step, we could use this PR as it is, but instead of panicing when the prebuilt binaries are not available, check out and do a full Skia build. Then, we can try to integrate the code you built to deploy the prebuilt binaries with the extended matching requirements.

@Brooooooklyn Brooooooklyn force-pushed the feature/prebuilt-skia branch 3 times, most recently from 483ffa3 to ccb7bd2 Compare March 25, 2019 11:19
@pragmatrix pragmatrix changed the title Use prebuilt skia static link to reduce build time [WIP] Use prebuilt skia static link to reduce build time Apr 6, 2019
@pragmatrix
Copy link
Member

Renamed this PR as Work In Progress.

@Brooooooklyn
Copy link
Member Author

Maybe we can create a new pkg named rust-skia-build.
In default case, we publish the prebuilt skia static link through azurepipeline, the users will download the prebuilt static link files and use it directly.
If the system of user's machine we didn't covered in prebuilt pipeline or prebuilt files downloaded fail, or the user want modify the default build arguments, they can write there own build.rs like below:

use rust_skia_build::{Arg, build};

fn main() {
  let arg = Arg {
    gn_args: "xxxx",
    download_third_parts: true,
    ....
  };

  build(arg).unwrap();
}

In the build function, we can do things like what we are doing in our build.rs now.

@pragmatrix
Copy link
Member

pragmatrix commented Apr 14, 2019

@Brooooooklyn If you don't mind, I'd like to give an integration a shot. After all, you have done the heavy lifting already. But I will likely have to delay working on that for about two weeks because of two other projects and a planned vacation.

In default case, we publish the prebuilt skia static link through azurepipeline, the users will download the prebuilt static link files and use it directly.

Yes, as I can see from your azure script, it's possible to use a different repository for that. So what I would like is to produce the releases for every successful build on the master branch here and upload it to the Releases section to an empty repository.

If the system of user's machine we didn't covered in prebuilt pipeline or prebuilt files downloaded fail, or the user want modify the default build arguments, they can write there own build.rs like below.

No, for a first attempt I would really like to always fall back to the full build. Additional customization options for Skia could be provided through an environment variable. I hope this can be aligned with your plans, if not, please state why.

Then, when we generate a Cargo release package, we don't supply the Skia submodule, but will check it out and build it when the available downloads do not match Key of the Cargo release.

Technical Details (a bit more revised compared to my post before):

  • To make our builds in this repository faster, and generate packages for the downloadable crate, we combine

    • the Skia's submodule hash
    • the TARGET environment variable
    • the Skia configuration

    ... into a Key for naming the Skia package.

  • The generated bindings use the same Key combined with a hash of skia-bindings/src/bindings.cpp and perhaps the preprocessor symbols used to generate the bindings.rs file for the SkiaBindings library with the result that we may need to publish more binding packages than Skia packages. This is needed, so we can freely adjust bindings.cpp alongside the same Skia version.

We will encounter situations for which a Key matches, but the downloaded Skia or SkiaBindings library will not be able to be used, for example, because of system library dependency that has changed. We will fix them as they come in by integrating additional versions / hashes to the packages' Keys.

Going over the packages in https://github.com/rust-skia/skia/releases, I have a question:

Why are skia-include.tgz and skia-third-party.tgz needed? I think that it's probably best to pre-built bindings.rs and the bindings native library, too, and if so, do we need package the Skia includes at all?

@Brooooooklyn
Copy link
Member Author

Sorry for the delay to reply your comment, I've been busy these days.

If you don't mind, I'd like to give an integration a shot.

Yes, I don't mind.

for a first attempt I would really like to always fall back to the full build

I agree with this point.

Additional customization options for Skia could be provided through an environment variable

I am afraid control the skia build by pass some enviroment variables is not flexiable enough. What I mean in the code snippet below, the build function will do the full skia build like download skia repo, third part tools, and call gn & ninja to build them, just what we are doing in the build.rs which in skia-bindings package.

use rust_skia_build::{Arg, build};

fn main() {
  let arg = Arg {
    gn_args: "xxxx",
    download_third_parts: true,
    ....
  };

  build(arg).unwrap();
}

What I proposal is encapsulating the build.rs in skia-bindings as a standable package, which users could call it in their own build.rs while they want change the skia build arguments, or we can use it in skia-bindings to do fallback build when then prebuilt files cannot be downloaded.

About prebuld the bindings.rs, I've tried to provide a prebuild bindings.rs, but failed in CI. And I'm not sure if the bindings.rs is always the same in diffrent machines. So there are still skia-include.tgz and skia-third-party.tgz in release.

@pragmatrix
Copy link
Member

pragmatrix commented Apr 19, 2019

What I proposal is encapsulating the build.rs in skia-bindings as a standable package

Ok, I agree, this is a good thing to do. We'll try to provide a separate rust library for the build support.

And I'm not sure if the bindings.rs is always the same in diffrent machines.

It is different.

It's also different because of the the system headers used in the build, which means the compiler, the cpu architecture and even the operating system's version may all affect the build result.

But so is the Skia library.

Which means that if we provide a prebuilt Skia library we must also provide the matching bindings.rs and skiabindings library alongside with it. If users build bindings.rs themselves but pull in a prebuilt Skia library from our build servers, they would not match and cause linker errors or worse, spurious crashes.

@pragmatrix pragmatrix mentioned this pull request May 3, 2019
8 tasks
@pragmatrix
Copy link
Member

Closing this in favor of #97, but I'd like to keep the branch around for reference until #97 is merged.

@pragmatrix pragmatrix closed this May 4, 2019
@pragmatrix pragmatrix deleted the feature/prebuilt-skia branch June 29, 2019 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants