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 Files.createTempFile #141

Merged
merged 10 commits into from
Jun 21, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
22 changes: 22 additions & 0 deletions .github/workflows/slack-alert.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
name: slack-alert

on:
workflow_run:
workflows: [tests]
types: [completed]

jobs:
on-failure:
runs-on: ubuntu-latest
if: ${{ github.event.workflow_run.conclusion == 'failure' }}
steps:
- name: Send Slack Alert
id: slack
uses: slackapi/[email protected]
with:
payload: |
Github Actions ${{ github.event.workflow_run.conclusion }}
Repo: ${{github.event.workflow_run.repository.name }}
Workflow URL: ${{ github.event.workflow_run.html_url }}
env:
SLACK_WEBHOOK_URL: ${{ secrets.SLACK_WEBHOOK_URL }}
8 changes: 6 additions & 2 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ on:
branches: [master, stable-*]
pull_request:
branches: [master, stable-*]
schedule:
# Every Sunday, rerun
- cron: "0 12 * * 0"
Comment on lines +8 to +10
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this re-run? What would the test suite catch when re-run?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I feel like it's often my experience that the CI process for this repo turns out to have some problem when I go to make a change here, and I'd rather be alerted to that proactively rather than when trying to fix something else. Perhaps the CI issues encountered here are indeed not something that would be usefully caught by this; if that's the case then maybe we don't need it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. There definitely seems to be some flakiness involving the docker images. I wonder if it would make more sense to pull the Dockerfile into the repo and have the test suite build its own docker image to then run? I guess that would depend on how long it takes to build the image.


jobs:
tests:
Expand Down Expand Up @@ -64,7 +67,7 @@ jobs:
os: [ubuntu-latest]
java-distribution: [adopt]
java-version: [17]
dockcross-tag: ["20220906-e88a3ce", "20230116-670f7f7"]
dockcross-tag: ["20230116-670f7f7", "20240529-0dade71", "latest"]

steps:
- uses: actions/[email protected]
Expand Down Expand Up @@ -94,7 +97,8 @@ jobs:
strategy:
matrix:
# TODO: Docker on macos-latest running is not working
os: [macos-latest, windows-latest]
# TODO: Windows pinned back
os: [macos-latest, windows-2019]
java-distribution: [adopt]
java-version: [8, 11, 15, 17]

Expand Down
47 changes: 38 additions & 9 deletions src/main/java/com/uber/h3core/H3CoreLoader.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2017-2018 Uber Technologies, Inc.
* Copyright 2017-2018, 2024 Uber Technologies, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -20,7 +20,12 @@
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.nio.file.Files;
import java.nio.file.attribute.FileAttribute;
import java.nio.file.attribute.PosixFilePermission;
import java.nio.file.attribute.PosixFilePermissions;
import java.util.Locale;
import java.util.Set;

/** Extracts the native H3 core library to the local filesystem and loads it. */
public final class H3CoreLoader {
Expand Down Expand Up @@ -60,11 +65,6 @@ private static void copyStream(InputStream in, OutputStream out) throws IOExcept
* @throws UnsatisfiedLinkError The resource path does not exist
*/
static void copyResource(String resourcePath, File newH3LibFile) throws IOException {
// Set the permissions
newH3LibFile.setReadable(true);
newH3LibFile.setWritable(true, true);
newH3LibFile.setExecutable(true, true);

// Shove the resource into the file and close it
try (InputStream resource = H3CoreLoader.class.getResourceAsStream(resourcePath)) {
if (resource == null) {
Expand Down Expand Up @@ -93,6 +93,24 @@ public static NativeMethods loadNatives() throws IOException {
return loadNatives(os, arch);
}

private static File createTempLibraryFile(OperatingSystem os) throws IOException {
if (os.isPosix()) {
// Note this is already done by the implementation of Files.createTempFile that I looked at,
// but the javadoc does not seem to gaurantee the permissions will be restricted to owner
// write.
final FileAttribute<Set<PosixFilePermission>> attr =
PosixFilePermissions.asFileAttribute(PosixFilePermissions.fromString("rwx------"));
return Files.createTempFile("libh3-java", os.getSuffix(), attr).toFile();
} else {
// When not a POSIX OS, try to ensure the permissions are secure
final File f = Files.createTempFile("libh3-java", os.getSuffix()).toFile();
f.setReadable(true, true);
f.setWritable(true, true);
f.setExecutable(true, true);
return f;
}
}

/**
* For use when the H3 library should be unpacked from the JAR and loaded. The native library for
* the specified operating system and architecture will be extract.
Expand All @@ -115,8 +133,7 @@ public static synchronized NativeMethods loadNatives(OperatingSystem os, String
final String dirName = String.format("%s-%s", os.getDirName(), arch);
final String libName = String.format("libh3-java%s", os.getSuffix());

final File newLibraryFile = File.createTempFile("libh3-java", os.getSuffix());

final File newLibraryFile = createTempLibraryFile(os);
newLibraryFile.deleteOnExit();

copyResource(String.format("/%s/%s", dirName, libName), newLibraryFile);
Expand Down Expand Up @@ -146,13 +163,20 @@ public enum OperatingSystem {
ANDROID(".so"),
DARWIN(".dylib"),
FREEBSD(".so"),
WINDOWS(".dll"),
WINDOWS(".dll", false),
LINUX(".so");

private final String suffix;
private final boolean posix;

OperatingSystem(String suffix) {
this.suffix = suffix;
this.posix = true;
}

OperatingSystem(String suffix, boolean posix) {
this.suffix = suffix;
this.posix = posix;
}

/** Suffix for native libraries. */
Expand All @@ -164,6 +188,11 @@ public String getSuffix() {
public String getDirName() {
return name().toLowerCase(H3_CORE_LOCALE);
}

/** Whether to try to use POSIX file permissions when creating the native library temp file. */
public boolean isPosix() {
return this.posix;
}
}

/**
Expand Down
3 changes: 2 additions & 1 deletion src/test/java/com/uber/h3core/TestH3CoreLoader.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
import org.junit.Test;

/** H3CoreLoader is mostly tested by {@link TestH3CoreFactory}. This also tests OS detection. */
Expand Down Expand Up @@ -60,7 +61,7 @@ public void testDetectArch() {

@Test(expected = UnsatisfiedLinkError.class)
public void testExtractNonexistant() throws IOException {
File tempFile = File.createTempFile("test-extract-resource", null);
File tempFile = Files.createTempFile("test-extract-resource", null).toFile();

tempFile.deleteOnExit();

Expand Down
Loading