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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for CRLF files #36

Merged
merged 1 commit into from Apr 5, 2022
Merged

Add support for CRLF files #36

merged 1 commit into from Apr 5, 2022

Conversation

clemenswasser
Copy link
Contributor

This is crucial to support Windows. Since I'm not that familiar with the Ninja syntax, I don't really know how to test this in depth.
I have only changed the parse_defaults to using CRLF and tested it on the CMake Ninja test file, which at least doesn't fail because of CRLF anymore 馃槈

Copy link
Owner

@evmar evmar left a comment

Choose a reason for hiding this comment

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

I was expecting a change around here somewhere too

https://github.com/evmar/n2/blob/main/src/depfile.rs#L38

Is it possible your .d files are not crlf?
Or you don't have .d files at all?

src/parse.rs Outdated
let mut buf = "
var = 3
default a b$var c
let mut buf = "\r\n
Copy link
Owner

Choose a reason for hiding this comment

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

I think the \r\n behavior would be better as its own test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've now made a function test_for_line_endings, which tests some Input for both line endings. I hope it's okay, since this prevents test code duplication.

@evmar
Copy link
Owner

evmar commented Apr 5, 2022

Is it possible your .d files are not crlf? Or you don't have .d files at all?

Answering my own question, on Windows you likely don't have .d files, so don't worry about this part.

@evmar
Copy link
Owner

evmar commented Apr 5, 2022

Thanks!

@evmar evmar merged commit c3e370b into evmar:main Apr 5, 2022
@evmar
Copy link
Owner

evmar commented Apr 5, 2022

BTW, I wanted to warn you that the Windows support is still pretty unfinished:

4d78339

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants