Handle Windows style line-endings in transport_security_state_static.pins.

Handle Windows style line-endings in transport_security_state_static.pins.

The build fails when the pins file uses Windows style line-endings:

[0605/151613.923:ERROR:input_file_parsers.cc(193)] Invalid name in pinsfile:
[0605/151613.923:ERROR:transport_security_state_generator.cc(213)] Error while parsing the input files.

The actual file uses correct Unix style line-endings that get converted
to Windows style in some checkouts. This causes the build to fail with a
confusing error message.

BUG=729553

[martijnc, 2017-06-05 14:17:50]

The CQ bit was checked by martijn@martijnc.be to run a CQ dry run

[commit-bot: I haz the power, 2017-06-05 14:18:02]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[martijnc, 2017-06-05 14:18:05]

Description was changed from

==========
Handle Windows style line-endings in tranport_security_state_static.pins.

The build fails when the pins file uses Windows style line-endings:

[0605/151613.923:ERROR:input_file_parsers.cc(193)] Invalid name in pinsfile:
[0605/151613.923:ERROR:transport_security_state_generator.cc(213)] Error while
parsing the input files.

The actual file uses correct Unix style line-endings that get converted
to Windows style in some checkouts. This causes the build to fail with a
confusing error message.

BUG=729553
==========

to

==========
Handle Windows style line-endings in transport_security_state_static.pins.

The build fails when the pins file uses Windows style line-endings:

[0605/151613.923:ERROR:input_file_parsers.cc(193)] Invalid name in pinsfile:
[0605/151613.923:ERROR:transport_security_state_generator.cc(213)] Error while
parsing the input files.

The actual file uses correct Unix style line-endings that get converted
to Windows style in some checkouts. This causes the build to fail with a
confusing error message.

BUG=729553
==========

[commit-bot: I haz the power, 2017-06-05 15:35:30]

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

[commit-bot: I haz the power, 2017-06-05 15:35:31]

Dry run: This issue passed the CQ dry run.

[martijnc, 2017-06-05 17:50:05]

martijn@martijnc.be changed reviewers:
+ rsleevi@chromium.org

[martijnc, 2017-06-05 17:50:06]

Some Windows users end up with incorrect line-endings in their checkout and the
generator fails because of that. The generator is the only reason the build
fails for those people.

This CL relaxes the pins file parsing by trimming whitespace characters at the
line edges.

Can you review? Thanks!

[Ryan Sleevi, 2017-06-08 18:59:45]

On 2017/06/05 17:50:06, martijnc wrote:
> Some Windows users end up with incorrect line-endings in their checkout and
the
> generator fails because of that. The generator is the only reason the build
> fails for those people.
> 
> This CL relaxes the pins file parsing by trimming whitespace characters at the
> line edges.
> 
> Can you review? Thanks!

I'm inclined to say no, because as noted, Chrome expects autocrlf to be false as
a whole. I think what you're doign is reasonable to address the symptom, but as
far as root causes go, making sure all files observe the autocrlf false property
seems like the better answer - and working with Infra to make sure checkouts
always observe that is the best solution.

Does that make sense why I'm Not LGTMing this? :)

[martijnc, 2017-06-08 20:15:56]

On 2017/06/08 at 18:59:45, rsleevi wrote:
> I'm inclined to say no, because as noted, Chrome expects autocrlf to be false
as a whole. I think what you're doign is reasonable to address the symptom, but
as far as root causes go, making sure all files observe the autocrlf false
property seems like the better answer - and working with Infra to make sure
checkouts always observe that is the best solution.
> 
> Does that make sense why I'm Not LGTMing this? :)

I was worried this was a bigger problem when I sent this out as there were
multiple reports from users building on Windows and the build change was fairly
recent. Having had a couple more days to think about it, I think these report
may have been a coincidence. The generator has been running during the build for
over 2 months to generate test data which would have resulted in the same error.
So I'm happy keeping the parsing strict - especially given the potential impact
of parsing errors in the pins files.

From the reactions on the bug it appears depot_tools already set the autocrlf
value to false automatically. The problem occurs when the value is somehow
overridden.

If more people run into this it might be worthwhile adding a check to detect the
incorrect line-endings and output a more informative error message.

[Daniel Bratell, 2017-06-13 11:54:57]

On 2017/06/08 20:15:56, martijnc wrote:
> On 2017/06/08 at 18:59:45, rsleevi wrote:
> > I'm inclined to say no, because as noted, Chrome expects autocrlf to be
false
> as a whole. I think what you're doign is reasonable to address the symptom,
but
> as far as root causes go, making sure all files observe the autocrlf false
> property seems like the better answer - and working with Infra to make sure
> checkouts always observe that is the best solution.
> > 
> > Does that make sense why I'm Not LGTMing this? :)
> 
> I was worried this was a bigger problem when I sent this out as there were
> multiple reports from users building on Windows and the build change was
fairly
> recent. Having had a couple more days to think about it, I think these report
> may have been a coincidence. The generator has been running during the build
for
> over 2 months to generate test data which would have resulted in the same
error.
> So I'm happy keeping the parsing strict - especially given the potential
impact
> of parsing errors in the pins files.
> 
> From the reactions on the bug it appears depot_tools already set the autocrlf
> value to false automatically. The problem occurs when the value is somehow
> overridden.
> 
> If more people run into this it might be worthwhile adding a check to detect
the
> incorrect line-endings and output a more informative error message.

The only thing making autocrlf=false is that the bundled git in depot_tools is
hardcoded to do that. If you use a different git (intentionally or by accident)
then nothing stops the code from being checked out with CRLF line endings.
Normally it doesn't matter because all code is robust.

If it must be LF, then we should add a .gitattributes file to enforce it.