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

Use mktemp when creating temporary files. #6471

Closed
wants to merge 1 commit into from

Conversation

brown
Copy link
Contributor

@brown brown commented Oct 23, 2018

Before this change, the Java binary shell stub created temporary files in the
directory containing the binary, but that fails when the binary is stored in a
read-only file system or the directory is write protected.

Fixes issue #6289

Before this change, the Java binary shell stub created temporary files in the
directory containing the binary, but that fails when the binary is stored in a
read-only file system or the directory is write protected.

Fixes issue bazelbuild#6289
# Create manifest file
MANIFEST_FILE="${self}-${RAND_ID}.jar_manifest"
MANIFEST_FILE=$(mktemp)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
MANIFEST_FILE=$(mktemp)
MANIFEST_FILE="$(mktemp -d XXXXXXXX.jar_manifest)"

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bit more tolerant to weird $TMPDIRs and the resulting temporary file is better named.

Copy link
Contributor

Choose a reason for hiding this comment

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

@meteorcloudy , do you know why this wasn't using mktemp in the first place?

Copy link
Member

Choose a reason for hiding this comment

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

There's no specific reason to not use mktemp. I'm fine with this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

For the template you probably wanted something like /tmp/XXXXXXXX.jar_manifest instead of XXXXXXXX.jar_manifest; see @brown's comment below.

@@ -325,7 +324,7 @@ function create_and_run_classpath_jar() {
) >$MANIFEST_FILE

# Create classpath JAR file
MANIFEST_JAR_FILE="${self}-${RAND_ID}-classpath.jar"
MANIFEST_JAR_FILE=$(mktemp)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
MANIFEST_JAR_FILE=$(mktemp)
MANIFEST_JAR_FILE="$(mktemp).jar"

@cushon
Copy link
Contributor

cushon commented Oct 23, 2018

still LGTM

@bazel-io bazel-io closed this in a0a9da8 Oct 23, 2018
@brown
Copy link
Contributor Author

brown commented Oct 28, 2018

When applying my patch, someone substituted
MANIFEST_JAR_FILE="$(mktemp XXXXXXXX-classpath.jar)"
for
MANIFEST_JAR_FILE=$(mktemp)

The substitution breaks the patch. Running just "mktemp" creates a temporary file
in a temporary directory. When a template argument is supplied to mktemp, the
temporary file is created in the current directory, which is what the original patch
was trying to avoid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes team-Rules-Java Issues for Java rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants