When importing w3c repos, convert CRLF line endings.

When importing w3c repos, convert CRLF line endings.

Background:

The Chromium presubmit checks for Windows-style line endings in some text
files. For pan-project checks, //third_party/WebKit is excluded, but for
some other checks including the line endings check and permissions check,
it is not excluded.

(See https://cs.chromium.org/chromium/src/PRESUBMIT.py?l=2047)

In principle, I think we don't care a lot if automatically imported files
that we don't have control over don't don't meet the Chromium presubmit,
so it would be acceptable to ignore presubmits.

Earlier, in https://chromium-review.googlesource.com/c/419148/, I made
it so that when uploading changes with presubmit warnings the warnings
are ignored, but the warnings aren't ignored on commit because the
chromium_presubmit try job runs and fails on warnings.

Another option would be to change //PRESUBMIT.py to add the imported
wpt directory to the blacklist for particular checks.

The rationale for making this change here is that:
 1. We're already doing some transformations to pass presubmit.
 2. I didn't see any other checks that are likely to be an issue.
 3. Transforming CRLF -> LF line ending is a pretty simple transformation.

BUG=671683

[qyearsley, 2016-12-21 19:13:51]

qyearsley@chromium.org changed reviewers:
+ jeffcarp@chromium.org, rbyers@chromium.org

[qyearsley, 2016-12-21 19:13:51]

Hi, this is a proposed change to get csswg-test imports going again.

Note, there's an alternative change that would also work: changing the top-level
presubmit. But, adding one exception to one check in the top-level presubmit
seems a little messier than adding another little transformation when importing.

[qyearsley, 2016-12-21 19:54:18]

The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-21 19:54:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-21 21:27:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-21 21:27:29]

Dry run: This issue passed the CQ dry run.

[jeffcarp, 2016-12-21 23:12:02]

On 2016/12/21 at 19:13:51, qyearsley wrote:
> Hi, this is a proposed change to get csswg-test imports going again.
> 
> Note, there's an alternative change that would also work: changing the
top-level presubmit. But, adding one exception to one check in the top-level
presubmit seems a little messier than adding another little transformation when
importing.

lgtm. I don't have a ton of context on this but it seems ok for now. In the
longer term would it be feasible, if the list of files with CRLF in WPT is
small, to submit PRs to WPT to remove them?

[qyearsley, 2016-12-21 23:18:28]

On 2016/12/21 at 23:12:02, jeffcarp wrote:
> On 2016/12/21 at 19:13:51, qyearsley wrote:
> > Hi, this is a proposed change to get csswg-test imports going again.
> > 
> > Note, there's an alternative change that would also work: changing the
top-level presubmit. But, adding one exception to one check in the top-level
presubmit seems a little messier than adding another little transformation when
importing.
> 
> lgtm. I don't have a ton of context on this but it seems ok for now. In the
longer term would it be feasible, if the list of files with CRLF in WPT is
small, to submit PRs to WPT to remove them?

Yeah - The list of files is small. I made a PR a while ago when I first noticed
this, but I don't think it ever got merged:
https://github.com/w3c/csswg-test/pull/1157
So that's still pending.

Looking at that again now, I'm not certain that that PR is correct.

But yeah, ideally I think we want consistent line endings in web-platform-tests
too, although while there's no explicit check for that repo, it's always
possible for people to submit new changes with CRLF line endings, which would
break us in the future.

[Rick Byers, 2016-12-21 23:23:58]

Description was changed from

==========
When importing w3c repos, convert CRLF line endings.

Background:

The Chromium presubmit checks for Windows-style line endings in some text
files. For pan-project checks, //third_party/WebKit is excluded, but for
some other checks including the line endings check and permissions check,
it is not excluded.

