Skip to content

cmake: modules: using posix path for Kconfig generated file #15290

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

Merged

Conversation

tejlmand
Copy link
Collaborator

@tejlmand tejlmand commented Apr 9, 2019

Fixes: #15289

Kconfig requires posix style path when sourcing other files.
As abspath in python will use native host style, i.e. backslash '' in
windows this will cause invalid paths in Kconfig generated file and
thus the file will never be loaded.

This commit uses PurePath to convert the path to posix style, ensuring
Kconfig can load the modules.

Signed-off-by: Torsten Rasmussen [email protected]

Fixes: zephyrproject-rtos#15289

Kconfig requires posix style path when sourcing other files.
As abspath in python will use native host style, i.e. backslash '\' in
windows this will cause invalid paths in Kconfig generated file and
thus the file will never be loaded.

This commit uses PurePath to convert the path to posix style, ensuring
Kconfig can load the modules.

Signed-off-by: Torsten Rasmussen <[email protected]>
Copy link
Member

@carlescufi carlescufi left a comment

Choose a reason for hiding this comment

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

LGTM

@ulfalizer
Copy link
Collaborator

ulfalizer commented Apr 9, 2019

Just as an FYI, Kconfiglib can deal with Windows paths (except for the gotcha here). The problem is that "C:\foo\bar" is transformed into "C:foobar", due to how escaping works in Kconfig.

Wonder if I should change that so that unknown stuff is preserved instead. The only escapes are \" and \\ anyway. I just copied the C tools.

@@ -19,7 +19,7 @@
import pykwalify.core
import subprocess
import re

from pathlib import PurePath
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another option is to import posixpath and then do posixpath.abspath(kconfig_file), etc., if you consistently want to deal with *nix paths.

os.path is posixpath if you're on *nix, and ntpath if you're on Windows.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, that could also be a choice.
Only reason for deciding on PurePath here, is because it is what is also used in west.

@tejlmand
Copy link
Collaborator Author

tejlmand commented Apr 9, 2019

Just as an FYI, Kconfiglib can deal with Windows paths (except for the gotcha here). The problem is that "C:\foo\bar" is transformed into "C:foobar", due to how escaping works in Kconfig.

Yeah, I had the assumption that a \\ might also would have worked, but as posixpath is always working I find that to be the cleanest solution.

@ulfalizer
Copy link
Collaborator

Just as an FYI, Kconfiglib can deal with Windows paths (except for the gotcha here). The problem is that "C:\foo\bar" is transformed into "C:foobar", due to how escaping works in Kconfig.

Yeah, I had the assumption that a \\ might also would have worked, but as posixpath is always working I find that to be the cleanest solution.

Yeah, the real problem might be overzealous compatibility with the C tools.

@carlescufi carlescufi added the bug The issue is a bug, or the PR is fixing a bug label Apr 9, 2019
@carlescufi carlescufi added this to the v1.14.0 milestone Apr 9, 2019
@carlescufi carlescufi merged commit ad026c0 into zephyrproject-rtos:master Apr 9, 2019
@marc-hb
Copy link
Collaborator

marc-hb commented Apr 9, 2019

BTW forward slashes work pretty much everywhere in Windows except in CMD.exe. Use PowerShell :-)

@tejlmand tejlmand deleted the zephyr_module_kconfig_fix branch December 9, 2020 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug The issue is a bug, or the PR is fixing a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants