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

Remove duplicated fonts and put them under /fonts/ #9718

Merged
merged 4 commits into from Mar 7, 2018

Conversation

mrego
Copy link
Member

@mrego mrego commented Feb 28, 2018

This patch removes the duplicated fonts we have in the repository
by copying them under /fonts/ and removing the duplicates.
The paths are updated accordingly to point to the new location.

In addition this patch moves the content of /css/fonts/ under /fonts/,
so there is a single common "fonts" folder on the repository.

@gsnedders
Copy link
Member

gsnedders commented Feb 28, 2018

I think I'd rather have each font in its own directory rather than putting lots directly in /fonts (so then we can have adjacent README files with something about the font, which most of them could do with!); not expecting you to document all of them /now/ though.

@gsnedders gsnedders closed this Feb 28, 2018
@gsnedders gsnedders reopened this Feb 28, 2018
@gsnedders
Copy link
Member

(I fat-fingered the wrong button there)

@w3c-bots
Copy link

w3c-bots commented Feb 28, 2018

Build ERRORED

Started: 2018-03-07 11:57:49
Finished: 2018-03-07 12:13:35

Failing Jobs

  • firefox:nightly
  • chrome:dev
  • safari:11.0
  • MicrosoftEdge:16.16299

View more information about this build on:

@gsnedders
Copy link
Member

(The build failing is unsurprising given number of tests modified.)

@mrego
Copy link
Member Author

mrego commented Mar 1, 2018

@gsnedders what could be the structure of /fonts/ folder then, would it be something like this fine?

├── ad
│   └── AD.woff
├── adobe-fonts
│   ├── CSSFWOrientationTest.otf
│   ├── CSSHWOrientationTest.otf
│   ├── LICENSE
│   └── README.md
├── ahem-extra
│   ├── AHEM_*.TTF
├── Ahem.ttf
├── canvas-test
│   ├── CanvasTest.ttf
│   └── CanvasTest.ttf.sub.headers
├── CSSTest
│   ├── csstest-*.ttf
│   ├── LICENSE
│   └── README
├── gentiumplus-r
│   └── GentiumPlus-R.woff
├── math
│   ├── *.woff
├── mplus-1p-regular
│   └── mplus-1p-regular.woff
├── noto
│   ├── NotoSansAdlam-hinted
│   │   ├── LICENSE_OFL.txt
│   │   ├── NotoSansAdlam-Regular.ttf
│   │   └── README
│   ├── NotoSansCypriot-hinted
│   │   ├── LICENSE_OFL.txt
│   │   ├── NotoSansCypriot-Regular.ttf
│   │   └── README
│   └── NotoSansDeseret-Regular.ttf
├── OWNERS
├── README.md
├── revalia
│   └── Revalia.woff
├── sileot-webfont
│   └── sileot-webfont.woff
└── tcu-font
    └── tcu-font.woff

I'm asking before doing the actual changes on the patch, just in case you'd prefer something different.
Thanks.

@jgraham
Copy link
Contributor

jgraham commented Mar 1, 2018

I don't really care how this is organised within /fonts but one subdirectory per font doesn't seem like an obvious default. I understand doing it for cases like Noto where we might group lots of similar fonts together.

@wpt-pr-bot wpt-pr-bot requested a review from r12a March 7, 2018 11:52
@gsnedders
Copy link
Member

Okay, I've moved CanvasTest back out of a directory (because it's a solo font) and reverted the changes that point to it. This should fix the update_built_tests check failing.

Copy link
Member

@gsnedders gsnedders left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

waiting for Travis to finish…

@gsnedders gsnedders merged commit e45d0cf into web-platform-tests:master Mar 7, 2018
gsnedders added a commit to gsnedders/web-platform-tests that referenced this pull request Mar 7, 2018
My fixup broke these and I didn't notice, le sigh.
@plinss
Copy link
Contributor

