Skip to content

fix: resource references with property resolving #2153

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.

Sign up for GitHub

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

Open
maxandersen wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix: resource references with property resolving #2153

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8991a1b
fix: resource references with property resolving
maxandersen Aug 9, 2025
5ae89f5
Merge branch 'main' into aliasresolve
maxandersen Aug 12, 2025
aded982
chore: add missing test resource
maxandersen Aug 22, 2025
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
10 changes: 10 additions & 0 deletions itests/linuxxyz.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
///usr/bin/env jbang "$0" "$@" ; exit $?

import static java.lang.System.*;

public class linux {

public static void main(String... args) {
out.println("Hello linux");
}
}
10 changes: 10 additions & 0 deletions itests/osxxyz.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
///usr/bin/env jbang "$0" "$@" ; exit $?

import static java.lang.System.*;

public class osx {

public static void main(String... args) {
out.println("Hello osx");
}
}
10 changes: 10 additions & 0 deletions itests/winxyz.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
///usr/bin/env jbang "$0" "$@" ; exit $?

import static java.lang.System.*;

public class win {

public static void main(String... args) {
out.println("Hello Windows");
}
}
15 changes: 14 additions & 1 deletion src/main/java/dev/jbang/dependencies/Detector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
import java.util.List;
import java.util.Properties;

import dev.jbang.util.Util;

public class Detector extends kr.motd.maven.os.Detector {

public Detector() {
Expand All @@ -16,13 +18,19 @@ protected void log(String message) {

@Override
protected void logProperty(String name, String value) {

Util.verboseMsg("Auto-detected: " + name + "=" + value);
}

public void detect(Properties properties, List<String> classiferWithLikes) {
super.detect(properties, classiferWithLikes);

// "hack" to expose a property that works with javafx mac classifers
// https://repo1.maven.org/maven2/org/openjfx/javafx-controls/26-ea+3/
// linux-aarch64
// linux
// mac-aarch64
// mac
// win
String os = properties.getProperty("os.detected.name");
if (os.equals("osx")) {
os = "mac";
Expand All @@ -31,6 +39,11 @@ public void detect(Properties properties, List<String> classiferWithLikes) {
}
} else if (os.equals("windows")) {
os = "win";
} else if (os.equals("linux")) {
os = "linux";
if ("aarch64".equals(System.getProperty("os.arch"))) {
os = "linux-aarch64";
}
}
properties.setProperty("os.detected.jfxname", os);
}
Expand Down
34 changes: 32 additions & 2 deletions src/main/java/dev/jbang/source/ProjectBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,12 +258,41 @@ private List<MavenRepo> allToMavenRepo(List<String> repos) {
}

public Project build(String resource) {
ResourceRef resourceRef = resolveChecked(getResourceResolver(), resource);
ResourceRef resourceRef = resolveChecked(getResourceResolver(), resource, true);
return build(resourceRef);
}

private ResourceRef resolveChecked(ResourceResolver resolver, String resource) {
Util.verboseMsg("Resolving resource ref: " + resource);
return resolveChecked(resolver, resource, false);
}

/**
* Resolves the given resource, with property resolution from project context
* and handles retry on caching.
*
* Note: only enable property replacement if you know the resource is not
* something that can be embedded/included directly in the project from users
* data.
*
* i.e. jbang run someref${os.detected.os}.jar is ok but //FILES with
* ${user.home} is definitely not.
*
* @param resolver
Copy link
Contributor

@quintesse quintesse Aug 17, 2025

Choose a reason for hiding this comment

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

Either put proper docs or remove the @params :-)

* @param resource
* @return
*/
private ResourceRef resolveChecked(ResourceResolver resolver, String orginal_resource,
Copy link
Contributor

@quintesse quintesse Aug 17, 2025

Choose a reason for hiding this comment

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

This is already merged, but original_resource? Wtf? Did you use AI to generate this? 😁

Copy link
Collaborator Author

@maxandersen maxandersen Aug 17, 2025

Choose a reason for hiding this comment

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

This is what was already there afaik. Unless you identify as AI it was human :)

Copy link
Contributor

@quintesse quintesse Aug 17, 2025

Choose a reason for hiding this comment

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

Don't gaslight me, this code is new, the diff shows you just added it :-)

I would never ever write a (Java) variable name with an underscore, the very idea gives me shivers 😁

Copy link
Collaborator Author

@maxandersen maxandersen Aug 18, 2025

Choose a reason for hiding this comment

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

The only answer must be that I'm an AI then :)

boolean propertyReplacement) {
final String resource = propertyReplacement
? PropertiesValueResolver.replaceProperties(orginal_resource, getContextProperties())
: orginal_resource;

if (!resource.equals(orginal_resource)) {
Util.verboseMsg("Resolving resource ref: " + orginal_resource + " -> " + resource);
Copy link
Contributor

@quintesse quintesse Aug 17, 2025

Choose a reason for hiding this comment

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

Logging this is good, but I think the wording could be improved. In this case if there are no properties to be replaced you won't get a "resolving ..." message, which is wrong because we definitely are resolving. So I'd say that we'd either have to always log something or change the wording to something like "Applying properties" instead of "Resolving".

Copy link
Collaborator Author

@maxandersen maxandersen Aug 18, 2025

Choose a reason for hiding this comment

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

but something gets logged everytime - its just only when applying properties we put the "-> "

I did that to avoid it to say 99% of the time "Resolving resource ref: abc -> abc"

but if you can think of a better one do open pr.

Copy link
Contributor

@quintesse quintesse Aug 18, 2025

Choose a reason for hiding this comment

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

Well, that's why I said to simply change the text. Because it's not resolving, it's "applying properties to resource ref" or something like that. Just to avoid any confusion that that line suggests that resolving has been done/performed.

Copy link
Collaborator Author

@maxandersen maxandersen Aug 18, 2025

Choose a reason for hiding this comment

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

But resolve xyz is what we wrote before applying any properties. So are you saying the old one was wrong?

Copy link
Contributor

@quintesse quintesse Aug 18, 2025

Choose a reason for hiding this comment

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

Maybe I'm just misunderstanding things. Doesn't matter. Not important,

} else {
Util.verboseMsg("Resolving resource ref:" + resource);
}

boolean retryCandidate = catalogFile == null && !Util.isFresh() && Settings.getCacheEvict() > 0
&& (Catalog.isValidName(resource) || Catalog.isValidCatalogReference(resource)
|| Util.isRemoteRef(resource));
Expand Down Expand Up @@ -618,6 +647,7 @@ private ResourceResolver getAliasResourceResolver(Alias alias) {
if (alias != null) {
updateFromAlias(alias);
}

return new CombinedResourceResolver(
new RenamingScriptResourceResolver(forceType),
new LiteralScriptResourceResolver(forceType),
Expand Down
23 changes: 23 additions & 0 deletions src/test/java/dev/jbang/cli/TestInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,15 @@
import java.io.IOException;
import java.nio.file.Paths;
import java.util.Collection;
import java.util.Collections;
import java.util.Properties;

import org.hamcrest.Matchers;
import org.junit.jupiter.api.Test;

import dev.jbang.BaseTest;
import dev.jbang.dependencies.Detector;
import dev.jbang.util.PropertiesValueResolver;

import picocli.CommandLine;

Expand Down Expand Up @@ -198,4 +202,23 @@ void testInfoToolsWithDocs() throws IOException {
Tools docs = (Tools) pr.subcommand().subcommand().commandSpec().userObject();
docs.call();
}

@Test
void testMagicPropertyViaInfo() throws IOException {

String fileref = examplesTestFolder.resolve("${os.detected.name}${customvar}.java").toString();
// todo fix so --deps can use system properties
CommandLine.ParseResult pr = JBang.getCommandLine()
.parseArgs("info", "tools",
"-Dcustomvar=xyz",
fileref.toString());

Properties props = new Properties();
new Detector().detect(props, Collections.emptyList());
String expectedJar = PropertiesValueResolver.replaceProperties("${os.detected.name}xyz.java", props);

Tools tools = (Tools) pr.subcommand().subcommand().commandSpec().userObject();
BaseInfoCommand.ScriptInfo info = tools.getInfo(false);
assertThat(info.originalResource, endsWith(expectedJar));
}
}
29 changes: 29 additions & 0 deletions src/test/java/dev/jbang/cli/TestRun.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Properties;
import java.util.jar.Attributes;
import java.util.jar.JarFile;
import java.util.regex.Pattern;
Expand Down Expand Up @@ -65,6 +66,7 @@
import dev.jbang.catalog.Alias;
import dev.jbang.catalog.Catalog;
import dev.jbang.catalog.CatalogUtil;
import dev.jbang.dependencies.Detector;
import dev.jbang.net.TrustedSources;
import dev.jbang.source.BuildContext;
import dev.jbang.source.Builder;
Expand All @@ -80,6 +82,7 @@
import dev.jbang.source.sources.JavaSource;
import dev.jbang.util.CommandBuffer;
import dev.jbang.util.JavaUtil;
import dev.jbang.util.PropertiesValueResolver;
import dev.jbang.util.Util;

import picocli.CommandLine;
Expand Down Expand Up @@ -2101,6 +2104,32 @@ void testJavaFXMagicPropertyViaCommandline() throws IOException {

}

@Test
void testMagicPropertyViaRun(@TempDir Path tdir) throws IOException {

String fileref = examplesTestFolder.resolve("${os.detected.name}${customvar}.java").toString();
// todo fix so --deps can use system properties
CommandLine.ParseResult pr = JBang.getCommandLine()
.parseArgs("run",
"-Dcustomvar=xyz",
fileref.toString());

Run run = (Run) pr.subcommand().commandSpec().userObject();

ProjectBuilder pb = run.createProjectBuilderForRun();
pb.mainClass("fakemain");

Project prj = pb.build(fileref.toString());
String line = run.updateGeneratorForRun(CmdGenerator.builder(prj)).build().generate();

Properties props = new Properties();
new Detector().detect(props, Collections.emptyList());
String expectedJar = PropertiesValueResolver.replaceProperties("${os.detected.name}xyz.jar", props);

assertThat(line, containsString(expectedJar));

}

@Test
void testScriptCliReposAndDeps(@TempDir File output) throws IOException {
String base = "" +
Expand Down
Loading