Introduce BPI_TOMCAT_ADDITIONAL_JARS to enable adding JARs to CLASSPATH

[c0d1ngm0nk3y]

Summary

Allow BPI_TOMCAT_ADDITIONAL_JARS to contain jars that will be added to the classpath via setenv.sh.

see paketo-buildpacks/apache-tomee#200

Use Cases

This allows another buildpack(s) to contribute a jar that needs to be added to the tomcat classloader. The buildpack can contribute its dependency and add the location to BPI_TOMCAT_ADDITIONAL_JARS. This can be done by several buildpacks independently without caring for one big external configuration that might contain jars you do not need.

Checklist

• I have viewed, signed, and submitted the Contributor License Agreement.
• I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
• I have added an integration test, if necessary.
• I have reviewed the styleguide for guidance on my code quality.
• I'm happy with the commit history on this PR (I have rebased/squashed as needed).

[dmikusa]

Do you have some details on the use case that this satisfies? Also, how would one get a JAR file into the build to use this functionality?

I think that historically the path to take if you want to add JARs is to use the external configuration mechanism. That allows you to overlay things on top of the Tomcat install.

If I'm being honest I've never cared for the external configuration feature but it serves a purpose. Is there a desire to replace that functionality?

Thanks

[c0d1ngm0nk3y]

Do you have some details on the use case that this satisfies?

Agreed, the description in the pull request is a bit short. The idea is a buildpack that adds a certain jar to the classpath if needed. Potentially, there could be several buildpacks doing that.

Also, how would one get a JAR file into the build to use this functionality?

Another buildpack can write a layer and point to that layer. It is a normal dependency. It is not meant for the user to set this environment variable. Therefor, I didn't add it to the configuration and used BPI_ and not BP_. But maybe I mixed up something.

I think that historically the path to take if you want to add JARs is to use the external configuration mechanism. That allows you to overlay things on top of the Tomcat install.

The external configuration can switch the configuration, but not modify an existing one. So if you have 2 buildpacks that want to contribute to the buildpack, external configuration can't be used.

If I'm being honest I've never cared for the external configuration feature but it serves a purpose. Is there a desire to replace that functionality?

We discussed this internally quite a bit. This would be a simple and easy way to have several buildpacks adding something to the classpath. If we want something similar with configuration, we would have to modify the external configuration feature in its current form or rather "substitute" it with a more granular approach.
But currently, we only want to add jars, but in different buildpacks.

[c0d1ngm0nk3y]

@anthonydahanne I think all requested changes were adressed.

@dmikusa Is the use case clear now?

[loewenstein]

I agree that this is a much easier way to add to the classpath that external configuration it. Actually, one could even argue that external configuration might never have been intended to allow adding JAR files in the first place.

In particular, external configuration allows only a single contributor.

[dmikusa]

1. Can you update your PR please?

2. Is the intent of this env variable to only allow an individual JAR to be added? should it work to add multiple JARS? if so what would the contents of the env variable look like? would it also be allowed to specify a folder full of class files? Do we care about the difference?

3. Have you tried just modifying the CLASSPATH env directly? The buildpack spec has a method of appending to env variables, so one buildpack could set CLASSPATH and instruct the lifecycle to append that value to CLASSPATH. I'm not 100% sure that would work with the way we write setenv.sh, but we might be able to just adjust that to be CLASSPATH=layer/bin/artifact-name.jar,$CLASSPATH or something like that. Then it would pull from the env variable that's set by buildpacks. Just thinking that if we can avoid introducing a new variable, that would be preferred.

[modulo11]

Using buildpack means to modify the CLASSPATH directly does not work b/c it is controlled/ignored by Tomcat's catalina.sh.

[c0d1ngm0nk3y]

• Can you update your PR please?

Done.

• Is the intent of this env variable to only allow an individual JAR to be added? should it work to add multiple JARS? if so what would the contents of the env variable look like? would it also be allowed to specify a folder full of class files? Do we care about the difference?

It could contain one or many jars. Whatever should be added to the classpath.