plinss commented Mar 9, 2018

Having all the fonts being referred to using absolute paths is breaking the css test infrastructure as the URLs are no longer within the test built test suite...

@fantasai
Copy link
Contributor

fantasai commented Mar 9, 2018

Please revert this change. This just broke way too much stuff. CSSWG test infrastructure is broken, running local tests while one is working is broken, and it's entirely likely that other systems pulling these tests are broken. There's a reason we use relative URLs. Maybe there's a better way, but at least if you're making a change don't break everything in the process.

@frivoal
Copy link
Contributor

frivoal commented Mar 9, 2018

Also, the merge has invalidated all the manual testing results stored in testharness, since the files have changed :(

That doesn't mean we should never do any kind of sweeping change, but since it does cause data loss, we shouldn't do such changes lightly

mrego added a commit to mrego/wpt that referenced this pull request Mar 9, 2018
@mrego
Copy link
Member Author

mrego commented Mar 9, 2018

Here is a PR to revert this until for now: #9940

I'm sorry for this being breaking the CSS infrastructure, but I'm not sure I understand the reasons.
Using relative paths has also issues with the CSS infrastructure if they are like ../foo/bar.baz,
however using absolute paths has been working fine for /resources/testharness.js
why that cannot be possible for /fonts/?

And about the manual testing results, I think that this kind of changes will only be needed once, so we'd need to assume that I guess we have no other choice.
And I guess we all agree that avoiding duplicated files in the repo is a good goal.

And my apologizes again for breaking this but it's not always easy to know what's going on in the CSS build system. 😞

@r12a
Copy link
Contributor

r12a commented Mar 9, 2018

Where does this leave us in terms of lodging copies of the CSS fonts under /fonts so that people can use them in that location in future? I'm hoping that that's still ok, since i recently changed several hundred tests (only a few submitted so far) to look in /fonts.

Note that I already started that process with #9818 (and i'm relying on that location for the 500 test PR at #9871).

@r12a r12a mentioned this pull request Mar 9, 2018
@mrego
Copy link
Member Author

mrego commented Mar 9, 2018

I don't really know what we should do here, so I'll let other people with more experience to decide.

BTW, I did this PR after this comment:
#9689 (comment)
Note that there are currently a bunch of duplicated fonts in WPT, so it seemed a good idea to get rid of the duplicates.

@foolip
Copy link
Member

foolip commented Mar 9, 2018

@foolip
Copy link
Member

foolip commented Mar 9, 2018

So, I'm ready to revert #9940 but am waiting for confirmation, so that we don't flip back and forth and create more trouble.

@plinss
Copy link
Contributor

plinss commented Mar 9, 2018

I don’t believe this needs to be reverted.

There’s a secondary problem that when tests are changed, the results posted to the older version no longer apply, and a lot of manual testing can be lost unnecessarily for changes like these. The CSS test harness has a mechanism where different versions of a test can be declared to equivalent (formerly used to allow certain meta data changes), this could be leveraged to reinstate the old results, but will take some work.

@foolip
Copy link
Member

foolip commented Mar 10, 2018

@Hexcles and I were talking about what it would take to avoid the need for urgent fixing of this sort going forward, and lints that enforce the constraints seem like the obvious way. @plinss, what's a good place to learn more about how the CSS test harness works? Are there other constraints apart from required spec links and what directories can be included from?

@mrego
Copy link
Member Author

mrego commented Mar 12, 2018

I'd be interested in knowing more about the CSS build system to check which are the requirements.
In my mind it seems it'd be nice to extend https://wpt.fyi to eventually replace https://test.csswg.org

gsnedders added a commit that referenced this pull request Mar 15, 2018
My fixup broke these and I didn't notice, le sigh.
Hexcles added a commit that referenced this pull request Sep 21, 2018
Following #9718, the font is now in /fonts/ instead of /fonts/CSSTest/.
Globally fix the path using sed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants