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

Bugfix: fixes a CLI argument bug for the conda-debug command #4448

Merged
merged 11 commits into from May 12, 2022

Conversation

travishathaway
Copy link
Contributor

@travishathaway travishathaway commented May 4, 2022

Addresses the following issue:

Although at first glance this issue seem pretty straight forward, it was not. The bug unearthed a dormant code path that was revealed by some interesting behavior in Python.

After some debugging, it became obvious that this line wasn't doing what it intended to do:

if not any(os.path.splitext(_args.recipe_or_package_file_path)[1] in ext for ext in CONDA_PACKAGE_EXTENSIONS):

If I'm correct, the intent of the above line was to filter out everything out that was not a "conda package file", so in theory, directories would be diverted to this code path. In reality, it also allows strings like, /tmp/some-dir/ to evaluate as True, which means directories have always been sent to the next else block (currently the working and expected behavior).

Why is that so? It all has to do with the fact that '' in 'somestring evaluates to True. Check out the example for details:

>>> exts = {'.conda', '.tar.bz2'}
>>> path1 = '/tmp/some-dir-not-trailing-slash'
>>> os.path.splitext(path1)[1] in exts
False
>>> path1 = '/tmp/some-dir-with-slash/'
>>> os.path.splitext(path2)[1] in exts
True

With all that being said, I think it is fair to assume that the code executed under this if statement was never actually working in the first place because when it is run, it generates the error that we see. Therefore, I made the decision to entirely remove the execution of this code.

I'm not entirely sure why that was ever there in the first place, but it's definitely been broken for a bit. Additionally, the conda_build.cli.main_debug module had zero test coverage when I began, so it's very easy to imagine that this broke a while back, and we were unaware of it.

If you know more about this command, please comment, so we can find the best way to fix it!

@anaconda-issue-bot anaconda-issue-bot added the cla-signed [bot] added once the contributor has signed the CLA label May 4, 2022
@travishathaway travishathaway force-pushed the bug/conda-debug-file-path-4404 branch 2 times, most recently from 670f90c to 78da400 Compare May 4, 2022 12:49
@travishathaway travishathaway changed the title Bugfix: fixes a path bug for the conda-debug command Bugfix: fixes an CLI argument bug for the conda-debug command May 4, 2022
@travishathaway travishathaway changed the title Bugfix: fixes an CLI argument bug for the conda-debug command Bugfix: fixes a CLI argument bug for the conda-debug command May 4, 2022
@travishathaway travishathaway force-pushed the bug/conda-debug-file-path-4404 branch from 798e6e3 to 84f04df Compare May 4, 2022 13:56
Copy link
Contributor

@kenodegard kenodegard left a comment

Choose a reason for hiding this comment

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

I'm not thrilled about adding a new submodule but can see the value in standardizing validators. Perhaps we should identify a few other places where using validators will help simplify the code before we move ahead with this change?

conda_build/utils.py Show resolved Hide resolved
conda_build/cli/main_debug.py Outdated Show resolved Hide resolved
conda_build/validators.py Outdated Show resolved Hide resolved
conda_build/validators.py Outdated Show resolved Hide resolved
tests/cli/test_main_debug.py Outdated Show resolved Hide resolved
tests/test_validators.py Outdated Show resolved Hide resolved
conda_build/validators.py Outdated Show resolved Hide resolved
@travishathaway travishathaway force-pushed the bug/conda-debug-file-path-4404 branch 2 times, most recently from d73ae5b to d2ca8de Compare May 5, 2022 09:00
Copy link
Contributor

@kenodegard kenodegard left a comment

Choose a reason for hiding this comment

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

Looking good, just noticed that there's more we can remove since you removed the unused if clause.

conda_build/cli/validators.py Outdated Show resolved Hide resolved
conda_build/cli/main_debug.py Outdated Show resolved Hide resolved
@kenodegard
Copy link
Contributor

@travishathaway you may also want to rebase/squash 15f9605 to change the author/committer

@travishathaway travishathaway force-pushed the bug/conda-debug-file-path-4404 branch 2 times, most recently from c16b7fd to d78f075 Compare May 9, 2022 14:55
@kenodegard kenodegard force-pushed the bug/conda-debug-file-path-4404 branch from d78f075 to 46be298 Compare May 9, 2022 19:13
Copy link
Contributor

@kenodegard kenodegard left a comment

Choose a reason for hiding this comment

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

Everything looks good, just want to make sure we aren't changing any user-facing messages that don't need to be changed.

conda_build/cli/main_debug.py Outdated Show resolved Hide resolved
conda_build/cli/main_debug.py Outdated Show resolved Hide resolved
@travishathaway
Copy link
Contributor Author

Putting this on hold until I can resolve a comment from another one of our projects:
conda/conda#11463 (comment)

(I just need to implement fsspec instead of pyfakefs)

@travishathaway travishathaway force-pushed the bug/conda-debug-file-path-4404 branch from 46be298 to 203590d Compare May 10, 2022 16:25
@travishathaway
Copy link
Contributor Author

Putting this on hold until I can resolve a comment from another one of our projects: conda/conda#11463 (comment)

(I just need to implement fsspec instead of pyfakefs)

The pytest fixture tmpdir actually solved this problem for me.

@kenodegard kenodegard force-pushed the bug/conda-debug-file-path-4404 branch from 581bd3d to 11fdfb2 Compare May 11, 2022 21:50
Copy link
Member

@jezdez jezdez left a comment

Choose a reason for hiding this comment

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

Nice!

@jezdez jezdez merged commit 1295594 into master May 12, 2022
@jezdez jezdez deleted the bug/conda-debug-file-path-4404 branch May 12, 2022 10:53
@github-actions github-actions bot added the locked [bot] locked due to inactivity label May 13, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-signed [bot] added once the contributor has signed the CLA locked [bot] locked due to inactivity
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

conda debug fails when called on a path without trailing slash on Windows
4 participants