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

Added "\r" to WHITESPACE char for windows build. #69

Merged
merged 1 commit into from
Mar 17, 2025

Conversation

coskunergan
Copy link
Contributor

@coskunergan coskunergan commented Mar 13, 2025

see: #68

Copy link
Collaborator

@d3zd3z d3zd3z left a comment

Choose a reason for hiding this comment

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

A few minor issues, just with the commit text:

  1. The commit needs to be signed off by. This is inherited from the Zephyr project, although we should probably come up with guidelines for this project.
  2. There should be a bit more description in the commit text. Linking to the GH issue is fine (although just Fixes: #66 is better, as it will auto-fix when this merges).
  3. There should be a short tag in front of the title. Something like "zephyr-build: " is fine.
  4. Lastly, even more minor, the commit text should be in the present tense. Using the past tense probably feels a little more natural when writing, but the commits persist, and are generally read in the context of history.

What I'd suggest is something like:

zephyr-build: fix DTS parser for Windows

The zephyr-build DTS parser fails on Windows, due to the "\r\n" whitespace
used on that platform. Accept the "\r" in addition to the "\n" to allow it to
parse successfully.

Fixes: #66

Signed-off-by: Firstname Lastname <coskungeran@gmail>

I could update this for you if you'd prefer, but I do need to know what fullname to use.

@@ -74,4 +74,4 @@ nodename = @{
(ASCII_ALPHANUMERIC | "_" | "," | "." | "?" | "-" | "@" | "#")+
}

WHITESPACE = _{ " " | "\n" | "\t" }
WHITESPACE = _{ " " | "\r" | "\n" | "\t" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

This definitely will work, although I think technically it should be "\r\n" and "\n", but this solution will also allow this to run on ancient Macs as well.

The zephyr-build DTS parser fails on Windows, due to the "\r\n" whitespace
used on that platform. Accept the "\r" in addition to the "\n" to allow it to
parse successfully.

Fixes: zephyrproject-rtos#68
See: zephyrproject-rtos#68

Signed-off-by: coskun ergan <[email protected]>
@d3zd3z
Copy link
Collaborator

d3zd3z commented Mar 17, 2025

CI issue with docs is unrelated.

@d3zd3z d3zd3z merged commit 7395ae4 into zephyrproject-rtos:main Mar 17, 2025
3 of 4 checks passed
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.

3 participants