Skip to content

Conversation

davidsanfal
Copy link
Contributor

@davidsanfal davidsanfal commented Jul 30, 2025

Changelog: Fix: Raise an exception when trying to create a directory at a path where a file of the same name already exists.
Docs: Omit

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

@davidsanfal davidsanfal requested review from czoido and AbrilRBS July 30, 2025 08:30
@memsharded memsharded added this to the 2.20 milestone Jul 30, 2025
Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

I know this check is super fast, but it still reads a bit overly protective, "overkill":

  • It protects against an error that is extremely unlikely
  • It only provides a better error message (or avoid a stack trace)
  • These low-level functions might be called many times inside Conan internal code. Maybe not the case for this one, but still

It might be better to just change os.path.exists() => os.path.isdir() and let the subsequent os.makedirs() to raise the exception about that?

These functions are also a bit legacy, in the sense that they were introduced before the exists_ok=True argument was introduced in os.makedirs(). New code is already using directly os.makedirs() instead, so this wouldn't protect against this error in those cases.

@AbrilRBS
Copy link
Member

Note that the code here raises after the fact with a stacktrace when trying to build the generators, that's why the idea to prettify the root cause came to be, but yeah maybe it ends up being a tad too defensive

@davidsanfal
Copy link
Contributor Author

The problem arises when there is a file, so isdir leaves us in the same situation. It will try to create a path that exists and will fail with an uncontrolled error trace.
We can add the exists_ok=True flag, but in this case, I don't think it's better to indicate that the folder cannot be created. It is now more a question of UI and expected result.

@memsharded
Copy link
Member

Maybe the approach is to protect the caller of the mkdir, capturing the exception there.

The working hypothesis for mkdir, or precondition is that such a case doesn't exist. Most of the time, the mkdir might be used to create dirs inside the Conan cache, which is 100% controlled that such spureous files won't exist.

In any case, I was being also extremely picky about this, I think this is very likely very fast to execute, without any performance penalty anywhere, it was mostly to raise a general design principles awareness and potential risks of doing this kind of things if something is in a critical or massively called path.

This can be merged as-is, no need to invest more time in it, thanks!

Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

After talking with @AbrilRBS, I think this is still a bit too defensive.
Lets go to protect only the "build" folder-file collision in:

def run_build_method(conanfile, hook_manager):
    if os.path.isfile(conanfile.build_folder):
           raise ConanException(f"Can't create the build folder, it is an existing file: {conanfile.build_folder}")
    mkdir(conanfile.build_folder)
    ...

@czoido czoido removed this from the 2.20 milestone Sep 1, 2025
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.

4 participants