ninja-build / ninja

a small build system with a focus on speed
https://ninja-build.org/
Apache License 2.0
11.33k stars 1.62k forks source link

\r\n in dep files under windows + cygwin breaks ninja #752

Closed RedX2501 closed 10 years ago

RedX2501 commented 10 years ago

When generated dependency files have \r\n in them ninja will not parse them properly. Instead it will output something like:

is dirtylain: \ is dirtylain: \ is dirtylain: \ is dirtylain: \ is dirtylain: \ is dirtylain: \ is dirtylain: \ is dirtylain: \ is dirtylain: \ is dirtylain: \ ninja explain: MCAL/bin/eTIMER_Gpt_LLD_IRQ.o is dirty

When piped to a file under vim i see:

1 ninja explain: ^M is dirty 2 ninja explain: ^M is dirty 3 ninja explain: ^M is dirty 4 ninja explain: ^M is dirty 5 ninja explain: ^M is dirty 6 ninja explain: ^M is dirty 7 ninja explain: ^M is dirty 8 ninja explain: ^M is dirty 9 ninja explain: ^M is dirty 10 ninja explain: ^M is dirty 11 ninja explain: ^M is dirty 12 ninja explain: ^M is dirty 13 ninja explain: ^M is dirty 14 ninja explain: ^M is dirty 15 ninja explain: CDD/bin/Cdd_Ssbm.o is dirty

Indicating that the \r in the file is culprit. Using the tool dos2unix on the generated depfiles let's ninja work properly confirming that the \r\n was at fault.

nico commented 10 years ago

Yes, depfiles don't support \r\n style newlines. (The error for this should be better.)

Where are you getting this depfile from? cygwin gcc's -MD -MD foo.d writes depfiles with just \n for me.

RedX2501 commented 10 years ago

The compiler is called Windriver. Not very popular.

I think an option is to strip \r additionally then you get universal newline compatibility. I can't see any downside to that.

nico commented 10 years ago

Arguments for rejecting \r bytes in depfiles:

Arguments for supporting \r in depfiles:

I'm leaning towards accepting \r bytes in depfiles. If anyone has an opinion on this (e.g. if \r support in depfiles is useful for you, or if you can think of a critical flaw caused by supporting them), please let me know!

nico commented 10 years ago

I merged the patch to accept \r\n line endings. This should work better on trunk.