Allow making an executable-jar image by only providing an executable JAR #265

sap-ali · 2024-04-03T13:42:30Z

In this PR, I'm enabling the capability to only provide a standalone executable JAR without the need to have a file in META-INF/MANIFEST.MF

[Fixes #264]

linux-foundation-easycla · 2024-04-03T13:42:34Z

The committers listed above are authorized under a signed CLA.

✅ login: sap-ali / name: Ali Rabiee (824b6a9, b1ab799, 40ed839)

dmikusa · 2024-04-03T13:58:08Z

Can you expand more on the use case here? I saw you other issue, but it's not clear to me exactly what you're trying to do and why. Can you provide some more details? A working reproduction including code/steps to produce a JAR file that fits what you're trying to do would be helpful.

sap-ali · 2024-04-03T14:21:45Z

Can you expand more on the use case here? I saw you other issue, but it's not clear to me exactly what you're trying to do and why. Can you provide some more details? A working reproduction including code/steps to produce a JAR file that fits what you're trying to do would be helpful.

Let's say we have build a fat JAR from our project: example-fat.jar.

We can apply the buildpack to create an image that runs the jar using:

pack build example:latest \
  --path example-fat.jar
  --builder paketobuildpacks/builder:base \
  --buildpack gcr.io/paketo-buildpacks/sap-machine \
  --buildpack gcr.io/paketo-buildpacks/syft \
  --buildpack gcr.io/paketo-buildpacks/executable-jar

However, this has the downside that the JAR is exploded in the image, which can cause some side-effects (as it has in our case using Quarkus).

Specifying --path . would also not work because there is no META-INF/MANIFEST.MF in the path.

Let's say we create such file identical to what already is there in the jar.

Now, we can build an image by running:

pack build example:latest \
  --path .
  --builder paketobuildpacks/builder:base \
  --buildpack gcr.io/paketo-buildpacks/sap-machine \
  --buildpack gcr.io/paketo-buildpacks/syft \
  --buildpack gcr.io/paketo-buildpacks/executable-jar

In this case, the buildpack is participating (because the META-INF/MANIFEST.MF exists) and can find the JAR by searching in the path. So, it can create a docker image in our desired way; an image with the exact jar that we had built before, not exploded or modified in any way.

However, now when we try to run the image we get a Class not found error for the defined entry point in the META-INF/MANIFEST.MF.

To fix this, first, we need to get rid of the requirement for a META-INF/MANIFEST.MF when we have a stand-alone JAR file that can run on its own. The rest of the code already handles the case very well. It is only in the detection phase, when it does not let the buildpack proceed without a manual META-INF/MANIFEST.MF.

With the proposed changes, when there is a stand-alone JAR in the path, the buildpack can find and place that JAR in the image without requiring a separate META-INF/MANIFEST.MF and without requiring to explode the JAR, and the produced image runs perfectly.

sap-ali · 2024-04-04T15:00:28Z

@dmikusa I have gathered the following shell-script that show cases the use-case where the buildpack is not working as expected without the proposed changes:

#!/usr/bin/env bash
gh repo clone jreleaser/helloworld-java-jar
cd helloworld-java-jar
./mvnw verify
pack build helloworld-java:latest \
  --path target/ \
  --builder paketobuildpacks/builder:base \
  --buildpack gcr.io/paketo-buildpacks/sap-machine \
  --buildpack gcr.io/paketo-buildpacks/syft \
  --buildpack gcr.io/paketo-buildpacks/executable-jar &&
docker run --rm helloworld-java:latest
cd -

dmikusa · 2024-04-25T04:11:12Z

Sorry for the delay. I got a chance to take a look at this and what you're doing with the target isn't really something that's supported presently.

pack build helloworld-java:latest \
  --path target/

You're pointing it to a whole folder of stuff and expecting the buildpack to pick through all that. The Java buildpacks expect one of two things: a.) an exploded JAR file that you've compiled in advance b.) Java source code that it can build.

If you take the demo app here and give it either of those things, then it produces a functional app image.

I think what you're proposing here is basically a fix for #132, and if I'm remembering this one correctly it came out of the work with Quarkus. You're hitting it in a slightly different use case, but it amounts to the same thing. You want to be able to set a path with pack build to a folder of one or more JAR files (in your case it's one, in the #132, it's multiple).

