Skip to content

Commit cb59f0c

Browse files
whitewhite
white
authored and
white
committed
Apply code review improvements
- Change cache path from ~/.gradle/python-embed to ~/.python-embed for tool neutrality - Make pythonSource required in FingerprintManager.read() for consistency with write() - Add @optional to VenvTask.getPipExtraArgs() to prevent up-to-date false negatives - Replace /tmp/venv with @tempdir in VenvConfigTest for cross-platform correctness - Decouple FingerprintManagerTest from SHA-256 implementation detail - Standardize publish workflows: use :module:task syntax, remove working-directory - Add VenvManagerTest covering runCommand, error paths, and up-to-date skip logic
1 parent 20e71b7 commit cb59f0c

9 files changed

Lines changed: 105 additions & 25 deletions

File tree

‎.github/workflows/publish-build-common.yml‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,9 +11,6 @@ jobs:
1111
runs-on: ubuntu-latest
1212
permissions:
1313
contents: write
14-
defaults:
15-
run:
16-
working-directory: python-embed-build-common
1714
steps:
1815
- uses: actions/checkout@v4
1916

@@ -37,10 +34,10 @@ jobs:
3734
fi
3835
3936
- name: Build
40-
run: ./gradlew build${{ steps.version.outputs.VERSION && format(' -PreleaseVersion={0}', steps.version.outputs.VERSION) || '' }}
37+
run: ./gradlew :python-embed-build-common:build${{ steps.version.outputs.VERSION && format(' -PreleaseVersion={0}', steps.version.outputs.VERSION) || '' }}
4138

4239
- name: Publish to Maven Central
43-
run: ./gradlew publishToMavenCentral --no-configuration-cache${{ steps.version.outputs.VERSION && format(' -PreleaseVersion={0}', steps.version.outputs.VERSION) || '' }}
40+
run: ./gradlew :python-embed-build-common:publishToMavenCentral --no-configuration-cache${{ steps.version.outputs.VERSION && format(' -PreleaseVersion={0}', steps.version.outputs.VERSION) || '' }}
4441
env:
4542
ORG_GRADLE_PROJECT_mavenCentralUsername: ${{ secrets.MAVEN_CENTRAL_USERNAME }}
4643
ORG_GRADLE_PROJECT_mavenCentralPassword: ${{ secrets.MAVEN_CENTRAL_PASSWORD }}

‎.github/workflows/publish-gradle-plugin.yml‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,9 +11,6 @@ jobs:
1111
runs-on: ubuntu-latest
1212
permissions:
1313
contents: write
14-
defaults:
15-
run:
16-
working-directory: python-embed-gradle-plugin
1714
steps:
1815
- uses: actions/checkout@v4
1916

@@ -37,10 +34,10 @@ jobs:
3734
fi
3835
3936
- name: Build
40-
run: ./gradlew build${{ steps.version.outputs.VERSION && format(' -PreleaseVersion={0}', steps.version.outputs.VERSION) || '' }}
37+
run: ./gradlew :python-embed-gradle-plugin:build${{ steps.version.outputs.VERSION && format(' -PreleaseVersion={0}', steps.version.outputs.VERSION) || '' }}
4138

4239
- name: Publish to Gradle Plugin Portal
43-
run: ./gradlew publishPlugins --no-configuration-cache${{ steps.version.outputs.VERSION && format(' -PreleaseVersion={0}', steps.version.outputs.VERSION) || '' }}
40+
run: ./gradlew :python-embed-gradle-plugin:publishPlugins --no-configuration-cache${{ steps.version.outputs.VERSION && format(' -PreleaseVersion={0}', steps.version.outputs.VERSION) || '' }}
4441
env:
4542
GRADLE_PUBLISH_KEY: ${{ secrets.GRADLE_PUBLISH_KEY }}
4643
GRADLE_PUBLISH_SECRET: ${{ secrets.GRADLE_PUBLISH_SECRET }}

‎.github/workflows/publish-maven-plugin.yml‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,9 +11,6 @@ jobs:
1111
runs-on: ubuntu-latest
1212
permissions:
1313
contents: write
14-
defaults:
15-
run:
16-
working-directory: python-embed-maven-plugin
1714
steps:
1815
- uses: actions/checkout@v4
1916

@@ -37,10 +34,10 @@ jobs:
3734
fi
3835
3936
- name: Build
40-
run: ./gradlew build${{ steps.version.outputs.VERSION && format(' -PreleaseVersion={0}', steps.version.outputs.VERSION) || '' }}
37+
run: ./gradlew :python-embed-maven-plugin:build${{ steps.version.outputs.VERSION && format(' -PreleaseVersion={0}', steps.version.outputs.VERSION) || '' }}
4138

