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

sis_setup_directory: fix crash on broken input symlink #236

Merged
merged 1 commit into from
Feb 6, 2025

Conversation

patrick-wilken
Copy link
Contributor

@patrick-wilken patrick-wilken commented Jan 10, 2025

Small thing that annoys me from time to time: linking the job inputs fails with "File exists" if the creator path does not exist (and multiple inputs have same creator) because isdir(broken_symlink) == False.
If you run into this, clearly something else is wrong, but should fail at another place, not here.

Copy link
Contributor

@michelwi michelwi left a comment

Choose a reason for hiding this comment

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

could we even remove the isdir check now?

@patrick-wilken
Copy link
Contributor Author

Hmm, would work, yes. Except if somehow one manages to create an actual directory (or file) with that name. Most robust would actually be to check exists and islink, then we never crash because of existing file here.

@patrick-wilken
Copy link
Contributor Author

So, should I change to exists?

@michelwi
Copy link
Contributor

michelwi commented Feb 6, 2025

So, should I change to exists?

I have no strong opinion and accept either way.. you may go ahead and implement the more robust check.

@patrick-wilken patrick-wilken force-pushed the pwilken-fix-input-symlink-crash branch from 4e827fd to a2b8d34 Compare February 6, 2025 12:49
@patrick-wilken
Copy link
Contributor Author

Okay, done. Also quickly checked that it works as expected.

@patrick-wilken patrick-wilken merged commit 2bf1d94 into master Feb 6, 2025
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.

2 participants