Closed Bug 974900 Opened 6 years ago Closed 6 years ago

The Skia build is no longer unified as of bug 910754

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: bjacob, Assigned: bjacob)

References

Details

Attachments

(4 files, 2 obsolete files)

Seems like the UNIFIED_SOURCES moz.build was overwritten.
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.
Blocks: unified
If we agree that build times matter at all, then this is really worth fixing!
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?
Flags: needinfo?(snorp)
(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?
Flags: needinfo?(snorp) → needinfo?(gwright)
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?
Flags: needinfo?(gwright)
(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.
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).
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
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.
Flags: needinfo?(gwright)
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
Flags: needinfo?(gwright)
(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.  ;-)
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.
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.
Attachment #8389698 - Flags: review?(ehsan)
Comment on attachment 8389698 [details] [diff] [review]
0001-Unified-sources-for-Skia.patch

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

Looks great, thanks George!
Attachment #8389698 - Flags: review?(ehsan) → review+
Any news here?
Attached patch Re-unify the skia build manually (obsolete) — Splinter Review
Attachment #8435221 - Flags: review?(ehsan)
Comment on attachment 8435221 [details] [diff] [review]
Re-unify the skia build manually

Try run?
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!
Attachment #8435221 - Flags: review?(ehsan) → review+
Attachment #8435991 - Flags: review?(gwright)
Attachment #8435221 - Attachment is obsolete: true
Attachment #8435993 - Flags: review+
Attachment #8435991 - Flags: review?(gwright) → review+
Attachment #8435992 - Flags: review?(gwright) → review+
Benoit broke the Skia buildgenerator :(

Let's fix this properly.
Attachment #8389698 - Attachment is obsolete: true
Attachment #8446338 - Flags: review?(snorp)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8446338 [details] [diff] [review]
0001-Unified-sources-for-Skia.patch

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

nice
Attachment #8446338 - Flags: review?(snorp) → review+
\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.
https://hg.mozilla.org/mozilla-central/rev/f2b276900cdf
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.