How it's implemented really isn't that different, with the exception that your PR doesn't handle the case where other JARs (or resources) in the folder need to be on the classpath. That works in your case, cause you have only the one FAT jar, but I believe Quarkus' "fast-jar" format has the executable JAR plus some other JARs that need to be on the classpath. In that case, this PR wouldn't work.

Would you be interested in submitting code changes to support that use case as well? (i.e. both #132 and #264)

There is a lot of testing and verification that's required for a change like this because it impacts the build plan and there are a lot of different scenarios to cover. It would be helpful if we could do it all at once.

sap-ali · 2024-04-30T07:16:58Z

@dmikusa Thanks for getting back to me. I would be happy to help with the other case, but since I don't have a good overview on all the possible cases that you mentioned, maybe we could somehow work on it together?

For example, you could write down all the tests that you think are necessary, and then I could see how I can implement the required functionality, or alternatively we could also schedule a 1h pairing session. What do you think?

pbusko · 2024-05-13T15:38:32Z

You're pointing it to a whole folder of stuff and expecting the buildpack to pick through all that. The Java buildpacks expect one of two things: a.) an exploded JAR file that you've compiled in advance b.) Java source code that it can build.

I think what you're proposing here is basically a fix for #132, and if I'm remembering this one correctly it came out of the work with Quarkus. You're hitting it in a slightly different use case, but it amounts to the same thing. You want to be able to set a path with pack build to a folder of one or more JAR files (in your case it's one, in the #132, it's multiple).

@dmikusa At the moment there's basically no way to build an image using executable jar and not exploding it. The use-case should be supported, given the following in the readme:

This buildpack will participate if all the following conditions are met:

<APPLICATION_ROOT>/**/*.jar exists and that JAR has a /META-INF/MANIFEST.MF file which contains a Main-Class entry

and

$BP_EXECUTABLE_JAR_LOCATION | An optional glob to specify the JAR used as an entrypoint. Defaults to "", which causes the buildpack to do a breadth-first search for the first executable JAR it finds.

executable-jar/executable/build.go

Lines 83 to 87 in bba2e76

    
           if execJar.ExplodedJAR { 
        
           	arguments = append(arguments, execJar.MainClass) 
        
           } else { 
        
           	arguments = append(arguments, "-jar", execJar.Path) 
        
           }

The following is not working:

$ tree .
.
└── build
    └── runner.jar

2 directories, 1 file

$ pack build --verbose --trust-builder --builder paketobuildpacks/builder-jammy-base -b gcr.io/paketo-buildpacks/bellsoft-liberica -b gcr.io/paketo-buildpacks/syft -b gcr.io/paketo-buildpacks/executable-jar -e 'BP_EXECUTABLE_JAR_LOCATION=build/runner.jar' my-executable-jar

This scenario fails because the executable-jar buildpack completely ignores the BP_EXECUTABLE_JAR_LOCATION during detection and only reads it during build, which I would consider a bug in the current implementation.

dmikusa · 2024-05-13T16:00:20Z

@pbusko Yes, what you're referencing is exactly why #132 is needed. Those settings only apply when building from source code because detect hasn't been updated to support multiple JARs.

To be clear, I'd 100% like to see #132 implemented (I think it addresses #264 as well). This PR needs a little more though, see my previous notes.

pbusko · 2024-05-14T07:19:37Z

How it's implemented really isn't that different, with the exception that your PR doesn't handle the case where other JARs (or resources) in the folder need to be on the classpath. That works in your case, cause you have only the one FAT jar, but I believe Quarkus' "fast-jar" format has the executable JAR plus some other JARs that need to be on the classpath. In that case, this PR wouldn't work.

Do you have an example at hand? I just tried with Quarkus fatjar and the scenario described in #265 (comment) and everything works as expected.

Also the #132 states:

This enhancement request is to have detect also look at the present workspace and see if either a.) the workspace itself has a manifest or if b.) it can identify a single executable JAR file in the workspace.

Which is exactly what this PR does in my opinion.

c0d1ngm0nk3y · 2024-05-14T11:22:00Z

Which is exactly what this PR does in my opinion.

I agree that this pr looks good as it is. Imho it is rather a bugfix than a enhancement. The build logic handled the case well (by adding -jar), but it could never be detected.

Also this supports the most simplest use case - only a single executable jar present.

c0d1ngm0nk3y

Some minor remarks - probably a matter of taste. But lgtm and fixes the bug.

c0d1ngm0nk3y · 2024-05-14T11:29:34Z

executable/detect.go

