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 crash when building inside a submodule #854

Merged
merged 1 commit into from
Aug 7, 2024

Conversation

ausbin
Copy link
Contributor

@ausbin ausbin commented Aug 6, 2024

If .git is a file instead of a directory (such as when you are building in a submodule), scikit-build-core will currently crash because a NotADirectoryError is thrown instead of a FileNotFoundError. This PR accounts for the resulting exception (NotADirectoryError) instead of crashing.

Another option instead of hardcoding this list would be to catch the more general OSError. However, I did not do that, because I am worried about silently swallowing too broad of a class of exceptions.

I wrote a unit test and also tested the fix against the project where I encountered this.

(By the way, this project is very helpful, thank you. The fact that I encountered this issue only a couple of hours after the release of 0.10.0 tells you how often I use it!)

@@ -13,7 +13,7 @@
__all__ = ["each_unignored_file"]

EXCLUDE_LINES = [
".git/",
".git",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is so the .git file itself does not show up in the list

@henryiii
Copy link
Collaborator

henryiii commented Aug 6, 2024

I think the error here actually depends on the operating system. python/cpython#119993 PermissionsError is another one.

If .git is a file instead of a directory (such as when you are building
in a submodule), scikit-build-core will currently crash. This commit
accounts for the resulting exception (NotADirectoryError) instead of
crashing.
Copy link

codecov bot commented Aug 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.88%. Comparing base (b6ad498) to head (858e5aa).
Report is 46 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #854   +/-   ##
=======================================
  Coverage   83.87%   83.88%           
=======================================
  Files          74       74           
  Lines        4361     4362    +1     
=======================================
+ Hits         3658     3659    +1     
  Misses        703      703           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@henryiii henryiii merged commit c8cb8b6 into scikit-build:main Aug 7, 2024
52 checks passed
@henryiii
Copy link
Collaborator

henryiii commented Aug 7, 2024

Thanks for a great and fast PR that included a test! I’ll release once I get #853 working on PyPy.

@ausbin ausbin deleted the bugfix/submodule-build branch August 7, 2024 00:10
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.

2 participants