New issue
Advanced search Search tips

Issue 697015 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android
Pri: 3
Type: Bug



Sign in to add a comment

Unify freetype2 and freetype-android to one checkout

Project Member Reported by drott@chromium.org, Feb 28 2017

Issue description

We have third_party/freetype2 as well as third_party/freetype-android. We should unify those, which has become possible after the freetype upgrade to 2.7.1 for Linux content shell.


 

Comment 1 by drott@chromium.org, Feb 28 2017

Components: Blink>Fonts

Comment 2 by drott@chromium.org, Feb 28 2017

Cc: js...@chromium.org
Project Member

Comment 3 by bugdroid1@chromium.org, Mar 1 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4db6eb187710a109f98606588f4fa53b501e15ba

commit 4db6eb187710a109f98606588f4fa53b501e15ba
Author: drott <drott@chromium.org>
Date: Wed Mar 01 12:30:06 2017

Unify freetype-android and freetype2 into one checkout

Removing freetype2 for now, later renaming freetype-android to something
more generic.

Moving freetype-android from source_set to a component target so that
it's generally usable in the other configurations. This should not
change much on Android where component resolves to static_library.

Also, adding myself to freetype-android OWNERS.

TBR'ing jochen for the removal, as presubmit requests an OWNER.

BUG= 697015 
TBR=jochen