(See https://cs.chromium.org/chromium/src/PRESUBMIT.py?l=2047)

In principle, I think we don't care a lot if automatically imported files
that we don't have control over don't don't meet the Chromium presubmit,
so it would be acceptable to ignore presubmits.

Earlier, in https://chromium-review.googlesource.com/c/419148/, I made
it so that when uploading changes with presubmit warnings the warnings
are ignored, but the warnings aren't ignored on commit because the
chromium_presubmit try job runs and fails on warnings.

Another option would be to change //PRESUBMIT.py to add the imported
wpt directory to the blacklist for particular checks.

The rationale for making this change here is that:
 1. We're already doing some transformations to pass presubmit.
 2. I didn't see any other checks that are likely to be an issue.
 3. Transforming CRLF -> LF line ending is a pretty simple transformation.

BUG=671683
==========

to

==========
When importing w3c repos, convert CRLF line endings.

Background:

The Chromium presubmit checks for Windows-style line endings in some text
files. For pan-project checks, //third_party/WebKit is excluded, but for
some other checks including the line endings check and permissions check,
it is not excluded.

(See https://cs.chromium.org/chromium/src/PRESUBMIT.py?l=2047)

In principle, I think we don't care a lot if automatically imported files
that we don't have control over don't don't meet the Chromium presubmit,
so it would be acceptable to ignore presubmits.

Earlier, in https://chromium-review.googlesource.com/c/419148/, I made
it so that when uploading changes with presubmit warnings the warnings
are ignored, but the warnings aren't ignored on commit because the
chromium_presubmit try job runs and fails on warnings.

Another option would be to change //PRESUBMIT.py to add the imported
wpt directory to the blacklist for particular checks.

The rationale for making this change here is that:
 1. We're already doing some transformations to pass presubmit.
 2. I didn't see any other checks that are likely to be an issue.
 3. Transforming CRLF -> LF line ending is a pretty simple transformation.

BUG=671683
==========

[Rick Byers, 2016-12-21 23:25:39]

On 2016/12/21 23:18:28, qyearsley wrote:
> On 2016/12/21 at 23:12:02, jeffcarp wrote:
> > On 2016/12/21 at 19:13:51, qyearsley wrote:
> > > Hi, this is a proposed change to get csswg-test imports going again.
> > > 
> > > Note, there's an alternative change that would also work: changing the
> top-level presubmit. But, adding one exception to one check in the top-level
> presubmit seems a little messier than adding another little transformation
when
> importing.
> > 
> > lgtm. I don't have a ton of context on this but it seems ok for now. In the
> longer term would it be feasible, if the list of files with CRLF in WPT is
> small, to submit PRs to WPT to remove them?
> 
> Yeah - The list of files is small. I made a PR a while ago when I first
noticed
> this, but I don't think it ever got merged:
> https://github.com/w3c/csswg-test/pull/1157
> So that's still pending.
> 
> Looking at that again now, I'm not certain that that PR is correct.
> 
> But yeah, ideally I think we want consistent line endings in
web-platform-tests
> too, although while there's no explicit check for that repo, it's always
> possible for people to submit new changes with CRLF line endings, which would
> break us in the future.

Doesn't doing any sort of transformation like this pose a problem for the
auto-exporter?  We don't want a little change to look like it touched every line
due to a change in whitespace.

If it is a problem then perhaps we can get the upstream repo's configured to
disallow CRLF endings at commit time.  Usually I think that's from a
misconfigured git client on Windows.  +gsnedders who's probably the authority to
ask here.

That said if we can safely hack around this without causing export issues, then
that works for me too.

[qyearsley, 2016-12-21 23:35:21]

On 2016/12/21 at 23:25:39, rbyers wrote:
> On 2016/12/21 23:18:28, qyearsley wrote:
> > On 2016/12/21 at 23:12:02, jeffcarp wrote:
> > > On 2016/12/21 at 19:13:51, qyearsley wrote:
> > > > Hi, this is a proposed change to get csswg-test imports going again.
> > > > 
> > > > Note, there's an alternative change that would also work: changing the
> > top-level presubmit. But, adding one exception to one check in the top-level
> > presubmit seems a little messier than adding another little transformation
when
> > importing.
> > > 
> > > lgtm. I don't have a ton of context on this but it seems ok for now. In
the
> > longer term would it be feasible, if the list of files with CRLF in WPT is
> > small, to submit PRs to WPT to remove them?
> > 
> > Yeah - The list of files is small. I made a PR a while ago when I first
noticed
> > this, but I don't think it ever got merged:
> > https://github.com/w3c/csswg-test/pull/1157
> > So that's still pending.
> > 
> > Looking at that again now, I'm not certain that that PR is correct.
> > 
> > But yeah, ideally I think we want consistent line endings in
web-platform-tests
> > too, although while there's no explicit check for that repo, it's always
> > possible for people to submit new changes with CRLF line endings, which
would
> > break us in the future.
> 
> Doesn't doing any sort of transformation like this pose a problem for the
auto-exporter?  We don't want a little change to look like it touched every line
due to a change in whitespace.

That's right -- with this way of doing things, if there was a file that had CRLF
endings upstream and was converted when added, and then one line was changed and
exported, I think it would look like the change touched every line. Even if this
works, it might be confusing.

> If it is a problem then perhaps we can get the upstream repo's configured to
disallow CRLF endings at commit time.  Usually I think that's from a
misconfigured git client on Windows.  +gsnedders who's probably the authority to
ask here.

That solution sounds better to me :-)

> That said if we can safely hack around this without causing export issues,
then that works for me too.

[Rick Byers, 2016-12-21 23:43:54]

On 2016/12/21 23:35:21, qyearsley wrote:
> On 2016/12/21 at 23:25:39, rbyers wrote:
> > On 2016/12/21 23:18:28, qyearsley wrote:
> > > On 2016/12/21 at 23:12:02, jeffcarp wrote:
> > > > On 2016/12/21 at 19:13:51, qyearsley wrote:
> > > > > Hi, this is a proposed change to get csswg-test imports going again.
> > > > > 
> > > > > Note, there's an alternative change that would also work: changing the
> > > top-level presubmit. But, adding one exception to one check in the
top-level
> > > presubmit seems a little messier than adding another little transformation
> when
> > > importing.
> > > > 
> > > > lgtm. I don't have a ton of context on this but it seems ok for now. In
> the
> > > longer term would it be feasible, if the list of files with CRLF in WPT is
> > > small, to submit PRs to WPT to remove them?
> > > 
> > > Yeah - The list of files is small. I made a PR a while ago when I first
> noticed
> > > this, but I don't think it ever got merged:
> > > https://github.com/w3c/csswg-test/pull/1157
> > > So that's still pending.
> > > 
> > > Looking at that again now, I'm not certain that that PR is correct.
> > > 
> > > But yeah, ideally I think we want consistent line endings in
> web-platform-tests
> > > too, although while there's no explicit check for that repo, it's always
> > > possible for people to submit new changes with CRLF line endings, which
> would
> > > break us in the future.
> > 
> > Doesn't doing any sort of transformation like this pose a problem for the
> auto-exporter?  We don't want a little change to look like it touched every
line
> due to a change in whitespace.
> 
> That's right -- with this way of doing things, if there was a file that had
CRLF
> endings upstream and was converted when added, and then one line was changed
and
> exported, I think it would look like the change touched every line. Even if
this
> works, it might be confusing.
> 
> > If it is a problem then perhaps we can get the upstream repo's configured to
> disallow CRLF endings at commit time.  Usually I think that's from a
> misconfigured git client on Windows.  +gsnedders who's probably the authority
to
> ask here.
> 
> That solution sounds better to me :-)
> 
> > That said if we can safely hack around this without causing export issues,
> then that works for me too.

