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

Revert "Remove duplicated fonts and put them under /fonts/ (#9718)" #9940

Closed
wants to merge 1 commit into from

Conversation

@mrego
Copy link
Member

mrego commented Mar 9, 2018

This reverts commit e45d0cf as it broke CSS test infrastructure.

@wpt-pr-bot wpt-pr-bot requested review from nattokirai, nox, plinss and r12a Mar 9, 2018

@Ms2ger
Copy link
Contributor

Ms2ger left a comment

I suggest those interested in maintaining the CSSWG test infrastructure fix that instead.

@frivoal

This comment has been minimized.

Copy link
Contributor

frivoal commented Mar 9, 2018

I suggest those interested in maintaining the CSSWG test infrastructure fix that instead.

This is not OK. The CSSWG test infrastructure may be quirky / not to your taste, but it works, and we're sharing this repo. Breaking it and letting other people deal with the fallout is not OK.

Reverting a single commit is much cheaper than requiring people to rewrite software that has worked for years.

Maybe we should replace the CSSWG test infrastructure. Maybe we should modernize it. But we haven't done that yet, so for now, we need this commit reverted.

CC: @plinss @gsnedders @fantasai

@r12a

r12a approved these changes Mar 9, 2018

@plinss

This comment has been minimized.

Copy link
Contributor

plinss commented Mar 9, 2018

At this point I've added an alias to serve the wpt fonts directory from the root of test.csswg.org (in the same manner as we serve /resources), so this revert isn't necessary to get the built test suites and test harness working again.

This may be a reasonable long-term fix for this particular issue, but it needs some more consideration. We also need a better path forward for coordinating and testing these kinds of changes in the future.

@wpt-pr-bot wpt-pr-bot requested a review from svgeesus Mar 9, 2018

@mrego

This comment has been minimized.

Copy link
Member Author

mrego commented Mar 9, 2018

Ok, let's close this then.

I'm sorry for breaking this, but I don't know what's the way to test it properly and know that you are not going to break anything.
Is there any way to do that in order to avoid this kind of issues in the future?

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Mar 9, 2018

Reverting when things are broken by accident is pretty much the default, see these in the past 6 months:

I don't know exactly how the CSS build system is used, by given how fast @plinss noticed it sure seems like it's an important part of the workflow. Even though it touches lots of files, I believe this revert should have been done ASAP. It's already been imported into Blink with no trouble, but large changes tend to be more trouble, so spreading them out over time is no fun.

@plinss, is this revert still required to get everything back into shape? I would like to merge the revert, and @jgraham said on IRC "I don't mind reverting this", so I think that's what we should do.

We should revert this ASAP if still required to unblock the CSSWG, then work out how to reland.

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Mar 9, 2018

Travis is failing, but there's no chance that Travis could pass, so we should ignore that too if we merge this. @plinss, will revert if I don't hear back before the end of day.

@mrego

This comment has been minimized.

Copy link
Member Author

mrego commented Mar 9, 2018

@foolip check the last comment from @plinss

At this point I've added an alias to serve the wpt fonts directory from the root of test.csswg.org (in the same manner as we serve /resources), so this revert isn't necessary to get the built test suites and test harness working again.

If I get it right this revert is no longer needed, so we can close this PR. Do you agree?

BTW, I also said I didn't have problems with the revert and I'm sorry for creating all this mess.

@plinss

This comment has been minimized.

Copy link
Contributor

plinss commented Mar 9, 2018

As far as I’m aware the current work-around is working, so no need to revert.

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Mar 9, 2018

@mrego, yep, I saw it, but the "needs some more consideration" made we wonder if it's a speculative fix and not confirmed to be enough.

So, crisis averted, closing this, and just filed #9953 for "better path forward for coordinating and testing these kinds of changes in the future."

@foolip foolip closed this Mar 9, 2018

@plinss

This comment has been minimized.

Copy link
Contributor

plinss commented Mar 10, 2018

@foolip, as far as I know the current fix is enough to get things working again. The "needs some more consideration" just means I need to think if this is the best way to keep things working long-term (and maybe we need a generic system for paths to common files so we don't have to add these kinds of aliases over and over).

@gsnedders

This comment has been minimized.

Copy link
Contributor

gsnedders commented Mar 12, 2018

@plinss I think we should probably just make it possible to use all the top-level shared resource directories (common, fonts, images, media, resources); I think all of these have existed since WPT has, so there's not much risk of new ones getting added.

@plinss

This comment has been minimized.

Copy link
Contributor

plinss commented Mar 12, 2018

If it's just that set of 5 then fine, I can add the others as aliases to the csswg test server. All I had in mind was something like if new shared-resource directories get added that happens under /common (or let's create /shared and put everything, or all new things under that). That way new ones can be added without having to coordinate changes or worry about breakage.

@gsnedders

This comment has been minimized.

Copy link
Contributor

gsnedders commented Mar 12, 2018

@plinss given the most recent of them dates from 2013 in the repo, and web-platform-tests only really happened in 2014, I'd say the risk of adding any more is super low. :)

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Mar 13, 2018

@gsnedders would a lint that checks that css/ only includes from those 5 top-level directories be expensive, you think?

@gsnedders

This comment has been minimized.

Copy link
Contributor

gsnedders commented Mar 13, 2018

@foolip it would add a lot of extra complexity versus what we have now (we'd have to extract URLs to check, which especially in css/ would mean finding some CSS parser to do so), and obviously it couldn't be complete (finding out what URLs JS might load obviously reduces to the halting problem).

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Mar 13, 2018

@gsnedders, so there's nothing currently that checks anything about the included resources? Or just for scripts to catch broken testharness.js includes?

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Mar 13, 2018

I realize that doing it for fonts would be quite challenging, we'd need a CSS parser for that.

@gsnedders

This comment has been minimized.

Copy link
Contributor

gsnedders commented Mar 13, 2018

@foolip We literally just do <script> and check that if it contains testharness.js it's the path we expect, and <link rel=match> has an href that points to something that exists. I think historically the majority of broken URLs in the built output of the CSS testsuite have been in CSS and not HTML, because that's where resources are mostly loaded (given they almost all use inline stylesheets).

@plinss

This comment has been minimized.

Copy link
Contributor

plinss commented Mar 13, 2018

The css build system also scans the entire wpt repo for tests that link to css specs, not just the /css directory, so a lint restricted to that directory wouldn't help

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Mar 13, 2018

OK, I suppose a lint for this isn't really practical at this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.