@@ -65,9 +65,15 @@ func (d Detect) Detect(context libcnb.DetectContext) (libcnb.DetectResult, error
 		return libcnb.DetectResult{}, fmt.Errorf("unable to read manifest in %s\n%w", context.Application.Path, err)
 	}

+	jarGlob, _ := cr.Resolve("BP_EXECUTABLE_JAR_LOCATION")


Could we move those 2 lines into the else? Searching for a jar is only relevant if no "Main-Class" is found yet.

So something like

if m.Get("Main-Class") { //case 1 } else { //search jar if found { //case 2 } }

The same checks are already present in

executable-jar/executable/executable_jar.go

Line 40 in bba2e76

func LoadExecutableJAR(appPath string, executableJarGlob string) (ExecutableJAR, error) {

The detection logic can be minimised down to:

jarGlob, _ := cr.Resolve("BP_EXECUTABLE_JAR_LOCATION") execJar, err := LoadExecutableJAR(context.Application.Path, jarGlob) if err != nil { return libcnb.DetectResult{}, fmt.Errorf("unable to load executable JAR\n%w", err) } if !reflect.DeepEqual(execJar, ExecutableJAR{}) { d.Logger.Info("PASSED: 'Main-Class' manifest attribute found") result.Plans[0].Provides = append(result.Plans[0].Provides, libcnb.BuildPlanProvide{Name: PlanEntryJVMApplicationPackage}) }

Thanks, updated :)

@sap-ali the idea was to replace everything with the suggested snippet, starting from the line 64:

executable-jar/executable/detect.go

Lines 64 to 83 in 4178165

m, err := libjvm.NewManifest(context.Application.Path)

if err != nil {

return libcnb.DetectResult{}, fmt.Errorf("unable to read manifest in %s\n%w", context.Application.Path, err)

}

if _, ok := m.Get("Main-Class"); ok {

d.Logger.Info("PASSED: 'Main-Class' manifest attribute found")

result.Plans[0].Provides = append(result.Plans[0].Provides, libcnb.BuildPlanProvide{Name: PlanEntryJVMApplicationPackage})

} else {

jarGlob, _ := cr.Resolve("BP_EXECUTABLE_JAR_LOCATION")

execJar, err := LoadExecutableJAR(context.Application.Path, jarGlob)

if err != nil {

return libcnb.DetectResult{}, fmt.Errorf("unable to load executable JAR\n%w", err)

}

if !reflect.DeepEqual(execJar, ExecutableJAR{}) {

d.Logger.Info("PASSED: 'Main-Class' manifest attribute found")

result.Plans[0].Provides = append(result.Plans[0].Provides, libcnb.BuildPlanProvide{Name: PlanEntryJVMApplicationPackage})

}

}

@pbusko Got you, thanks; updated.

executable/detect.go

c0d1ngm0nk3y

@sap-ali Could you have a look at this comment - should be a minor change.

c0d1ngm0nk3y · 2024-05-15T08:46:03Z

@paketo-buildpacks/java-maintainers This fixes the use case that gives the buildpack its name (#264). This does not yet fix #132 , but imho it is a valuable improvement over the current state. Or do you see something missing?

loewenstein · 2024-05-21T14:10:38Z

@paketo-buildpacks/java-maintainers This fixes the use case that gives the buildpack its name (#264). This does not yet fix #132 , but imho it is a valuable improvement over the current state. Or do you see something missing?

@paketo-buildpacks/java-maintainers Would you mind to have a look?

loewenstein · 2024-05-27T16:10:07Z

@anthonydahanne & @pivotal-david-osullivan what's your take on this? I'd be much in favor of getting this merged as is. It's fixing a bug and that should not be coupled to feature development like #132 imho.

anthonydahanne · 2024-05-31T14:13:51Z

what's your take on this? I'd be much in favor of getting this merged as is. It's fixing a bug and that should not be coupled to feature development like #132 imho.

Hello!
merging this contribution would require to run a decent number of tests; such as all the Java samples; and make sure the detection and plan are working properly for all use cases.
I'd very much prefer to perform this effort after #132 is also fixed / included in the fix so that we squash 2 related issues during 1 big regression testing session

c0d1ngm0nk3y · 2024-06-06T06:29:05Z

How can we continue with that? @dmikusa Could you give some details on the buildplan changes you see critical? Actually, there is no change in the buildplan at all, right, "Only" one additional case is detected. And fwiw, the case that gives the buildpack its name (it is not called "exploded executable JAR").

For me, this is a clear bugfix, since the behavior was already documented and even implemented in build. There was a bug in the detect. I think when we do not feel comfortable in merging bugfixes since they might have side effects, it is a clear indication that some tests are missing and I would like to better understand what is seen as critical here.

Imho, this is a good contribution to enable an additional use case.

pbusko · 2024-06-19T11:40:26Z

The integration tests from https://github.com/paketo-buildpacks/java passed successfully with the change from this PR.
@dmikusa is there anything else blocking this change from being merged?

loewenstein · 2024-06-19T15:08:15Z

cc @paketo-buildpacks/java-maintainers

loewenstein-sap · 2024-06-25T06:29:36Z

@anthonydahanne WDYT, ready to merge? If not, what do you think is still missing?

loewenstein-sap · 2024-07-03T06:18:07Z

@paketo-buildpacks/java-maintainers If I recall correctly, the only concern left with this was its effect on the build plan and the lack of coverage in tests for the kind of problems that can occur in case of changes to the build plan. With paketo-buildpacks/java#1399 being merged, is there anything left to do or could we get this approved and merged?

loewenstein-sap · 2024-07-11T09:42:07Z

@paketo-buildpacks/java-maintainers If I recall correctly, the only concern left with this was its effect on the build plan and the lack of coverage in tests for the kind of problems that can occur in case of changes to the build plan. With paketo-buildpacks/java#1399 being merged, is there anything left to do or could we get this approved and merged?

@paketo-buildpacks/java-maintainers Would be nice to get a comment on that. Can we merge it or what is left to do before we can?

anthonydahanne

tests pass, code is now similar to what build.go does, which I believe is a plus.

It will detect in more cases, not less, which should not deteriorate plans involving executable-jar and other BPs

anthonydahanne · 2024-07-17T20:49:11Z

rebasing on top of main

anthonydahanne · 2024-07-17T21:10:17Z

thanks for your patience all! detection of executable jar is very sensitive, hopefully everything will be smooth and it will expand use cases!

sap-ali requested a review from a team as a code owner April 3, 2024 13:42

sap-ali mentioned this pull request Apr 3, 2024

Packing a single executable fat-Jar does not work #264

Closed

sap-ali changed the title ~~Allow making an executable-jar image by only providing an executable JAR~~ Allow making an executable-jar image by only providing an executable JAR [Fixes #264] Apr 3, 2024

sap-ali changed the title ~~Allow making an executable-jar image by only providing an executable JAR [Fixes #264]~~ Allow making an executable-jar image by only providing an executable JAR Apr 3, 2024

dmikusa added type:enhancement A general enhancement semver:minor A change requiring a minor version bump labels Apr 25, 2024

c0d1ngm0nk3y self-requested a review May 14, 2024 11:18

c0d1ngm0nk3y approved these changes May 14, 2024

View reviewed changes

c0d1ngm0nk3y mentioned this pull request May 15, 2024

Enable detect to support multiple jar files #132

Open

c0d1ngm0nk3y requested changes May 15, 2024

View reviewed changes

c0d1ngm0nk3y approved these changes May 17, 2024

View reviewed changes

loewenstein-sap mentioned this pull request Jun 4, 2024

Add initial set of integration tests paketo-buildpacks/java#1399

Merged

5 tasks

sap-ali force-pushed the allow-single-jar branch from 3e64d9e to 2e46fd0 Compare June 19, 2024 07:23

anthonydahanne approved these changes Jul 17, 2024

View reviewed changes

sap-ali added 3 commits July 17, 2024 16:49

Allow making an executable-jar image by only providing an executable JAR

824b6a9

Refactor and remove duplicate logic

b1ab799

Simplify logic

40ed839

anthonydahanne force-pushed the allow-single-jar branch from 2e46fd0 to 40ed839 Compare July 17, 2024 20:49

anthonydahanne merged commit e2e9876 into paketo-buildpacks:main Jul 17, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow making an executable-jar image by only providing an executable JAR #265

Allow making an executable-jar image by only providing an executable JAR #265

sap-ali commented Apr 3, 2024 •

edited

Loading

linux-foundation-easycla bot commented Apr 3, 2024 •

edited

Loading

dmikusa commented Apr 3, 2024

sap-ali commented Apr 3, 2024

sap-ali commented Apr 4, 2024

dmikusa commented Apr 25, 2024

sap-ali commented Apr 30, 2024

pbusko commented May 13, 2024 •

edited

Loading

dmikusa commented May 13, 2024

pbusko commented May 14, 2024

c0d1ngm0nk3y commented May 14, 2024

c0d1ngm0nk3y left a comment

c0d1ngm0nk3y May 14, 2024

pbusko May 14, 2024 •

edited

Loading

sap-ali May 17, 2024

pbusko May 17, 2024 •

edited

Loading

sap-ali May 17, 2024

c0d1ngm0nk3y left a comment

c0d1ngm0nk3y commented May 15, 2024

loewenstein commented May 21, 2024

loewenstein commented May 27, 2024

anthonydahanne commented May 31, 2024

c0d1ngm0nk3y commented Jun 6, 2024

pbusko commented Jun 19, 2024 •

edited

Loading

loewenstein commented Jun 19, 2024

loewenstein-sap commented Jun 25, 2024

loewenstein-sap commented Jul 3, 2024

loewenstein-sap commented Jul 11, 2024

anthonydahanne left a comment

anthonydahanne commented Jul 17, 2024

anthonydahanne commented Jul 17, 2024

	m, err := libjvm.NewManifest(context.Application.Path)
	if err != nil {
	return libcnb.DetectResult{}, fmt.Errorf("unable to read manifest in %s\n%w", context.Application.Path, err)
	}

	if _, ok := m.Get("Main-Class"); ok {
	d.Logger.Info("PASSED: 'Main-Class' manifest attribute found")
	result.Plans[0].Provides = append(result.Plans[0].Provides, libcnb.BuildPlanProvide{Name: PlanEntryJVMApplicationPackage})
	} else {
	jarGlob, _ := cr.Resolve("BP_EXECUTABLE_JAR_LOCATION")
	execJar, err := LoadExecutableJAR(context.Application.Path, jarGlob)
	if err != nil {
	return libcnb.DetectResult{}, fmt.Errorf("unable to load executable JAR\n%w", err)
	}

	if !reflect.DeepEqual(execJar, ExecutableJAR{}) {
	d.Logger.Info("PASSED: 'Main-Class' manifest attribute found")
	result.Plans[0].Provides = append(result.Plans[0].Provides, libcnb.BuildPlanProvide{Name: PlanEntryJVMApplicationPackage})
	}
	}

Allow making an executable-jar image by only providing an executable JAR #265

Allow making an executable-jar image by only providing an executable JAR #265

Conversation

sap-ali commented Apr 3, 2024 • edited Loading

linux-foundation-easycla bot commented Apr 3, 2024 • edited Loading

dmikusa commented Apr 3, 2024

sap-ali commented Apr 3, 2024

sap-ali commented Apr 4, 2024

dmikusa commented Apr 25, 2024

sap-ali commented Apr 30, 2024

pbusko commented May 13, 2024 • edited Loading

dmikusa commented May 13, 2024

pbusko commented May 14, 2024

c0d1ngm0nk3y commented May 14, 2024

c0d1ngm0nk3y left a comment

Choose a reason for hiding this comment

c0d1ngm0nk3y May 14, 2024

Choose a reason for hiding this comment

pbusko May 14, 2024 • edited Loading

Choose a reason for hiding this comment

sap-ali May 17, 2024

Choose a reason for hiding this comment

pbusko May 17, 2024 • edited Loading

Choose a reason for hiding this comment

sap-ali May 17, 2024

Choose a reason for hiding this comment

c0d1ngm0nk3y left a comment

Choose a reason for hiding this comment

c0d1ngm0nk3y commented May 15, 2024

loewenstein commented May 21, 2024

loewenstein commented May 27, 2024

anthonydahanne commented May 31, 2024

c0d1ngm0nk3y commented Jun 6, 2024

pbusko commented Jun 19, 2024 • edited Loading

loewenstein commented Jun 19, 2024

loewenstein-sap commented Jun 25, 2024

loewenstein-sap commented Jul 3, 2024

loewenstein-sap commented Jul 11, 2024

anthonydahanne left a comment

Choose a reason for hiding this comment

anthonydahanne commented Jul 17, 2024

anthonydahanne commented Jul 17, 2024

sap-ali commented Apr 3, 2024 •

edited

Loading

linux-foundation-easycla bot commented Apr 3, 2024 •

edited

Loading

pbusko commented May 13, 2024 •

edited

Loading

pbusko May 14, 2024 •

edited

Loading

pbusko May 17, 2024 •

edited

Loading

pbusko commented Jun 19, 2024 •

edited

Loading