Skip to content

Commit

Permalink
fix: Module fixes (#1594)
Browse files Browse the repository at this point in the history
* chore: removed unneeded `info modulepath`

And any code creating module paths because a list with only modules is
either the same as the list of dependencies (class path) or it would
contain only "real" modules which is a useless distinction.

* refactor: minor rollback of changes to `resolveInJavaHome()`

Earlier changes were moving in the direction of passing a `Jdk` object
instead of a version string. But that is actually not future save,
where there might be a point where we would want to be able to say:
"okay, I've used 'javac' from Jdk X to compile this and now I need
'native-image', but it's not in this Jdk X. Is there perhaps a newer
Jdk I could use?". If we pass a `Jdk` object we would be stuck at the
current version.

* chore: minor change in running `jar`

Now using the generic `Util.runCommand()`

* fix: fixed issue with module name handling

Setting the module name to an empty string on the command line would not
result in the expected behavior (turn the script into a module with a
name generated from the script name)

* fix: fixed info classpath implementation

* chore: make `ModuleUtil.isModule()` slightly smarter

The method will now also look for an alternative location for the
`module-info.class` file. This should still be improved though.

* chore: minor text change

* fix: `CmdGeneratorBuilder` now has its own `moduleName`

This is similar to `mainClass` which also exists both in the `Project`
and in the `CmdGenerator`. And for the same reason: we want to be able
to use a different value at runtime than that at buid time.
But unlike `mainClass` the value for `moduleName` is not always copied
as the default value from `Project` to `CmdGenerator`; we don't always
want to run JARs as modules by default, for example, but instead we'd
like the user to state their intentions explicitely by passing
`--module` to the `run` command.

* feat: added `info jar` and some other options

The command `info classpath` now takes an optional `--deps-only` and
`info tools` taken an optional `--select` where you tell it to only
return information for that specific field.

* chore: allow `//MODULE` tag without name

This will cause JBang to generate a proper module name.

* chore: rewording of error message

* feat: `//MODULE` is now more usable

JBang will now automatically add all system modules as required and
will open the script's package to everyone. This is not ideal because it
breaks all encapsulation, but it results in code that will work in most
cases. If you want better control you'll have to fall back to using a
separate `module-info.java` file.

* test: fixed tests not always working right

First of all having a `@BeforeEach` with the same method name in a base
class and a sub class can sometimes cause problems, especially when you
try to run single tests.
And the second problem would normally occur only on Windows where the
TEMP folder is actually in the same filesystem tree as the user files,
meaning that when we created a temp folder strcture for the tests the
code that looks for aliases and catalogs etc would still return actual
user files, ruining the tests.
  • Loading branch information
quintesse committed Mar 17, 2023
1 parent bb84139 commit d1a2f17
Show file tree
Hide file tree
Showing 27 changed files with 331 additions and 147 deletions.
21 changes: 21 additions & 0 deletions src/main/java/dev/jbang/Settings.java
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ public class Settings {
public static final String JBANG_REPO = "JBANG_REPO";
public static final String JBANG_DIR = "JBANG_DIR";
public static final String JBANG_CACHE_DIR = "JBANG_CACHE_DIR";
public static final String JBANG_LOCAL_ROOT = "JBANG_LOCAL_ROOT";

public static final String TRUSTED_SOURCES_JSON = "trusted-sources.json";
public static final String DEPENDENCY_CACHE_JSON = "dependency_cache.json";
Expand Down Expand Up @@ -81,6 +82,26 @@ public static Path getConfigEditorDir() {
return getConfigDir(true).resolve(EDITOR_DIR);
}

/**
* This returns the directory where the lookup of "local" files should stop.
* Local files are files like `jbang-catalog.json` that are read from the
* current directory and then recursively upwards until the root of the file
* system has been reached or until the folder returned by this method was
* encountered.
*
* @return The directory where local lookup should be stopped
*/
public static Path getLocalRootDir() {
Path dir;
String jlr = System.getenv(JBANG_LOCAL_ROOT);
if (jlr != null) {
dir = Paths.get(jlr);
} else {
dir = Paths.get(System.getProperty("user.home"));
}
return dir;
}

public static void setupJBangDir(Path dir) {
// create JBang configuration dir if it does not yet exist
dir.toFile().mkdirs();
Expand Down
79 changes: 48 additions & 31 deletions src/main/java/dev/jbang/cli/Info.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,10 @@
import static dev.jbang.Settings.CP_SEPARATOR;

import java.io.IOException;
import java.lang.reflect.Field;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
Expand All @@ -24,11 +23,12 @@
import dev.jbang.net.JdkProvider;
import dev.jbang.source.*;
import dev.jbang.util.JavaUtil;
import dev.jbang.util.ModuleUtil;

import picocli.CommandLine;

@CommandLine.Command(name = "info", description = "Provides info about the script for tools (and humans who are tools).", subcommands = {
Tools.class, ClassPath.class, ModulePath.class })
Tools.class, ClassPath.class, Jar.class })
public class Info {
}

Expand Down Expand Up @@ -84,7 +84,6 @@ static class ScriptInfo {
List<String> dependencies;
List<Repo> repositories;
List<String> resolvedDependencies;
List<String> resolvedModules;
String javaVersion;
String requestedJavaVersion;
String availableJdkPath;
Expand Down Expand Up @@ -113,6 +112,7 @@ public ScriptInfo(Project prj, BuildContext ctx, boolean assureJdkInstalled) {
? ctx.getNativeImageFile().toAbsolutePath().toString()
: null;
mainClass = prj.getMainClass();
module = ModuleUtil.getModuleName(prj);
requestedJavaVersion = prj.getJavaVersion();

try {
Expand All @@ -125,18 +125,14 @@ public ScriptInfo(Project prj, BuildContext ctx, boolean assureJdkInstalled) {
// Ignore
}

String cp = prj.resolveClassPath().getClassPath();
if (cp.isEmpty()) {
List<ArtifactInfo> artifacts = prj.resolveClassPath().getArtifacts();
if (artifacts.isEmpty()) {
resolvedDependencies = Collections.emptyList();
} else {
resolvedDependencies = Arrays.asList(cp.split(CP_SEPARATOR));
}

String mp = prj.resolveClassPath().getModulePath();
if (mp.isEmpty()) {
resolvedModules = null;
} else {
resolvedModules = Arrays.asList(cp.split(CP_SEPARATOR));
resolvedDependencies = artifacts
.stream()
.map(a -> a.getFile().toString())
.collect(Collectors.toList());
}

if (prj.getJavaVersion() != null) {
Expand All @@ -152,7 +148,7 @@ public ScriptInfo(Project prj, BuildContext ctx, boolean assureJdkInstalled) {
Project jarProject = Project.builder().build(ctx.getJarFile());
mainClass = jarProject.getMainClass();
gav = jarProject.getGav().orElse(gav);
module = jarProject.getModuleName().orElse(module);
module = ModuleUtil.getModuleName(jarProject);
}
}
}
Expand Down Expand Up @@ -239,11 +235,38 @@ ProjectBuilder createProjectBuilder() {
@CommandLine.Command(name = "tools", description = "Prints a json description usable for tools/IDE's to get classpath and more info for a jbang script/application. Exact format is still quite experimental.")
class Tools extends BaseInfoCommand {

@CommandLine.Option(names = {
"--select" }, description = "Indicate the name of the field to select and return from the full info result")
String select;

@Override
public Integer doCall() throws IOException {

Gson parser = new GsonBuilder().disableHtmlEscaping().setPrettyPrinting().create();
parser.toJson(getInfo(true), System.out);
ScriptInfo info = getInfo(true);
if (select != null) {
try {
Field f = info.getClass().getDeclaredField(select);
Object v = f.get(info);
if (v != null) {
if (v instanceof String || v instanceof Number) {
System.out.println(v);
} else {
parser.toJson(v, System.out);
}
} else {
// We'll return an error code for `null` so
// any calling scripts can easily detect that
// situation instead of having to ambiguously
// compare against the string "null"
return EXIT_GENERIC_ERROR;
}
} catch (NoSuchFieldException | IllegalAccessException e) {
throw new ExitException(EXIT_INVALID_INPUT, "Cannot return value of unknown field: " + select, e);
}
} else {
parser.toJson(info, System.out);
}

return EXIT_OK;
}
Expand All @@ -252,12 +275,17 @@ public Integer doCall() throws IOException {
@CommandLine.Command(name = "classpath", description = "Prints class-path used for this application using operating system specific path separation.")
class ClassPath extends BaseInfoCommand {

@CommandLine.Option(names = {
"--deps-only" }, description = "Only include the dependencies in the output, not the application jar itself")
boolean dependenciesOnly;

@Override
public Integer doCall() throws IOException {

ScriptInfo info = getInfo(false);
List<String> cp = new ArrayList<>(info.resolvedDependencies.size() + 1);
if (info.applicationJar != null && !info.resolvedDependencies.contains(info.applicationJar)) {
if (!dependenciesOnly && info.applicationJar != null
&& !info.resolvedDependencies.contains(info.applicationJar)) {
cp.add(info.applicationJar);
}
cp.addAll(info.resolvedDependencies);
Expand All @@ -267,24 +295,13 @@ public Integer doCall() throws IOException {
}
}

@CommandLine.Command(name = "modulepath", description = "Prints module-path used for this application using operating system specific path separation.")
class ModulePath extends BaseInfoCommand {
@CommandLine.Command(name = "jar", description = "Prints the path to this application's JAR file.")
class Jar extends BaseInfoCommand {

@Override
public Integer doCall() throws IOException {

ScriptInfo info = getInfo(false);
if (info.resolvedModules != null) {
List<String> cp = new ArrayList<>(info.resolvedModules.size() + 1);
if (info.applicationJar != null
&& !info.resolvedModules.contains(info.applicationJar)
&& ArtifactInfo.isModule(Paths.get(info.applicationJar))) {
cp.add(info.applicationJar);
}
cp.addAll(info.resolvedModules);
System.out.println(String.join(CP_SEPARATOR, cp));
}

System.out.println(info.applicationJar);
return EXIT_OK;
}
}
1 change: 1 addition & 0 deletions src/main/java/dev/jbang/cli/Run.java
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ CmdGeneratorBuilder updateGeneratorForRun(CmdGeneratorBuilder gb) {
.setArguments(userParams)
.runtimeOptions(runMixin.javaRuntimeOptions)
.mainClass(buildMixin.main)
.moduleName(buildMixin.module)
.interactive(runMixin.interactive)
.enableAssertions(runMixin.enableAssertions)
.enableSystemAssertions(runMixin.enableSystemAssertions)
Expand Down
11 changes: 1 addition & 10 deletions src/main/java/dev/jbang/dependencies/ArtifactInfo.java
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
package dev.jbang.dependencies;

import java.io.InputStream;
import java.net.URL;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Objects;
Expand Down Expand Up @@ -46,14 +44,7 @@ public boolean isModule() {
}

public static boolean isModule(Path file) {
try {
URL url = new URL("jar:" + file.toUri().toURL() + "!/module-info.class");
try (InputStream s = url.openStream()) {
return true;
}
} catch (Exception ex) {
return false;
}
return ModuleUtil.isModule(file);
}

public String getModuleName() {
Expand Down
12 changes: 0 additions & 12 deletions src/main/java/dev/jbang/dependencies/ModularClassPath.java
Original file line number Diff line number Diff line change
Expand Up @@ -49,14 +49,6 @@ public String getClassPath() {
return String.join(CP_SEPARATOR, getClassPaths());
}

public List<String> getModulePaths() {
if (modulePaths == null) {
modulePaths = getArtifactPaths(artifacts.stream().filter(ArtifactInfo::isModule));
}

return modulePaths;
}

private List<String> getArtifactPaths(Stream<ArtifactInfo> artifacts) {
return artifacts
.map(it -> it.getFile().toAbsolutePath().toString())
Expand All @@ -65,10 +57,6 @@ private List<String> getArtifactPaths(Stream<ArtifactInfo> artifacts) {
.collect(Collectors.toList());
}

public String getModulePath() {
return String.join(CP_SEPARATOR, getModulePaths());
}

public String getManifestPath() {
return artifacts.stream()
.map(it -> it.getFile().toAbsolutePath().toUri())
Expand Down
4 changes: 3 additions & 1 deletion src/main/java/dev/jbang/source/AppBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,9 @@ public CmdGeneratorBuilder build() throws IOException {
}
}

return CmdGenerator.builder(project, ctx);
return CmdGenerator .builder(project, ctx)
.mainClass(project.getMainClass())
.moduleName(project.getModuleName().orElse(null));
}

public static boolean keepClasses() {
Expand Down
10 changes: 10 additions & 0 deletions src/main/java/dev/jbang/source/CmdGeneratorBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ public class CmdGeneratorBuilder {
private List<String> runtimeOptions = Collections.emptyList();

private String mainClass;
private String moduleName;
private Boolean interactive;
private Boolean enableAssertions;
private Boolean enableSystemAssertions;
Expand Down Expand Up @@ -61,6 +62,11 @@ public CmdGeneratorBuilder mainClass(String mainClass) {
return this;
}

public CmdGeneratorBuilder moduleName(String moduleName) {
this.moduleName = moduleName;
return this;
}

public CmdGeneratorBuilder interactive(Boolean interactive) {
this.interactive = interactive;
return this;
Expand Down Expand Up @@ -117,6 +123,7 @@ private JarCmdGenerator createJarCmdGenerator() {
.runtimeOptions(runtimeOptions)
.mainClass(mainClass)
.mainRequired(interactive != Boolean.TRUE)
.moduleName(moduleName)
.assertions(enableAssertions == Boolean.TRUE)
.systemAssertions(enableSystemAssertions == Boolean.TRUE)
.classDataSharing(
Expand Down Expand Up @@ -150,6 +157,9 @@ private void updateFromAlias(Alias alias) {
if (mainClass == null) {
mainClass(alias.mainClass);
}
if (moduleName == null) {
moduleName(alias.moduleName);
}
if (flightRecorderString == null) {
flightRecorderString(alias.jfr);
}
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/dev/jbang/source/Project.java
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import dev.jbang.dependencies.MavenRepo;
import dev.jbang.dependencies.ModularClassPath;
import dev.jbang.source.sources.JavaSource;
import dev.jbang.util.ModuleUtil;
import dev.jbang.util.Util;

/**
Expand Down Expand Up @@ -218,7 +219,7 @@ protected String getStableId() {
if (stableId == null) {
Stream<String> sss = mainSourceSet.getStableIdInfo();
if (moduleName != null) {
Stream<String> s = Stream.of(moduleName);
Stream<String> s = Stream.of(ModuleUtil.getModuleName(this));
sss = Stream.concat(sss, s);
}
stableId = Util.getStableID(sss);
Expand Down
13 changes: 8 additions & 5 deletions src/main/java/dev/jbang/source/TagReader.java
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public abstract class TagReader {
private static final String FILES_COMMENT_PREFIX = "FILES ";
private static final String SOURCES_COMMENT_PREFIX = "SOURCES ";
private static final String MAIN_COMMENT_PREFIX = "MAIN ";
private static final String MODULE_COMMENT_PREFIX = "MODULE ";
private static final String MODULE_COMMENT_PREFIX = "MODULE";
private static final String DESCRIPTION_COMMENT_PREFIX = "DESCRIPTION ";
private static final String GAV_COMMENT_PREFIX = "GAV ";

Expand Down Expand Up @@ -177,16 +177,19 @@ public Optional<String> getModule() {
Util.warnMsg(
"Multiple //MODULE lines found, only one should be defined in a source file. Using the first");
}
if (!Util.isValidModuleIdentifier(mods.get(0))) {
if (mods.get(0).isEmpty()) {
return Optional.of("");
} else if (Util.isValidModuleIdentifier(mods.get(0).substring(1))) {
return Optional.of(mods.get(0).substring(1));
} else {
throw new IllegalArgumentException(
"//MODULE line has wrong format, should be '//MODULE identifier]'");
"//MODULE line has wrong format, should be '//MODULE [identifier]'");
}
return Optional.of(mods.get(0));
}
}

protected boolean isModuleDeclare(String line) {
return line.startsWith(MODULE_COMMENT_PREFIX);
return line.equals(MODULE_COMMENT_PREFIX) || line.startsWith(MODULE_COMMENT_PREFIX + " ");
}

public Optional<String> getGav() {
Expand Down
Loading

0 comments on commit d1a2f17

Please sign in to comment.