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

KAFKA-4247: Prevent CLASSPATH from beginning with a single colon #4406

Closed
wants to merge 2 commits into from

Conversation

C0urante
Copy link
Contributor

@C0urante C0urante commented Jan 9, 2018

Different fix for problem addressed by #1953. Should prevent the CLASSPATH environment variable from being prefixed by a single colon before the JVM is invoked in the run-class script, which will then prevent the current working directory from being unintentionally included in the classpath when using the Reflections library.

If the current working directory should still be included in the classpath, it just needs to be explicitly specified either with its fully-qualified pathname or as a single dot (".").

@rnpridgeon
Copy link
Contributor

👍

Copy link
Contributor

@rhauch rhauch left a comment

Choose a reason for hiding this comment

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

Nicely done.

@C0urante
Copy link
Contributor Author

C0urante commented Jan 9, 2018

Not sure what's going on with the skipped tests, but after scanning the results it appears they've been skipped for several past builds as well so I don't think they should be a problem. @rhauch, as long as that seems reasonable to you I'm ready to request a merge from a committer.

@C0urante
Copy link
Contributor Author

C0urante commented Jan 9, 2018

@junrao seems ready to merge, mind taking a look? Looks like the build failures seem related to recent infrastructure changes and are not caused by the changes included in this PR.

Copy link
Contributor

@junrao junrao left a comment

Choose a reason for hiding this comment

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

@C0urante : Thanks for the patch. LGTM. Just a minor comment below.

@@ -260,6 +256,9 @@ if [ "x$GC_LOG_ENABLED" = "xtrue" ]; then
fi
fi

# Remove a possible colon prefix from the classpath (happens at lines like `CLASSPATH="$CLASSPATH:$file"` when CLASSPATH is blank)
CLASSPATH=${CLASSPATH#:}
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps just add a comment to explain what ${CLASSPATH#:} does.

@junrao
Copy link
Contributor

junrao commented Jan 9, 2018

@C0urante : Thanks for the update. LGTM

@junrao junrao closed this in abf4c54 Jan 9, 2018
@C0urante C0urante deleted the fix-colon-prefixed-classpath branch January 9, 2018 22:55
@C0urante
Copy link
Contributor Author

C0urante commented Jan 9, 2018

Thanks @junrao!

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