I suggest just hopping on #testing on irc.w3.org and asking gsnedders/jgraham
what they think about this.

[m_gsnedders, 2016-12-21 23:53:35]

On Wed, Dec 21, 2016 at 11:35 PM,  <qyearsley@chromium.org> wrote:
> On 2016/12/21 at 23:25:39, rbyers wrote:
>> If it is a problem then perhaps we can get the upstream repo's configured
>> to
> disallow CRLF endings at commit time. Usually I think that's from a
> misconfigured git client on Windows. +gsnedders who's probably the authority
> to
> ask here.
>
> That solution sounds better to me :-)

Yo.

So the status quo for a long time has been that new files in wpt (and
more recently csswg-test) aren't allowed to use CRLF. This is
maintained by the /lint tool, taking into account /lint.whitelist.
There may be new things added if there's large test-dumps where it
isn't practical to convert them, though that seems quite unlikely.

*However*, CRLF is only one of the things that the lint tool checks
for, and ideally we won't be introducing any failed lints. One
potential option is to just import the lint tool and related files and
require that to always pass on Chromium check-in.

/g

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[m_gsnedders, 2016-12-21 23:53:35]

On Wed, Dec 21, 2016 at 11:35 PM,  <qyearsley@chromium.org> wrote:
> On 2016/12/21 at 23:25:39, rbyers wrote:
>> If it is a problem then perhaps we can get the upstream repo's configured
>> to
> disallow CRLF endings at commit time. Usually I think that's from a
> misconfigured git client on Windows. +gsnedders who's probably the authority
> to
> ask here.
>
> That solution sounds better to me :-)

