974900 - The Skia build is no longer unified as of bug 910754

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Seems like the UNIFIED_SOURCES moz.build was overwritten.

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

Yeah this was fallout from the new script we have to generate the moz.build. When I started on it I just didn't want to have to deal with problems due to unified build. We should try to fix it now, though.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

If we agree that build times matter at all, then this is really worth fixing!

Comment 3

[(no longer active)]

When I try to run this script, I get:

$ ./gyp_mozbuild
cp: cannot create regular file `trunk/third_party/externals/gyp/pylib/gyp/generator': No such file or directory
sh: 1: ./gyp_skia: not found
Failed to generate sources for linux
sh: 1: ./gyp_skia: not found
Failed to generate sources for mac
sh: 1: ./gyp_skia: not found
Failed to generate sources for android
sh: 1: ./gyp_skia: not found
Failed to generate sources for win
Traceback (most recent call last):
  File "./generate_mozbuild.py", line 306, in <module>
    main()
  File "./generate_mozbuild.py", line 302, in main
    write_mozbuild(includes, separated_sources)
  File "./generate_mozbuild.py", line 271, in write_mozbuild
    write_list(f, 'SOURCES', sources['mac'], 4)
KeyError: 'mac'

It seems like the gyp_skia script was not checked into the tree.

snorp, can you please take a look at this?

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #3)
> When I try to run this script, I get:
> 
> $ ./gyp_mozbuild
> cp: cannot create regular file
> `trunk/third_party/externals/gyp/pylib/gyp/generator': No such file or
> directory
> sh: 1: ./gyp_skia: not found
> Failed to generate sources for linux
> sh: 1: ./gyp_skia: not found
> Failed to generate sources for mac
> sh: 1: ./gyp_skia: not found
> Failed to generate sources for android
> sh: 1: ./gyp_skia: not found
> Failed to generate sources for win
> Traceback (most recent call last):
>   File "./generate_mozbuild.py", line 306, in <module>
>     main()
>   File "./generate_mozbuild.py", line 302, in main
>     write_mozbuild(includes, separated_sources)
>   File "./generate_mozbuild.py", line 271, in write_mozbuild
>     write_list(f, 'SOURCES', sources['mac'], 4)
> KeyError: 'mac'
> 
> It seems like the gyp_skia script was not checked into the tree.
> 
> snorp, can you please take a look at this?

You need to have a full Skia tree under trunk, not just the parts we use, in order to run the gyp_mozbuild script. It should be possible to just have a full tree in m-c now, I think, without too much trouble. George is there a reason we don't do that?

Comment 5

[George Wright (:gw280) (needinfo me!)]

Seems like it would be a huge waste to check in the entire Skia tree including buildsystem into mozilla-central. That's why I never did it. If we think it's worth it, then we can do it?

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to George Wright (:gw280) from comment #5)
> Seems like it would be a huge waste to check in the entire Skia tree
> including buildsystem into mozilla-central. That's why I never did it. If we
> think it's worth it, then we can do it?

I see some value. You can easily make a standalone build of Skia that way and run their unit tests, etc. This can be useful in order to nail down a bug. Also it lets people modify the moz.build, as in this case.

Comment 7

[George Wright (:gw280) (needinfo me!)]

OK. Let's import the entire Skia tree with the next update of Skia (slated for the week of March 18th which I believe is when the next train leaves).

Comment 8

[George Wright (:gw280) (needinfo me!)]

Detailed steps to run the script for now:

- Clone skia from https://code.google.com/p/skia/source/checkout
- Ensure you grab the same revision as is listed in README_MOZILLA in gfx/skia
- Copy that entire source tree to gfx/skia/trunk/
- Run gyp_mozbuild
- Profit

Comment 9

[(no longer active)]

Sorry if this is a stupid question, but here's what I get following those instructions:

$ ./gyp_mozbuild
cp: trunk/third_party/externals/gyp/pylib/gyp/generator: No such file or directory
Traceback (most recent call last):
  File "./gyp_skia", line 29, in <module>
    import gyp
ImportError: No module named gyp
Failed to generate sources for linux
Traceback (most recent call last):
  File "./gyp_skia", line 29, in <module>
    import gyp
ImportError: No module named gyp
Failed to generate sources for mac
Traceback (most recent call last):
  File "./gyp_skia", line 29, in <module>
    import gyp
ImportError: No module named gyp
Failed to generate sources for android
Traceback (most recent call last):
  File "./gyp_skia", line 29, in <module>
    import gyp
ImportError: No module named gyp
Failed to generate sources for win
Traceback (most recent call last):
  File "./generate_mozbuild.py", line 303, in <module>
    main()
  File "./generate_mozbuild.py", line 299, in main
    write_mozbuild(includes, separated_sources)
  File "./generate_mozbuild.py", line 268, in write_mozbuild
    write_list(f, 'SOURCES', sources['mac'], 4)
