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

Remove quotes in the argument of python /app/print-hash.py #91

Merged
merged 1 commit into from Jul 25, 2022

Conversation

dukecat0
Copy link
Member

Fix #90

@dukecat0 dukecat0 changed the title Remove quotes Remove quotes in the argument of python /app/print-hash.py Feb 2, 2022
@@ -45,7 +45,7 @@ if [[ ${INPUT_VERBOSE,,} != "false" ]] ; then
fi

if [[ ${INPUT_PRINT_HASH,,} != "false" || ${INPUT_VERBOSE,,} != "false" ]] ; then
python /app/print-hash.py "${INPUT_PACKAGES_DIR%%/}"
python /app/print-hash.py ${INPUT_PACKAGES_DIR%%/}
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this patch may end up opening a can of worms because unquoted variable interpolation may turn into multiple arguments if it has spaces in it.
Have you checked such problematic scenarios? I'd very much like to be more confident with this...
Also, why not change this script to work the same way as other commands accepting ${INPUT_PACKAGES_DIR%%/}/*?
Any ideas?

Copy link
Member Author

@dukecat0 dukecat0 Jul 25, 2022

Choose a reason for hiding this comment

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

I just had a few tests on it.

For example, a path like /github/workspace/dist package/ may fail all twine commands since there is a space in the path.
Also, error like this will be displayed:

Warning:  It looks like there are no Python distribution packages to publish in the directory 'dist package/'. Please verify that they are in place should you face this problem.

Because these arguments are not quoted:

if ( ! ls -A ${INPUT_PACKAGES_DIR%%/}/*.tar.gz &> /dev/null && \
! ls -A ${INPUT_PACKAGES_DIR%%/}/*.whl &> /dev/null )

twine check ${INPUT_PACKAGES_DIR%%/}/*

exec twine upload ${TWINE_EXTRA_ARGS} ${INPUT_PACKAGES_DIR%%/}/*

Copy link
Member Author

@dukecat0 dukecat0 Jul 25, 2022

Choose a reason for hiding this comment

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

Also, why not change this script to work the same way as other commands accepting ${INPUT_PACKAGES_DIR%%/}/*?

Because iterdir() is used in the script so using ${INPUT_PACKAGES_DIR%%/}/* will result in NotADirectoryError.

Copy link
Member

Choose a reason for hiding this comment

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

I meant that you wouldn't need to use iterdir() because the outer shell would resolve that into multiple arguments IIUC.

Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

Looking at #90 (comment), I think I'll merge this now. But for the future, it may be worth looking into improving the quoting.

@webknjaz webknjaz merged commit 37f50c2 into pypa:master Jul 25, 2022
@dukecat0 dukecat0 deleted the remove-quotes branch July 25, 2022 16:57
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.

FileNotFoundError: [Errno 2] No such file or directory: '/github/workspace/wheelhouse/*'
2 participants