Build jar in Augment phase #3338

stuartwdouglas · 2019-07-26T03:56:29Z

Initial work on this feature, this PR is for early review to make sure that people are happy with this approach.

The basic idea is that as much logic as possible is moved from the maven/gradle plugins and the 'creator' project, and instead it all happens inside the augment process via build steps. This will have quite a few benefits, including allowing extensions to define their own packaging formats (e.g. AWS lambda could create a zip with all required metadata and the uberjar).

This will also mean that a lot of config that is currently in the plugins will move to application.properties (and some kinda of compatibility strategy will be needed for migration), as well as requiring docs and quickstarts to be updated.

dmlloyd · 2019-07-26T12:21:09Z

core/deployment/src/main/java/io/quarkus/runner/RuntimeRunner.java

@@ -115,6 +116,12 @@ public void run() {

                transformerTarget.setTransformers(functions);
            }
+            for (GeneratedClassBuildItem i : result.consumeMulti(GeneratedClassBuildItem.class)) {


Maybe these should be in a build step too?

What do you mean? I think generated classes are a reasonable output from the runtime runners point of view, I don't think trying to stuff them into a class loader as part of a build step feels right.

Previously they just wrote to the class output, which was a build item. If class output were still a build item, these could be in steps and duplication removed.

ClassOutputBuildItem violated the BuildItem contract though. It was a build item that was consumed to produce output. After the initial set of changes the build step that produced the main class ended up being ignored, as it was only consuming ClassOutputBuildItem and not producing something the jar build steps expected.

If something is used to produce output it needs to be a BuildProducer

But it already existed, which is a big plus. :)

Really having something that is a build item that "catches" output isn't all that bad - it's essentially similar to a build item that specifies where to put something; it's just a bit more active about it. So in this case the "output" is the fact that the classes were produced, which can be represented by a virtual build item.

But it's broken, it only worked by accident because it was consumed by things that were also produced things necessary for the build.

You can make it work by always consuming it in a build step that also produces something useful, but that is pretty hacky.

With no outputs it may have worked on accident in the past, but by wiring it in to a virtual build output it stops being hacky IMO. But it's not super important to resolve now, I was just thinking of a way to encapsulate things a little better.

dmlloyd · 2019-07-26T12:24:59Z

core/deployment/src/main/java/io/quarkus/deployment/pkg/builditem/CurateOutcomeBuildItem.java