• Have you tried just modifying the CLASSPATH env directly? The buildpack spec has a method of appending to env variables, so one buildpack could set CLASSPATH and instruct the lifecycle to append that value to CLASSPATH. I'm not 100% sure that would work with the way we write setenv.sh, but we might be able to just adjust that to be CLASSPATH=layer/bin/artifact-name.jar,$CLASSPATH or something like that. Then it would pull from the env variable that's set by buildpacks.

Currently that does not work (see comment), therefor this pr.

Just thinking that if we can avoid introducing a new variable, that would be preferred.

tbh, I don't really see the problem if it supports a valid usecase straight forward.

[dmikusa]

Using buildpack means to modify the CLASSPATH directly does not work b/c it is controlled/ignored by Tomcat's catalina.sh.

Right. The standard Tomcat behavior.

I was probably not clear enough in my idea. The idea is to have the buildpack overlap and also use CLASSPATH. That way it's usage is clear, because anyone familiar with Java knows of CLASSPATH.

Ex:

	b.Logger.Bodyf("Writing %s/bin/setenv.sh", layer.Path)

	s := fmt.Sprintf(`CLASSPATH="%s"`, file)
	if additionalJarsToClasspath, ok := os.LookupEnv("CLASSPATH"); ok {
		s = fmt.Sprintf(`CLASSPATH="%s:%s"`, file, additionalJarsToClasspath)
	}
	b.Logger.Bodyf("Setting %s", s)

	file = filepath.Join(layer.Path, "bin", "setenv.sh")
	if err = os.WriteFile(file, []byte(s), 0755); err != nil {
		return fmt.Errorf("unable to write file %s\n%w", file, err)
	}

Instead of making a new env variable someone needs to know about, and BPI_ env variables are hard to document by nature, we just use CLASSPATH. Other buildpack authors can use all of the standard buildpack env variable handling, like default, overwrite, append, etc... to handle this env variable.

We get around the Tomcat behavior because we're taking the value of CLASSPATH from build time and writing that into bin/setenv.sh. Same thing we were doing with the other env variable, but just calling it CLASSPATH.

tbh, I don't really see the problem if it supports a valid usecase straight forward.

When we add new settings, we try to keep a few things in mind.

1. The more settings we add the more complexity there is in utilizing the buildpacks. You don't need all of the settings all of the time, but there are still more settings one has to consider when trying to figure out how to make something work. Adding two settings that do close to the same thing, makes things more difficult. I don't think this is the case here, just including it to share some background on how we reason about this.

2. We always prefer native settings as opposed to buildpack-specific settings. This allows users to retain skills from their native toolset. It also keeps the number of custom env variables lower. This is a good example of using the native way, everyone in Java knows about CLASSPATH, so what it's doing here will immediately make sense.

[modulo11]

BPI_TOMCAT_ADDITIONAL_JARS is envisioned as a contract between buildpacks not between buildpacks and the user.

A real-world example would be a buildpack which supplies a database driver and some Tomcat Datasource to be used in an application. Other examples might be JARs that need special configuration (done conveniently by a buildpack) or custom valves which must reside in Tomcat's lib folder and just cannot be shipped with the application.

The idea is that buildpacks write their JARs into a layer and then use layer.LaunchEnvironment.Append to write the correct location into BPI_TOMCAT_ADDITIONAL_JARS. This can be done by multiple buildpacks and will be "collected" once in the setenv.sh script.

Overall the name of the environment variable is not that important, but I think that using CLASSPATH might lead to unnecessary confusion as expectations might differ from how we use it inside the buildpack.

[dmikusa]

BPI_TOMCAT_ADDITIONAL_JARS is envisioned as a contract between buildpacks not between buildpacks and the user.

✅ but just remember that it's not actually restricted to being between buildpacks. A user could absolutely set that env variable.

Overall the name of the environment variable is not that important, but I think that using CLASSPATH might lead to unnecessary confusion as expectations might differ from how we use it inside the buildpack.

I'm not sold on the name, but I do agree that it isn't ultimately that important. If you feel strongly and want to keep it BPI_TOMCAT_ADDITIONAL_JARS, let's go ahead so we can get this feature rolled out. Please just add something into the README.md so that the env variable is documented, for other buildpack authors.