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.

Sign up for GitHub

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

[HOTFIX] Fix PySpark pip packaging tests by non-ascii compatible character #22782

Closed
wants to merge 7 commits into from
Closed

[HOTFIX] Fix PySpark pip packaging tests by non-ascii compatible character #22782

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
25435dc
Fix
HyukjinKwon Oct 20, 2018
cd5cab6
python test
HyukjinKwon Oct 20, 2018
5439ff2
skip scala tests
HyukjinKwon Oct 20, 2018
38abee5
skip scala tests
HyukjinKwon Oct 20, 2018
8743bb1
Revert "python test"
HyukjinKwon Oct 20, 2018
352f922
Revert "skip scala tests"
HyukjinKwon Oct 20, 2018
114fe0f
Revert "skip scala tests"
HyukjinKwon Oct 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bin/docker-image-tool.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ function build {
fi

# Verify that Spark has actually been built/is a runnable distribution
# i.e. the Spark JARs that the Docker files will place into the image are present
Copy link
Member

Choose a reason for hiding this comment

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

Hi, @rvesse .
Do you have any clue about the special characters after '#' found by @kiszk at here?
I'm wondering if we can avoid this situation in the future. Do you have any idea?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, some text editors insert non-breakable spaces and probably that's why. I dealt with similar problems at md files before. I think we have scalastyle for nonascii but looks not for scripts ..

Copy link
Member

Choose a reason for hiding this comment

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

Right. We need some automatic check for scripts too. Anyway, thank you for fixing this, @HyukjinKwon ! This blocks almost everything PRs.

Copy link
Member

Choose a reason for hiding this comment

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

Huh, this was edited in Xcode on OS X so almost certainly defaulting to UTF-8 encoding

Copy link
Member

Choose a reason for hiding this comment

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

Huh, this was edited in Xcode on OS X so almost certainly defaulting to UTF-8 encoding

Copy link
Member Author

Choose a reason for hiding this comment

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

It was more because non-breakable space was used instead instead of space. It was in utf-8 but non ascii compatible.

Copy link
Member

Choose a reason for hiding this comment

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

Obvious question but why are we still using ASCII encoding for anything?

Copy link
Member Author

Choose a reason for hiding this comment

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

For the issue itself, It's related to a historical reason for Python. Python 2 supported str type as bytes like string. It looked a mistake that confuses users about the concept between bytes and string, and then Python 3 introduced str as unicode strings concepts like other programing languages.

open(...).read() reads it as str (which is bytes) in Python 2 but it's read in unicode strings in Python 3 - where we need an implicit conversion between bytes and strings. Looks it had to be to minimise the breaking changes in users codes.

So, bytes to string conversion happened here and unfortunately our Jenkins's system default encoding is set to ascii (even though arguably UTF-8 is common).

For non-ascii itself, please see the justification at http://www.scalastyle.org/rules-dev.html in ScalaStyle.

Copy link
Member Author

Choose a reason for hiding this comment

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

Therefore, using non-breakable spaces in the codes is obviously not a good practice. Please avoid next time.

Copy link
Member

Choose a reason for hiding this comment

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

It wasn't as if I used a non-breaking space intentionally, I just used my OSes default editor for Shell scripts!

# i.e. the Spark JARs that the Docker files will place into the image are present
local TOTAL_JARS=$(ls $JARS/spark-* | wc -l)
TOTAL_JARS=$(( $TOTAL_JARS ))
if [ "${TOTAL_JARS}" -eq 0 ]; then
Expand Down