+    private final AppModel effectiveModel;
+    private final AppModelResolver resolver;
+
+    public CurateOutcomeBuildItem(AppModel effectiveModel, AppModelResolver resolver) {


It would be nice if we could have actual inputs for each element of the class path, similar to the application archives thing but for the whole class path, where you can tell whether it's a JAR or filesystem path.

I hacked up something like this for the native image extension experiment. It's not really worth copying but it validates the idea a bit.

This could definitely be improved, but I think we can do the initial change without it.

dmlloyd · 2019-07-26T12:26:01Z

core/deployment/src/main/java/io/quarkus/deployment/pkg/PackageConfig.java

+import io.quarkus.runtime.annotations.ConfigRoot;
+
+@ConfigRoot
+public class PackageConfig {


+1 to the idea of putting more of the build config into config objects from the Mojo.

dmlloyd · 2019-07-26T12:26:22Z

core/deployment/src/main/java/io/quarkus/deployment/pkg/builditem/OutputTargetBuildItem.java

+ *
+ * TODO: should we just create them in temp directories, and leave it up to the integration to move them where they want?
+ */
+public final class OutputTargetBuildItem extends SimpleBuildItem {


Maybe this should be config as well?

The output directory? That's generally not something the user wants to manually configure as tools have their own standard locations? Or do you mean something else?

I think it makes sense not from the perspective of a user trying to manually configure things, but from the perspective that all build steps should be configured in a consistent way.
Sometimes a given build system might have its own configuration items which overlap with Quarkus's, but in these cases it just means the build system can provide default values for these items. It may transpire that there are some items which are configurable in build system A that are not configurable in build system B.

And, if you make it a configuration item, then the build systems can "stuff" it with a default as needed like we already do for application name and version, which is perhaps easier than producing a bunch of initial items which are in the end just configuration.

Configuration is more of an end user thing though that a build system thing. Having what are basically internals exposed as user config does not seem like a great option from a user experience point of view.

Perhaps for the case when we launch the build/provisioning outside of Maven/Gradle context. This is what creator is supporting currently, where you can configure a general work/output dir and then per phase, if necessary.

On the contrary, it's irritating to users when they can't adjust a knob they want to adjust. It's only "internals" when you look at the build system + augmentation combination as a single closed box; if you look at them as two separate boxes it makes a lot more sense. As we've said from day one, it doesn't make sense to be married into a particular build system. But all this strong coupling really prevents us from being truly portable across build systems.

I think the stronger principle is to unify all configuration as configuration. Let the build system provide all the defaults that make sense for that particular system, and don't make any assumptions about what the build system might provide.

I will leave this for now, and revisit it once the main functionality is done (unless I run into similar issues elsewhere).

One other use case to think about is how we generate the example config files, which is done by running a minimal build. The task needs to know the location to create the example file, which IMHO should be a build item as it is a task specific configuration piece rather than general quarkus config.

I guess the more general class of this problem is the idea that if there are configuration options that only apply to certain systems then these will still be cluttering up the config namespace of other build systems.

dmlloyd · 2019-07-26T12:36:33Z

core/deployment/src/main/java/io/quarkus/deployment/pkg/PackageConfig.java

+     * If an uberjar should be produced as a result of this build
+     */
+    @ConfigItem
+    public Optional<Boolean> uberJar;


These normally should just be boolean with a defaultValue="false" probably.

So my thoughts on this were that there is no real reason why you could not generate both.

So:

nothing defined == thin jar
one set to true == the specified one
both set to true == we generate both

In addition there may be other cases where one is forced to be generated because a later build step requires a specific output format.

What I like in the current implementation is that the augmentation is separate from the packaging.

OK. I wonder if that's a little confusing though... maybe a List<BuiltInPackagingType> where that is an enum with e.g. THIN_JAR, UBER_JAR etc., with a default value of thin-jar?

JNI is another thing that is similar - you can force it on, otherwise it's enabled when an extension needs it. I chose to word it like this: "Add code to allow JNI libraries to be linked in to the image. If an extension is included which requires JNI, this option will be ignored and assumed to be true." So maybe we could just word it like "Specify the basic packaging types to create. Extensions may cause these to be produced even if not specified here" or something like that?

That is really limiting though, it means that extensions that require different packaging formats can't really do anything. The main use case we have for this at the moment is Serverless, where cloud providers expect a zip file in a specific format. With this change it will be possible for say the AWS Lambda extension to consume an uberjar (or native image when this patch is complete) and build a .zip file with all metadata ready to upload.

Also for dev and test mode these still will not be generated, these are only generated if the environment asks for them. It also allows us to configure everything though our standard config properties, rather than different properties in the plugins.

They'd still be able to consume an uberjar; the only thing I'm suggesting is to change the representation in configuration to be less magical (one set of enums instead of a pair of tri-state booleans). You list the built-in formats that you want to produce; the build will definitely produce those, but might also produce additional ones if their corresponding build items are consumed.

If the default value varies depending on the execution mode, then the default value should be fed in as input IMO. Though it's also possible to treat empty as a special value meaning "auto detect", which would probably give a similar effect.

Regardless, extensions will still want to have their own switches to control their own output formats anyway, outside of this configuration.

That was in response to Alexey, I am fine with the config change.

dmlloyd · 2019-07-26T12:42:09Z

core/deployment/src/main/java/io/quarkus/deployment/pkg/builditem/ThinJarBuildItem.java

+
+import io.quarkus.builder.item.SimpleBuildItem;
+
+public final class ThinJarBuildItem extends SimpleBuildItem {


In the native image extension PoC I used a virtual build item to represent the build target (I called it io.quarkus.deployment.builditem.AugmentPhaseBuildItem). Do we need the information in these build items, given that the information would probably normally be in the configuration too?

That sounds like ArtifactResultBuildItem, which is a general output. Other build steps might want to work with a specific format though, so they can choose to consume the specific build item they need and force it to be generated

Yeah I agree with the intermediate step. I was just thinking that it might be better to have a general augment phase build result which is virtual, because that way the build system integration would only declare one final item. The individual steps would be able to decide what gets included based on config and/or launch mode.

dmlloyd · 2019-07-26T12:43:13Z

core/deployment/src/main/java/io/quarkus/deployment/pkg/steps/JarResultBuildStep.java

+ *
+ * However we still need an extension to be able to ask for a specify one of these despite the config,
+ * e.g. if a serverless environment needs an uberjar to build its deployment package then we need
+ * to be able to provide this.


I didn't think of that. Having the intermediate step does seem to make sense.

aloubyansky · 2019-08-15T15:06:48Z

Here are a couple of fixes for the creator tests

diff --git a/core/creator/src/main/java/io/quarkus/creator/CuratedApplicationCreator.java b/core/creator/src/main/java/io/quarkus/creator/CuratedApplicationCreator.java
index 379bbba42..fc483e738 100644
--- a/core/creator/src/main/java/io/quarkus/creator/CuratedApplicationCreator.java
+++ b/core/creator/src/main/java/io/quarkus/creator/CuratedApplicationCreator.java
@@ -242,6 +242,7 @@ public class CuratedApplicationCreator implements AutoCloseable {
         public Builder setAppAtrifact(Path path) throws AppCreatorException {
             try {
                 this.appArtifact = ModelUtils.resolveAppArtifact(path);
+                this.appArtifact.setPath(path);
             } catch (IOException e) {
                 throw new AppCreatorException("Unable to resolve app artifact " + path);
             }
diff --git a/core/creator/src/test/java/io/quarkus/creator/phase/runnerjar/test/CreatorOutcomeTestBase.java b/core/creator/src/test/java/io/quarkus/creator/phase/runnerjar/test/CreatorOutcomeTestBase.java
index c75f12280..c51bb3dc6 100644
--- a/core/creator/src/test/java/io/quarkus/creator/phase/runnerjar/test/CreatorOutcomeTestBase.java
+++ b/core/creator/src/test/java/io/quarkus/creator/phase/runnerjar/test/CreatorOutcomeTestBase.java
@@ -22,7 +22,7 @@ public abstract class CreatorOutcomeTestBase extends ResolverSetupCleanup {
         final CuratedApplicationCreator.Builder appCreationContext = CuratedApplicationCreator.builder()
                 .setModelResolver(resolver)
                 .setWorkDir(workDir)
-                .setAppArtifact(appJar.toAppArtifact());
+                .setAppAtrifact(resolver.resolve(appJar.toAppArtifact()));
         initProps(appCreationContext);
         testCreator(appCreationContext.build());

The fundamental issue though is that the augmentor won't perform any build steps, given that these tests are using abstract dependency trees (no classes) and using the bootstrap to resolve the effective app model (runtime + deployment deps) and the lib content purely in terms of the artifacts.

gsmet

Not very useful review but I spotted a typo :).

gsmet · 2019-10-08T12:45:57Z

core/creator/src/main/java/io/quarkus/creator/CuratedApplicationCreator.java

+            return this;
+        }
+
+        public Builder setAppAtrifact(Path path) throws AppCreatorException {


s/setAppAtrifact/setAppArtifact/

aloubyansky · 2019-10-09T16:35:34Z

bom/runtime/pom.xml

@@ -164,6 +164,9 @@
        <antlr.version>4.7.2</antlr.version>
        <quarkus-security.version>1.0.0.Alpha2</quarkus-security.version>
        <javax.interceptor-api.version>1.2</javax.interceptor-api.version>
+        <findbugs.version>3.0.2</findbugs.version>
+        <jsoup.version>1.11.3</jsoup.version>
+        <wagon-provider-api.version>3.0.0</wagon-provider-api.version>


Are these the versions we want to enforce in user apps?

There is not really anything we can do about this now without completely re-structuring our build to have a proper deployment and runtime split in the boms. This is no different to what we are doing elsewhere so I think it is fine for now.

How about moving them directly to the dependency management section of build-parent/pom.xml? If that works, that's where they should be for now.

aloubyansky · 2019-10-09T17:05:00Z

core/deployment/src/main/java/io/quarkus/deployment/pkg/steps/JarResultBuildStep.java

+    /**
+     * Native images are built from a specially created jar file. This allows for changes in how the jar file is generated.
+     *
+     * At present this is the same as the thin jar


Until they get different, I'd prefer to have a single method that builds the JAR and returns its path that could be used to resolve the lib dir as its sibling.

That comment is outdated, they are different. The standard runner jar should not have the native image feature generated.

The differences are the file name suffix, the logged message and the list of the generated classes, which is passed as an argument to the method.

List<GeneratedClassBuildItem> allClasses = new ArrayList<>(generatedClasses); allClasses.addAll(substrateResources.stream() .map((s) -> new GeneratedClassBuildItem(true, s.getName(), s.getClassData())).collect(Collectors.toList()));

The logic though is identical, isn't it?

This allows extensions much greater control over the output, including the ability to define new output formats. There are some breaking changes introduced by this commit. Namely: - The 'thin jar' now resides in its own folder, rather than in the target directory - Native image is now produced from a dedicated native jar file, rather than by running native-image on an existing jar - Most Maven/Gradle config has been moved to application.properties. This is still supported for now, but is deprecated - The 'native-image' mojo has been depricated

gsmet · 2019-10-21T08:41:32Z

I rebased and force pushed to get a clean build.

gsmet · 2019-10-21T11:01:18Z

Granted it's a bit risky to push it but I prefer we have some more feedback on it than releasing it next week.

stuartwdouglas requested review from dmlloyd and aloubyansky July 26, 2019 03:56

stuartwdouglas force-pushed the build-jar-in-augment branch from ac6e9b0 to 8925222 Compare July 26, 2019 05:30

dmlloyd reviewed Jul 26, 2019

View reviewed changes

stuartwdouglas force-pushed the build-jar-in-augment branch from 8925222 to aa95f04 Compare July 27, 2019 02:43

gsmet added the coding-in-progress label Aug 1, 2019

stuartwdouglas force-pushed the build-jar-in-augment branch from aa95f04 to 32df3b9 Compare August 15, 2019 03:51

stuartwdouglas force-pushed the build-jar-in-augment branch 4 times, most recently from b9b8ed5 to a665248 Compare August 16, 2019 09:36

stuartwdouglas force-pushed the build-jar-in-augment branch 3 times, most recently from 7a61846 to 5c4f62c Compare August 28, 2019 23:56

stuartwdouglas marked this pull request as ready for review August 29, 2019 01:04

stuartwdouglas removed the coding-in-progress label Aug 29, 2019

stuartwdouglas changed the title ~~WIP: build jar in Augment phase~~ Build jar in Augment phase Aug 29, 2019

stuartwdouglas force-pushed the build-jar-in-augment branch 6 times, most recently from 01fff9e to ba96e5d Compare September 4, 2019 07:05

stuartwdouglas mentioned this pull request Sep 10, 2019

RESTEasy SAM support #3180

Closed

stuartwdouglas force-pushed the build-jar-in-augment branch from 025d1a9 to eab18b0 Compare September 10, 2019 01:20

dmlloyd added the triage/needs-rebase This PR needs to be rebased first because it has merge conflicts label Sep 20, 2019

stuartwdouglas force-pushed the build-jar-in-augment branch from eab18b0 to eeb53d3 Compare September 20, 2019 03:21

stuartwdouglas removed the triage/needs-rebase This PR needs to be rebased first because it has merge conflicts label Sep 20, 2019

stuartwdouglas force-pushed the build-jar-in-augment branch 3 times, most recently from aa9944a to 9013023 Compare September 24, 2019 14:17

stuartwdouglas modified the milestones: 0.24.0, 0.25.0 Oct 2, 2019

stuartwdouglas force-pushed the build-jar-in-augment branch 4 times, most recently from 5c42d32 to fdb668c Compare October 8, 2019 02:44

gsmet requested changes Oct 8, 2019

View reviewed changes

aloubyansky reviewed Oct 9, 2019

View reviewed changes

stuartwdouglas force-pushed the build-jar-in-augment branch 2 times, most recently from d2a889a to 7e119b3 Compare October 14, 2019 13:19

cescoffier modified the milestones: 0.25.0, 0.26.0 Oct 15, 2019

stuartwdouglas force-pushed the build-jar-in-augment branch from 7e119b3 to 40304a8 Compare October 21, 2019 03:10

aloubyansky approved these changes Oct 21, 2019

View reviewed changes

aloubyansky requested a review from gsmet October 21, 2019 06:57

gsmet force-pushed the build-jar-in-augment branch from 40304a8 to 0d3335d Compare October 21, 2019 08:41

gsmet approved these changes Oct 21, 2019

View reviewed changes

gsmet merged commit 7003043 into quarkusio:master Oct 21, 2019


		import io.quarkus.builder.item.SimpleBuildItem;

		public final class ThinJarBuildItem extends SimpleBuildItem {

Build jar in Augment phase #3338

Build jar in Augment phase #3338

Conversation

stuartwdouglas commented Jul 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aloubyansky commented Aug 15, 2019

gsmet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aloubyansky Oct 10, 2019 • edited Loading

Choose a reason for hiding this comment

gsmet commented Oct 21, 2019

gsmet commented Oct 21, 2019

aloubyansky Oct 10, 2019 •

edited

Loading