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

fix: path_to_module to handle hidden files (e.g. .clang-tidy) correctly #987

Merged
merged 5 commits into from
Feb 13, 2025

Conversation

tae-jun
Copy link
Contributor

@tae-jun tae-jun commented Feb 6, 2025

It works fine with normal install pip install . but not for editable install pip install -e .

I got the error below without this fix because it can't handle hidden file's extension corretly:

    File "/opt/miniconda3/envs/pyapl/lib/python3.12/site-packages/scikit_build_core/build/_pathutil.py", line 31, in path_to_module
      path = path.with_name(path.name.split(".", 1)[0])
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    File "/opt/miniconda3/envs/pyapl/lib/python3.12/pathlib.py", line 634, in with_name
      raise ValueError("Invalid name %r" % (name))
  ValueError: Invalid name ''

For example, if it's .clang-tidy, the original splitting path.name.split(".", 1)[0] results empty string.

@LecrisUT
Copy link
Collaborator

LecrisUT commented Feb 6, 2025

Having a . at the start of the module name should not be supported because it clashes with relative imports import .foo. Can you provide more context on how you encounter this?

@tae-jun
Copy link
Contributor Author

tae-jun commented Feb 6, 2025

Hi, thanks for your quick response.

I have external library JUCE, and the problematic source is this .clang-tidy file.
(So the source is not what I wrote)

I guess .clang-tidy files doesn't need to be mapped as a module, so maybe we can just ignore them.

If sources are what I wrote, I would not use .starting_file_name but it's a lib that I didn't write.

Maybe I should have modified the library, but I hope scikit-build-core automatically handle such cases.

Thanks!

@LecrisUT
Copy link
Collaborator

LecrisUT commented Feb 6, 2025

Well .clang-tidy is not a python file, so it should not be picked up from the beginning, so we should hunt down on how to filter that. What is the pyproject.toml that you work with? There are 2 sources where the files are registered for editable install:

  • wheel.packages for the pure python files
  • install artifacts of CMake install() commands

Which ones does your pick up on. If it's the first case, it sounds like a bug that we should fix. If it's the second one, probably also a bug, but we need more context to see how it's being picked up

@henryiii
Copy link
Collaborator

henryiii commented Feb 7, 2025

Any ideas on a way to see the error? I tried adding these:

tests/packages/navigate_editable/python/shared_pkg/.clang-tidy
tests/packages/navigate_editable/src/shared_pkg/.clang-tidy
tests/packages/navigate_editable/src/shared_pkg/.gitignore

@LecrisUT
Copy link
Collaborator

LecrisUT commented Feb 7, 2025

Oh, I think I see where it comes from in that project. JUCE is installing all of its sources, including an additional .clang-tidy that OP linked to before.

https://github.com/juce-framework/JUCE/blob/51a8a6d7aeae7326956d747737ccf1575e61e209/extras/Build/CMake/JUCEModuleSupport.cmake#L672-L674
https://github.com/juce-framework/JUCE/blob/51a8a6d7aeae7326956d747737ccf1575e61e209/modules/CMakeLists.txt#L33-L35

One solution is to strictly filter out any .path since those should not be valid python paths anyway. A more long-term solution would be to eliminate any non-python recognizable file suffix files (I think the PR I did on the editable paths might have broken that?).

@henryiii
Copy link
Collaborator

henryiii commented Feb 7, 2025

Ahh, forgot to try adding it to CMakeLists.

Signed-off-by: Henry Schreiner <[email protected]>
Signed-off-by: Henry Schreiner <[email protected]>
@henryiii henryiii changed the title Fix path_to_module to handle hidden files (e.g. .clang-tidy) correctly fix: path_to_module to handle hidden files (e.g. .clang-tidy) correctly Feb 13, 2025
@henryiii henryiii merged commit 7568be0 into scikit-build:main Feb 13, 2025
57 of 58 checks passed
@henryiii
Copy link
Collaborator

henryiii commented Feb 13, 2025

(Only including valid file extensions could be a followup, but this is still a net improvement!) Thanks!

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