KeyError: 'mac'


The first line still suggests there is a missing file somewhere.  Also, where do I get the gyp module for python?  I tried cloning http://gyp.googlecode.com/svn/trunk/ but that repo doesn't have any insturctions on how to install this module.

Comment 10

[George Wright (:gw280) (needinfo me!)]

Checking out the Skia codebase should grab all that for you. Did you just do a straight up git clone or did you use gclient? I think the latter is necessary to grab the externals. 

https://sites.google.com/site/skiadocs/developer-documentation/contributing-code/downloading

Comment 11

[(no longer active)]

(In reply to comment #10)
> Checking out the Skia codebase should grab all that for you. Did you just do a
> straight up git clone or did you use gclient? I think the latter is necessary
> to grab the externals. 
> 
> https://sites.google.com/site/skiadocs/developer-documentation/contributing-code/downloading

I see, I just cloned skia directly because you said so in comment 8.  ;-)

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

Just to be sure - we want to automate the whole "update from upstream" Skia process and have it be done daily, without any human intervention.  If unified sources either help or hinder that, let's make sure we understand.

Comment 13

[George Wright (:gw280) (needinfo me!)]

Attachment: 0001-Unified-sources-for-Skia.patch

This unifies most of Skia. Most notably, it doesn't yet unify the sources for the gpu and pathops stuff, because there are a lot of build failures there. My opinion is that we should land this then create a new bug to track making gpu/pathops unifiable. This will require coordinating with upstream as the source will need to be modified.

Comment 14

[George Wright (:gw280) (needinfo me!)]

https://tbpl.mozilla.org/?tree=Try&rev=5e5527736a1d

Comment 15

[(no longer active)]

Comment on attachment 8389698 [details] [diff] [review]
0001-Unified-sources-for-Skia.patch

Review of attachment 8389698 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, thanks George!

Comment 16

[George Wright (:gw280) (needinfo me!)]

Updated try run: https://tbpl.mozilla.org/?tree=Try&rev=fac3e12d81fc

Comment 17

[Benoit Jacob [:bjacob] (mostly away)]

Any news here?

Comment 18

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Re-unify the skia build manually

Comment 19

[George Wright (:gw280) (needinfo me!)]

Comment on attachment 8435221 [details] [diff] [review]
Re-unify the skia build manually

Try run?

Comment 20

[Benoit Jacob [:bjacob] (mostly away)]

https://tbpl.mozilla.org/?tree=Try&rev=b48b0dc2bc25

Comment 21

[(no longer active)]

Comment on attachment 8435221 [details] [diff] [review]
Re-unify the skia build manually

Review of attachment 8435221 [details] [diff] [review]:
-----------------------------------------------------------------

Note that I don't know whether we want to upstream these changes, or whether we want to update the string, etc.  But as far as what goes into Gecko is concerned, this is good when it builds everywhere!

Comment 22

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Add missing-include-guards

Comment 23

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: #undef interface defined by windows headers

Comment 24

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Re-unify the skia build manually (r=ehsan)

Comment 25

[Benoit Jacob [:bjacob] (mostly away)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7b5f9225d255
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f0d95520c476
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/123b94215b3e

Comment 26

[Benoit Jacob [:bjacob] (mostly away)]

Green try run: https://tbpl.mozilla.org/?tree=Try&rev=108d53f92619

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7b5f9225d255
https://hg.mozilla.org/mozilla-central/rev/f0d95520c476
https://hg.mozilla.org/mozilla-central/rev/123b94215b3e

Comment 28

[George Wright (:gw280) (needinfo me!)]

Attachment: 0001-Unified-sources-for-Skia.patch

Benoit broke the Skia buildgenerator :(

Let's fix this properly.

Comment 29

[George Wright (:gw280) (needinfo me!)]

https://tbpl.mozilla.org/?tree=Try&rev=ec7dec9e0247

Comment 30

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8446338 [details] [diff] [review]
0001-Unified-sources-for-Skia.patch

Review of attachment 8446338 [details] [diff] [review]:
-----------------------------------------------------------------

nice

Comment 31

[George Wright (:gw280) (needinfo me!)]

Updated try run: https://tbpl.mozilla.org/?tree=Try&rev=a05763f854bd

Comment 32

[Benoit Jacob [:bjacob] (mostly away)]

\o/

Thanks for this! Skia is one of the largest directories with 23 unified files and unification saves about 3 minutes of CPU time to everyone making a clobber build, so this work is really useful.

Comment 33

[George Wright (:gw280) (needinfo me!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f2b276900cdf

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f2b276900cdf