Yo.

So the status quo for a long time has been that new files in wpt (and
more recently csswg-test) aren't allowed to use CRLF. This is
maintained by the /lint tool, taking into account /lint.whitelist.
There may be new things added if there's large test-dumps where it
isn't practical to convert them, though that seems quite unlikely.

*However*, CRLF is only one of the things that the lint tool checks
for, and ideally we won't be introducing any failed lints. One
potential option is to just import the lint tool and related files and
require that to always pass on Chromium check-in.

/g

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[qyearsley, 2016-12-22 00:16:15]

On 2016/12/21 at 23:53:35, me wrote:
> On Wed, Dec 21, 2016 at 11:35 PM,  <qyearsley@chromium.org> wrote:
> > On 2016/12/21 at 23:25:39, rbyers wrote:
> >> If it is a problem then perhaps we can get the upstream repo's configured
> >> to
> > disallow CRLF endings at commit time. Usually I think that's from a
> > misconfigured git client on Windows. +gsnedders who's probably the authority
> > to
> > ask here.
> >
> > That solution sounds better to me :-)
> 
> Yo.
> 
> So the status quo for a long time has been that new files in wpt (and
> more recently csswg-test) aren't allowed to use CRLF. This is
> maintained by the /lint tool, taking into account /lint.whitelist.
> There may be new things added if there's large test-dumps where it
> isn't practical to convert them, though that seems quite unlikely.
> 
> *However*, CRLF is only one of the things that the lint tool checks
> for, and ideally we won't be introducing any failed lints. One
> potential option is to just import the lint tool and related files and
> require that to always pass on Chromium check-in.
> 
> /g

Excellent, this is good news. There's already a check for CRLF endings
in Chromium, and if we can expect there to generally be no CRLF endings
in web-platform-tests (and csswg-test) in the future, then I don't
think this CL is necessary.

Related: Now trying to update https://github.com/w3c/csswg-test/pull/1157

[m_gsnedders, 2016-12-22 00:23:15]

On Thu, Dec 22, 2016 at 12:16 AM,  <qyearsley@chromium.org> wrote:
> On 2016/12/21 at 23:53:35, me wrote:
>> On Wed, Dec 21, 2016 at 11:35 PM, <qyearsley@chromium.org&gt; wrote:
>> > On 2016/12/21 at 23:25:39, rbyers wrote:
>> >> If it is a problem then perhaps we can get the upstream repo's
>> >> configured
>> >> to
>> > disallow CRLF endings at commit time. Usually I think that's from a
>> > misconfigured git client on Windows. +gsnedders who's probably the
>> > authority
>> > to
>> > ask here.
>> >
>> > That solution sounds better to me :-)
>>
>> Yo.
>>
>> So the status quo for a long time has been that new files in wpt (and
>> more recently csswg-test) aren't allowed to use CRLF. This is
>> maintained by the /lint tool, taking into account /lint.whitelist.
>> There may be new things added if there's large test-dumps where it
>> isn't practical to convert them, though that seems quite unlikely.
>>
>> *However*, CRLF is only one of the things that the lint tool checks
>> for, and ideally we won't be introducing any failed lints. One
>> potential option is to just import the lint tool and related files and
>> require that to always pass on Chromium check-in.
>>
>> /g
>
> Excellent, this is good news. There's already a check for CRLF endings
> in Chromium, and if we can expect there to generally be no CRLF endings
> in web-platform-tests (and csswg-test) in the future, then I don't
> think this CL is necessary.
>
> Related: Now trying to update https://github.com/w3c/csswg-test/pull/1157
>
> https://codereview.chromium.org/2598743002/

I'm not sure how realistic that expectation is, and I didn't mean to
create it! There's certainly some resistance to trying to fix too much
of things like that given it creates quite a lot of noise and churn
for relatively little gain. That might be worth bringing up on
public-test-infra@w3.org if you feel strongly!

/g

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[m_gsnedders, 2016-12-22 00:23:15]