4239
- name: Publish to Maven Central
43-
run: ./gradlew publishToMavenCentral --no-configuration-cache${{ steps.version.outputs.VERSION && format(' -PreleaseVersion={0}', steps.version.outputs.VERSION) || '' }}
40+
run: ./gradlew :python-embed-maven-plugin:publishToMavenCentral --no-configuration-cache${{ steps.version.outputs.VERSION && format(' -PreleaseVersion={0}', steps.version.outputs.VERSION) || '' }}
4441
env:
4542
ORG_GRADLE_PROJECT_mavenCentralUsername: ${{ secrets.MAVEN_CENTRAL_USERNAME }}
4643
ORG_GRADLE_PROJECT_mavenCentralPassword: ${{ secrets.MAVEN_CENTRAL_PASSWORD }}

‎python-embed-build-common/src/main/java/io/github/howtis/pythonembed/build/FingerprintManager.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ public static Fingerprint read(Path venvDir) {
8181
String pythonSource = props.getProperty(KEY_PYTHON_SOURCE);
8282
String pythonVersion = props.getProperty(KEY_PYTHON_VERSION);
8383
String packageHash = props.getProperty(KEY_PACKAGES_HASH);
84-
if (pythonVersion == null || packageHash == null) {
84+
if (pythonSource == null || pythonVersion == null || packageHash == null) {
8585
return null;
8686
}
8787
return new Fingerprint(pythonSource, pythonVersion, packageHash);

‎python-embed-build-common/src/main/java/io/github/howtis/pythonembed/build/VenvManager.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,7 @@ public static PythonEnvironment setup(VenvConfig config) throws IOException {
105105
pythonSource = "bundled";
106106
String targetOs = PythonResolver.resolveTargetOs(config.targetOs());
107107
Path cacheDir = Path.of(System.getProperty("user.home"),
108-
".gradle", "python-embed");
108+
".python-embed");
109109
log.accept("System Python not found. Downloading python-build-standalone...");
110110
pythonExe = PythonDownloader.download(pythonVersion, targetOs, cacheDir, venvDir, log);
111111
}

‎python-embed-build-common/src/test/java/io/github/howtis/pythonembed/build/FingerprintManagerTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ void computePackageHash_pipExtraArgs_affectsHash() throws IOException {
5959
void computePackageHash_emptyPackages_producesValidHash() throws IOException {
6060
String hash = FingerprintManager.computePackageHash(List.of(), null, List.of(), null);
6161
assertNotNull(hash);
62-
assertEquals(64, hash.length());
62+
assertFalse(hash.isEmpty());
6363
}
6464

6565
@Test

‎python-embed-build-common/src/test/java/io/github/howtis/pythonembed/build/VenvConfigTest.java‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package io.github.howtis.pythonembed.build;
22

33
import org.junit.jupiter.api.Test;
4+
import org.junit.jupiter.api.io.TempDir;
45

56
import java.nio.file.Path;
67
import java.util.List;
@@ -11,9 +12,10 @@
1112
class VenvConfigTest {
1213

1314
@Test
14-
void builder_minimalConfig_returnsDefaults() {
15+
void builder_minimalConfig_returnsDefaults(@TempDir Path tempDir) {
16+
Path venvPath = tempDir.resolve("venv");
1517
VenvConfig config = VenvConfig.builder()
16-
.venvDir(Path.of("/tmp/venv"))
18+
.venvDir(venvPath)
1719
.build();
1820

1921
assertNotNull(config.packages());
@@ -24,16 +26,16 @@ void builder_minimalConfig_returnsDefaults() {
2426
assertNull(config.pipIndexUrl());
2527
assertNotNull(config.pipExtraArgs());
2628
assertTrue(config.pipExtraArgs().isEmpty());
27-
assertEquals(Path.of("/tmp/venv"), config.venvDir());
29+
assertEquals(venvPath, config.venvDir());
2830
assertNull(config.targetOs());
2931
}
3032

3133
@Test
32-
void builder_fullConfig_setsAllFields() {
34+
void builder_fullConfig_setsAllFields(@TempDir Path tempDir) {
3335
Consumer<String> logger = msg -> {};
3436
Path requirementsFile = Path.of("requirements.txt");
3537
Path pyprojectTomlFile = Path.of("pyproject.toml");
36-
Path venvDir = Path.of("/tmp/venv");
38+
Path venvDir = tempDir.resolve("venv");
3739

3840
VenvConfig config = VenvConfig.builder()
3941
.packages(List.of("numpy", "pandas"))
@@ -59,11 +61,11 @@ void builder_fullConfig_setsAllFields() {
5961
}
6062

6163
@Test
62-
void config_isImmutable_copyDefensive() {
64+
void config_isImmutable_copyDefensive(@TempDir Path tempDir) {
6365
List<String> packages = new java.util.ArrayList<>(List.of("numpy"));
6466
VenvConfig config = VenvConfig.builder()
6567
.packages(packages)
66-
.venvDir(Path.of("/tmp/venv"))
68+
.venvDir(tempDir.resolve("venv"))
6769
.build();
6870

6971
packages.add("pandas");

‎python-embed-build-common/src/test/java/io/github/howtis/pythonembed/build/VenvManagerTest.java‎

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
package io.github.howtis.pythonembed.build;
2+
3+
import org.junit.jupiter.api.Test;
4+
import org.junit.jupiter.api.io.TempDir;
5+
6+
import java.io.IOException;
7+
import java.nio.file.Files;
8+
import java.nio.file.Path;
9+
import java.util.List;
10+
import java.util.function.Consumer;
11+
12+
import static org.junit.jupiter.api.Assertions.*;
13+
14+
class VenvManagerTest {
15+
16+
@Test
17+
void runCommand_success() throws IOException {
18+
Consumer<String> log = msg -> {};
19+
assertDoesNotThrow(() -> VenvManager.runCommand(log, "java", "--version"));
20+
}
21+
22+
@Test
23+
void runCommand_failure_throwsIOException() {
24+
Consumer<String> log = msg -> {};
25+
assertThrows(IOException.class,
26+
() -> VenvManager.runCommand(log, "java", "--nonexistent-flag-12345"));
27+
}
28+
29+
@Test
30+
void setup_missingRequirementsFile_throwsIOException(@TempDir Path tempDir) {
31+
Path nonExistentReq = tempDir.resolve("nonexistent.txt");
32+
VenvConfig config = VenvConfig.builder()
33+
.venvDir(tempDir.resolve("venv"))
34+
.requirementsFile(nonExistentReq)
35+
.build();
36+
37+
IOException ex = assertThrows(IOException.class, () -> VenvManager.setup(config));
38+
assertTrue(ex.getMessage().contains("requirements.txt not found"));
39+
}
40+
41+
@Test
42+
void setup_missingPyprojectTomlFile_throwsIOException(@TempDir Path tempDir) {
43+
Path nonExistentPyproject = tempDir.resolve("nonexistent.toml");
44+
VenvConfig config = VenvConfig.builder()
45+
.venvDir(tempDir.resolve("venv"))
46+
.pyprojectTomlFile(nonExistentPyproject)
47+
.build();
48+
49+
IOException ex = assertThrows(IOException.class, () -> VenvManager.setup(config));
50+
assertTrue(ex.getMessage().contains("pyproject.toml not found"));
51+
}
52+
53+
@Test
54+
void setup_upToDate_skipsSetup(@TempDir Path tempDir) throws IOException {
55+
Path venvDir = tempDir.resolve("venv");
56+
Files.createDirectories(venvDir);
57+
58+
// Create a mock Python executable so findPythonInDir returns non-null
59+
Path pythonExe;
60+
if (PythonResolver.isWindows()) {
61+
pythonExe = venvDir.resolve("python.exe");
62+
} else {
63+
pythonExe = venvDir.resolve("bin").resolve("python3");
64+
Files.createDirectories(pythonExe.getParent());
65+
}
66+
Files.createFile(pythonExe);
67+
68+
// Write a fingerprint matching the expected configuration.
69+
// VenvManager.setup() always adds "msgpack" to the packages list,
70+
// so the fingerprint must include it.
71+
String pythonVersion = "3.12";
72+
String packageHash = FingerprintManager.computePackageHash(
73+
List.of("msgpack"), null, List.of(), null);
74+
FingerprintManager.write(venvDir, "system", pythonVersion, packageHash);
75+
76+
VenvConfig config = VenvConfig.builder()
77+
.venvDir(venvDir)
78+
.build();
79+
80+
PythonEnvironment result = VenvManager.setup(config);
81+
82+
assertNotNull(result);
83+
assertEquals(venvDir, result.venvDir());
84+
assertEquals("system", result.source());
85+
}
86+
}

‎python-embed-gradle-plugin/src/main/java/io/github/howtis/pythonembed/gradle/VenvTask.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,7 @@ public abstract class VenvTask extends DefaultTask {
9090
*
9191
* @return optional extra pip install arguments
9292
*/
93+
@Optional
9394
@Input
9495
public abstract ListProperty<String> getPipExtraArgs();
9596

0 commit comments

Comments
 (0)