Review-Url: https://codereview.chromium.org/2720823003
Cr-Commit-Position: refs/heads/master@{#453918}

[modify] https://crrev.com/4db6eb187710a109f98606588f4fa53b501e15ba/DEPS
[modify] https://crrev.com/4db6eb187710a109f98606588f4fa53b501e15ba/content/shell/BUILD.gn
[modify] https://crrev.com/4db6eb187710a109f98606588f4fa53b501e15ba/third_party/WebKit/LayoutTests/platform/linux/fast/text/emoji-web-font-expected.png
[modify] https://crrev.com/4db6eb187710a109f98606588f4fa53b501e15ba/third_party/WebKit/LayoutTests/platform/linux/fast/text/emoji-web-font-expected.txt
[rename] https://crrev.com/4db6eb187710a109f98606588f4fa53b501e15ba/third_party/freetype-android/.clang-format
[modify] https://crrev.com/4db6eb187710a109f98606588f4fa53b501e15ba/third_party/freetype-android/BUILD.gn
[modify] https://crrev.com/4db6eb187710a109f98606588f4fa53b501e15ba/third_party/freetype-android/OWNERS
[modify] https://crrev.com/4db6eb187710a109f98606588f4fa53b501e15ba/third_party/freetype-android/include/freetype-android-config/ftoption.h
[delete] https://crrev.com/84b4215d6c4fa677341ba96dfdfa083a27345b74/third_party/freetype2/BUILD.gn
[delete] https://crrev.com/84b4215d6c4fa677341ba96dfdfa083a27345b74/third_party/freetype2/OWNERS
[delete] https://crrev.com/84b4215d6c4fa677341ba96dfdfa083a27345b74/third_party/freetype2/README.chromium
[delete] https://crrev.com/84b4215d6c4fa677341ba96dfdfa083a27345b74/third_party/freetype2/include/freetype-custom-config/ftconfig.h
[delete] https://crrev.com/84b4215d6c4fa677341ba96dfdfa083a27345b74/third_party/freetype2/include/freetype-custom-config/ftmodule.h
[delete] https://crrev.com/84b4215d6c4fa677341ba96dfdfa083a27345b74/third_party/freetype2/include/freetype-custom-config/ftoption.h
[delete] https://crrev.com/84b4215d6c4fa677341ba96dfdfa083a27345b74/third_party/freetype2/patches/freetype2_symbols_visibility.patch

Comment 4 by drott@chromium.org, Mar 1 2017

Status: Fixed (was: Started)

Comment 5 by boliu@chromium.org, Mar 1 2017

I think this broke android component build (there's no cq coverage for it, and I use it locally)

I'm getting a runtime error for not being able to load libfreetype.s.so:

ContentShellActivity: Caused by: java.lang.UnsatisfiedLinkError: dalvik.system.PathClassLoader[DexPathList[[zip file "/system/framework/android.test.runner.jar", zip file "/data/app/org.chromium.content_shell_apk.tests-1/base.apk", zip file "/data/app/org.chromium.content_shell_apk-1/base.apk"],nativeLibraryDirectories=[/data/app/
org.chromium.content_shell_apk.tests-1/lib/arm, /data/app/org.chromium.content_shell_apk-1/lib/arm, /data/app/org.chromium.content_shell_apk.tests-1/base.apk!/lib/armeabi-v7a, /data/app/org.chromium.content_shell_apk-1/base.apk!/lib/armeabi-v7a, /vendor/lib, /system/lib]]] couldn't find "libfreetype.s.so"

Comment 6 by boliu@chromium.org, Mar 1 2017

Cc: agrieve@chromium.org
+agrieve to help with component build things, gn is still beyond me..
It broke my non-component Android build, as well.

Comment 8 by boliu@chromium.org, Mar 1 2017

> It broke my non-component Android build, as well.

is_debug=true implies component build, so it was a component build
I went and built a clean component Android build and note that the FreeType library is built as libfreetype.so.6, it appears this should be libfreetype.cr.so. Probably something to do with the .cr.so handling...
the library is built, but not being put into the apk

If you touch one of the freetype source files, and print out the stuff that ninja does in the incremental build, all it does is rebuild the .o and .so, but doesn't rebuild the apk. This sort of thing has happened before, but I don't remember how this all works :/
boliu@, could you please try with this experimental fix https://codereview.chromium.org/2728463003 ?
We need the library name to be libfreetype.so.6 on linux, so that it's found by the linker and shadows the system freetype when we build content_shell (which links against it), so I put the name in the BUILD.gn file explicitly. Doing this seems to conflict with the Android component build. The speculative fix only explicitly sets the name when is_linux.
With the patch from https://codereview.chromium.org/2728463003 my component Android build now works.
Great, thanks for checking, then let's merge this CL.

Project Member

Comment 15 by bugdroid1@chromium.org, Mar 1 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1da0b00909796cae61b947e76830fdfa67d6802c

commit 1da0b00909796cae61b947e76830fdfa67d6802c
Author: twellington <twellington@chromium.org>
Date: Wed Mar 01 21:12:40 2017

Revert of Unify freetype-android and freetype2 into one checkout (patchset #2 id:20001 of https://codereview.chromium.org/2720823003/ )

Reason for revert:
This broke component builds on Android.

Original issue's description:
> Unify freetype-android and freetype2 into one checkout
>
> Removing freetype2 for now, later renaming freetype-android to something
> more generic.
>
> Moving freetype-android from source_set to a component target so that
> it's generally usable in the other configurations. This should not
> change much on Android where component resolves to static_library.
>
> Also, adding myself to freetype-android OWNERS.
>
> TBR'ing jochen for the removal, as presubmit requests an OWNER.
>
> BUG= 697015 
> TBR=jochen
>
> Review-Url: https://codereview.chromium.org/2720823003
> Cr-Commit-Position: refs/heads/master@{#453918}
> Committed: https://chromium.googlesource.com/chromium/src/+/4db6eb187710a109f98606588f4fa53b501e15ba

TBR=behdad@chromium.org,bungeman@chromium.org,dpranke@chromium.org,eae@chromium.org,jshin@chromium.org,drott@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 697015 

Review-Url: https://codereview.chromium.org/2729703002
Cr-Commit-Position: refs/heads/master@{#454028}

[modify] https://crrev.com/1da0b00909796cae61b947e76830fdfa67d6802c/DEPS
[modify] https://crrev.com/1da0b00909796cae61b947e76830fdfa67d6802c/content/shell/BUILD.gn
[modify] https://crrev.com/1da0b00909796cae61b947e76830fdfa67d6802c/third_party/WebKit/LayoutTests/platform/linux/fast/text/emoji-web-font-expected.png
[modify] https://crrev.com/1da0b00909796cae61b947e76830fdfa67d6802c/third_party/WebKit/LayoutTests/platform/linux/fast/text/emoji-web-font-expected.txt
[modify] https://crrev.com/1da0b00909796cae61b947e76830fdfa67d6802c/third_party/freetype-android/BUILD.gn
[modify] https://crrev.com/1da0b00909796cae61b947e76830fdfa67d6802c/third_party/freetype-android/OWNERS
[modify] https://crrev.com/1da0b00909796cae61b947e76830fdfa67d6802c/third_party/freetype-android/include/freetype-android-config/ftoption.h
[rename] https://crrev.com/1da0b00909796cae61b947e76830fdfa67d6802c/third_party/freetype2/.clang-format
[add] https://crrev.com/1da0b00909796cae61b947e76830fdfa67d6802c/third_party/freetype2/BUILD.gn
[add] https://crrev.com/1da0b00909796cae61b947e76830fdfa67d6802c/third_party/freetype2/OWNERS
[add] https://crrev.com/1da0b00909796cae61b947e76830fdfa67d6802c/third_party/freetype2/README.chromium
[add] https://crrev.com/1da0b00909796cae61b947e76830fdfa67d6802c/third_party/freetype2/include/freetype-custom-config/ftconfig.h
[add] https://crrev.com/1da0b00909796cae61b947e76830fdfa67d6802c/third_party/freetype2/include/freetype-custom-config/ftmodule.h
[add] https://crrev.com/1da0b00909796cae61b947e76830fdfa67d6802c/third_party/freetype2/include/freetype-custom-config/ftoption.h
[add] https://crrev.com/1da0b00909796cae61b947e76830fdfa67d6802c/third_party/freetype2/patches/freetype2_symbols_visibility.patch

I verified CL in #11 fixes this as well. Thanks!
Project Member

Comment 17 by bugdroid1@chromium.org, Mar 1 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6b4e0da088089a5c4346c8e7546f106d4e5cbbbe

commit 6b4e0da088089a5c4346c8e7546f106d4e5cbbbe
Author: drott <drott@chromium.org>
Date: Wed Mar 01 22:03:47 2017

Reland of Unify freetype-android and freetype2 into one checkout (patchset #1 id:1 of https://codereview.chromium.org/2729703002/ )

Reason for revert:
Fix ready and reviewed https://codereview.chromium.org/2728463003

Original issue's description:
> Revert of Unify freetype-android and freetype2 into one checkout (patchset #2 id:20001 of https://codereview.chromium.org/2720823003/ )
>
> Reason for revert:
> This broke component builds on Android.
>
> Original issue's description:
> > Unify freetype-android and freetype2 into one checkout
> >
> > Removing freetype2 for now, later renaming freetype-android to something
> > more generic.
> >
> > Moving freetype-android from source_set to a component target so that
> > it's generally usable in the other configurations. This should not
> > change much on Android where component resolves to static_library.
> >
> > Also, adding myself to freetype-android OWNERS.
> >
> > TBR'ing jochen for the removal, as presubmit requests an OWNER.
> >
> > BUG= 697015 
> > TBR=jochen
> >
> > Review-Url: https://codereview.chromium.org/2720823003
> > Cr-Commit-Position: refs/heads/master@{#453918}
> > Committed: https://chromium.googlesource.com/chromium/src/+/4db6eb187710a109f98606588f4fa53b501e15ba
>
> TBR=behdad@chromium.org,bungeman@chromium.org,dpranke@chromium.org,eae@chromium.org,jshin@chromium.org,drott@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG= 697015 
>
> Review-Url: https://codereview.chromium.org/2729703002
> Cr-Commit-Position: refs/heads/master@{#454028}
> Committed: https://chromium.googlesource.com/chromium/src/+/1da0b00909796cae61b947e76830fdfa67d6802c

TBR=behdad@chromium.org,bungeman@chromium.org,dpranke@chromium.org,eae@chromium.org,jshin@chromium.org,twellington@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 697015 

Review-Url: https://codereview.chromium.org/2730493002
Cr-Commit-Position: refs/heads/master@{#454056}

[modify] https://crrev.com/6b4e0da088089a5c4346c8e7546f106d4e5cbbbe/DEPS
[modify] https://crrev.com/6b4e0da088089a5c4346c8e7546f106d4e5cbbbe/content/shell/BUILD.gn
[modify] https://crrev.com/6b4e0da088089a5c4346c8e7546f106d4e5cbbbe/third_party/WebKit/LayoutTests/platform/linux/fast/text/emoji-web-font-expected.png
[modify] https://crrev.com/6b4e0da088089a5c4346c8e7546f106d4e5cbbbe/third_party/WebKit/LayoutTests/platform/linux/fast/text/emoji-web-font-expected.txt
[rename] https://crrev.com/6b4e0da088089a5c4346c8e7546f106d4e5cbbbe/third_party/freetype-android/.clang-format
[modify] https://crrev.com/6b4e0da088089a5c4346c8e7546f106d4e5cbbbe/third_party/freetype-android/BUILD.gn
[modify] https://crrev.com/6b4e0da088089a5c4346c8e7546f106d4e5cbbbe/third_party/freetype-android/OWNERS
[modify] https://crrev.com/6b4e0da088089a5c4346c8e7546f106d4e5cbbbe/third_party/freetype-android/include/freetype-android-config/ftoption.h
[delete] https://crrev.com/cb16a69fc771ddc62b00da78161c2b55c4385b3a/third_party/freetype2/BUILD.gn
[delete] https://crrev.com/cb16a69fc771ddc62b00da78161c2b55c4385b3a/third_party/freetype2/OWNERS
[delete] https://crrev.com/cb16a69fc771ddc62b00da78161c2b55c4385b3a/third_party/freetype2/README.chromium
[delete] https://crrev.com/cb16a69fc771ddc62b00da78161c2b55c4385b3a/third_party/freetype2/include/freetype-custom-config/ftconfig.h
[delete] https://crrev.com/cb16a69fc771ddc62b00da78161c2b55c4385b3a/third_party/freetype2/include/freetype-custom-config/ftmodule.h
[delete] https://crrev.com/cb16a69fc771ddc62b00da78161c2b55c4385b3a/third_party/freetype2/include/freetype-custom-config/ftoption.h
[delete] https://crrev.com/cb16a69fc771ddc62b00da78161c2b55c4385b3a/third_party/freetype2/patches/freetype2_symbols_visibility.patch

Project Member

Comment 18 by bugdroid1@chromium.org, Mar 1 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/be5b593038f45afaf9f39272b5d2c97fad02c766

commit be5b593038f45afaf9f39272b5d2c97fad02c766
Author: drott <drott@chromium.org>
Date: Wed Mar 01 22:12:59 2017

Fix for Android component build after FreeType merge

BUG= 697015 

Review-Url: https://codereview.chromium.org/2728463003
Cr-Commit-Position: refs/heads/master@{#454062}

[modify] https://crrev.com/be5b593038f45afaf9f39272b5d2c97fad02c766/third_party/freetype-android/BUILD.gn

Alright, thanks everyone for the quick responses. The Android component build issue should be resolved now.
Project Member

Comment 20 by bugdroid1@chromium.org, Mar 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fe896a85ac3ab250cc5cad0e857da7ef32283a11

commit fe896a85ac3ab250cc5cad0e857da7ef32283a11
Author: drott <drott@chromium.org>
Date: Mon Mar 06 12:12:54 2017

Rename third_party/freetype-android to third_party/freetype

Now that the FreeType checkouts are unified, we should no longer use
the _android suffix.

TBR'ing jochen for the rename, not adding any new library.

BUG= 697015 
TBR=jochen

Review-Url: https://codereview.chromium.org/2726873002
Cr-Commit-Position: refs/heads/master@{#454855}

[modify] https://crrev.com/fe896a85ac3ab250cc5cad0e857da7ef32283a11/DEPS
[modify] https://crrev.com/fe896a85ac3ab250cc5cad0e857da7ef32283a11/build/linux/BUILD.gn
[modify] https://crrev.com/fe896a85ac3ab250cc5cad0e857da7ef32283a11/content/shell/BUILD.gn
[modify] https://crrev.com/fe896a85ac3ab250cc5cad0e857da7ef32283a11/skia/BUILD.gn
[rename] https://crrev.com/fe896a85ac3ab250cc5cad0e857da7ef32283a11/third_party/freetype/.clang-format
[rename] https://crrev.com/fe896a85ac3ab250cc5cad0e857da7ef32283a11/third_party/freetype/BUILD.gn
[rename] https://crrev.com/fe896a85ac3ab250cc5cad0e857da7ef32283a11/third_party/freetype/OWNERS
[rename] https://crrev.com/fe896a85ac3ab250cc5cad0e857da7ef32283a11/third_party/freetype/README.chromium
[rename] https://crrev.com/fe896a85ac3ab250cc5cad0e857da7ef32283a11/third_party/freetype/include/freetype-custom-config/ftmodule.h
[rename] https://crrev.com/fe896a85ac3ab250cc5cad0e857da7ef32283a11/third_party/freetype/include/freetype-custom-config/ftoption.h

Project Member

Comment 21 by bugdroid1@chromium.org, Mar 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/84d67b491f426b51a8d19411d313738d0aa9e240

commit 84d67b491f426b51a8d19411d313738d0aa9e240
Author: dalecurtis <dalecurtis@chromium.org>
Date: Mon Mar 06 20:31:18 2017

Fixup stale freetype .gitignore entries.

Without this the tree is marked as dirty.

BUG= 697015 

Review-Url: https://codereview.chromium.org/2737613002
Cr-Commit-Position: refs/heads/master@{#454942}

[modify] https://crrev.com/84d67b491f426b51a8d19411d313738d0aa9e240/third_party/.gitignore

Ummmm so my repo got screwed up today, and I've traced it back to my .git/config file which -- somehow -- now lists the URL of my "origin" remote as "https://chromium.googlesource.com/chromium/src/third_party/freetype2.git".

Could that have somehow been triggered by one of these CLs?

I'm not sure when today this happened. I had done roughly the following steps:

1. git fetch origin master && git checkout origin/master
2. gclient sync
3. gn gen $OUTDIR && ninja -C $OUTDIR chrome
4. git status # suggested removing third_party/freetype2 and third_party/freetype-android
5. rm -rf third_party/freetype2 third_party/freetype-android
6. gclient sync

and now `git remote get-url origin` returns the freetype2.git repo. How is that even possible?
michaelpg@, are you sure git status suggested the removal, or was it gclient? Or how did git suggest it? By listing a lot of untracked files?

third_party/freetype/src/.git/ exists, for the DEPS based checkout of FreeType, which correctly lists 
$ git remote get-url origin
https://chromium.googlesource.com/chromium/src/third_party/freetype2.git
when I execute this in third_party/freetype/src/

Could it be that the .git directories got confused somehow? From where were you calling the 'git remote get-url origin' command? 

Comment 24 by drott@chromium.org, Mar 21 2017

Re #22, please see details in  issue 703343 . The issue is, that with old checkouts had an old folder third_party/freetype with a .git folder in it, which needs to be cleaned up.

Sign in to add a comment