On Thu, Dec 22, 2016 at 12:16 AM,  <qyearsley@chromium.org> wrote:
> On 2016/12/21 at 23:53:35, me wrote:
>> On Wed, Dec 21, 2016 at 11:35 PM, <qyearsley@chromium.org&gt; wrote:
>> > On 2016/12/21 at 23:25:39, rbyers wrote:
>> >> If it is a problem then perhaps we can get the upstream repo's
>> >> configured
>> >> to
>> > disallow CRLF endings at commit time. Usually I think that's from a
>> > misconfigured git client on Windows. +gsnedders who's probably the
>> > authority
>> > to
>> > ask here.
>> >
>> > That solution sounds better to me :-)
>>
>> Yo.
>>
>> So the status quo for a long time has been that new files in wpt (and
>> more recently csswg-test) aren't allowed to use CRLF. This is
>> maintained by the /lint tool, taking into account /lint.whitelist.
>> There may be new things added if there's large test-dumps where it
>> isn't practical to convert them, though that seems quite unlikely.
>>
>> *However*, CRLF is only one of the things that the lint tool checks
>> for, and ideally we won't be introducing any failed lints. One
>> potential option is to just import the lint tool and related files and
>> require that to always pass on Chromium check-in.
>>
>> /g
>
> Excellent, this is good news. There's already a check for CRLF endings
> in Chromium, and if we can expect there to generally be no CRLF endings
> in web-platform-tests (and csswg-test) in the future, then I don't
> think this CL is necessary.
>
> Related: Now trying to update https://github.com/w3c/csswg-test/pull/1157
>
> https://codereview.chromium.org/2598743002/

I'm not sure how realistic that expectation is, and I didn't mean to
create it! There's certainly some resistance to trying to fix too much
of things like that given it creates quite a lot of noise and churn
for relatively little gain. That might be worth bringing up on
public-test-infra@w3.org if you feel strongly!

/g

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[qyearsley, 2016-12-28 19:30:37]

On 2016/12/22 at 00:23:15, me wrote:
> On Thu, Dec 22, 2016 at 12:16 AM,  <qyearsley@chromium.org> wrote:
> > On 2016/12/21 at 23:53:35, me wrote:
> >> On Wed, Dec 21, 2016 at 11:35 PM, <qyearsley@chromium.org&gt; wrote:
> >> > On 2016/12/21 at 23:25:39, rbyers wrote:
> >> >> If it is a problem then perhaps we can get the upstream repo's
> >> >> configured
> >> >> to
> >> > disallow CRLF endings at commit time. Usually I think that's from a
> >> > misconfigured git client on Windows. +gsnedders who's probably the
> >> > authority
> >> > to
> >> > ask here.
> >> >
> >> > That solution sounds better to me :-)
> >>
> >> Yo.
> >>
> >> So the status quo for a long time has been that new files in wpt (and
> >> more recently csswg-test) aren't allowed to use CRLF. This is
> >> maintained by the /lint tool, taking into account /lint.whitelist.
> >> There may be new things added if there's large test-dumps where it
> >> isn't practical to convert them, though that seems quite unlikely.
> >>
> >> *However*, CRLF is only one of the things that the lint tool checks
> >> for, and ideally we won't be introducing any failed lints. One
> >> potential option is to just import the lint tool and related files and
> >> require that to always pass on Chromium check-in.
> >>
> >> /g
> >
> > Excellent, this is good news. There's already a check for CRLF endings
> > in Chromium, and if we can expect there to generally be no CRLF endings
> > in web-platform-tests (and csswg-test) in the future, then I don't
> > think this CL is necessary.
> >
> > Related: Now trying to update https://github.com/w3c/csswg-test/pull/1157
> >
> > https://codereview.chromium.org/2598743002/
> 
> I'm not sure how realistic that expectation is, and I didn't mean to
> create it! There's certainly some resistance to trying to fix too much
> of things like that given it creates quite a lot of noise and churn
> for relatively little gain. That might be worth bringing up on
> public-test-infra@w3.org if you feel strongly!
> 
> /g
> 
> -- 
> You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to chromium-reviews+unsubscribe@chromium.org.
> 

I'm now thinking of abandoning this CL because it shouldn't be necessary
assuming that the wpt lint script checks for CRLF endings.
If this is an issue in the future, we may want to change the test importer to
skip tests that have CRLF endings and emit a warning or message suggesting to
fix the file upstream.