Skip to content
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.

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

Change to only allow a single output type #4780

Merged
merged 1 commit into from
Oct 29, 2019

Conversation

stuartwdouglas
Copy link
Member

No description provided.

@stuartwdouglas stuartwdouglas force-pushed the single-build-target branch 3 times, most recently from 4c99637 to 057b86d Compare October 23, 2019 05:30
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

I think this is mostly good to go but here are a few fixes and suggestions.

}

@BuildStep
void verify(List<PackageTypeBuildItem> items, PackageConfig config) {
ServiceStartBuildItem verify(List<PackageTypeBuildItem> items, PackageConfig config) {
Copy link
Member

Choose a reason for hiding this comment

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

Why do you return a null ServiceStartBuildItem? To force the ordering?

Copy link
Member Author

Choose a reason for hiding this comment

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

So it is run at all

</build>
<properties>
<quarkus.package.type>native</quarkus.package.type>
</properties>
Copy link
Member

Choose a reason for hiding this comment

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

OK, we will need to also change all the quickstarts.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the current way still works, so existing projects will work as normal.

</profile>
</profiles>
----

[TIP]
====
You can provide additional options for `native-image` command using `<additionalBuildArgs>foo,bar</additionalBuildArgs>`
configuration element. Provided options have to be `,` separated.
You can provide additional options for `native-image` command using `quarkus.native.additional-buildA-args=foo,bar` in `application.properties`. Provided options have to be `,` separated.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
You can provide additional options for `native-image` command using `quarkus.native.additional-buildA-args=foo,bar` in `application.properties`. Provided options have to be `,` separated.
You can provide additional options for `native-image` command using `quarkus.native.additional-build-args=foo,bar` in `application.properties`. Provided options have to be `,` separated.

</profile>
</profiles>
----

[TIP]
====
You can provide additional options for `native-image` command using `<additionalBuildArgs>foo,bar</additionalBuildArgs>`
configuration element. Provided options have to be `,` separated.
You can provide additional options for `native-image` command using `quarkus.native.additional-buildA-args=foo,bar` in `application.properties`. Provided options have to be `,` separated.
====
Copy link
Member

Choose a reason for hiding this comment

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

We will need some serious work on a migration guide to have people moving their native-image config from the pom to the application.properties.

Copy link
Member Author

Choose a reason for hiding this comment

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

The pom.xml way still works, and all the properties have the same name. We should definitely document this, but it is not something that should cause any breakage.

====

We use a profile because, you will see very soon, packaging the native executable takes a _few_ seconds.
We use a profile because, you will see very soon, packaging the native executable takes a _few_ seconds. You could
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
We use a profile because, you will see very soon, packaging the native executable takes a _few_ seconds. You could
We use a profile because, you will see very soon, packaging the native executable takes a few minutes. You could

That's more in line with what I observe. But maybe I just have a slow machine? I know it was supposed to be sarcastic but I'm not sure users will get it.

----

These are properties are normal Quarkus config properties, so if you always want to build in a container
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
These are properties are normal Quarkus config properties, so if you always want to build in a container
These are normal Quarkus config properties, so if you always want to build in a container

These are provided in `application.properties` the same as any other config property.


These options can be seen in the link:all-config.html[All Config Documentation] under the prefix `quarkus.native.`.
Copy link
Member

Choose a reason for hiding this comment

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

I would rewrite this paragraph and include the properties directly with:

include::{generated-dir}/config/quarkus-core-native-config.adoc[opts=optional]

@gsmet gsmet added this to the 0.27.0 milestone Oct 23, 2019
@geoand geoand added the triage/needs-rebase This PR needs to be rebased first because it has merge conflicts label Oct 23, 2019
@stuartwdouglas stuartwdouglas removed the triage/needs-rebase This PR needs to be rebased first because it has merge conflicts label Oct 23, 2019
@stuartwdouglas stuartwdouglas force-pushed the single-build-target branch 2 times, most recently from cbc70cb to fc9cafb Compare October 23, 2019 23:39
@dmlloyd
Copy link
Member

dmlloyd commented Oct 23, 2019

This is a bit of a major design change, did I miss the list discussion about this? There was some discussion in chat but I don't think the conclusion was "do it".

@stuartwdouglas
Copy link
Member Author

The problem is that without this change we don't really get much extra flexibility from doing native in a build step (and eventually in an extension). You can produce extra stuff that goes into the image, but that is it.

@stuartwdouglas
Copy link
Member Author

There are also quite a few other problems with the current build step approach:

  • Anything that wants to consume the output (e.g. to zip it up into a deployment package) must pick the type of jar they want to consume. If the user has selected a different type both are generated.
  • If there is the possibility of generating both we need different names for the uberjar and thin jar in the output dir.

If this PR is a problem then I would rather revert the change to building in build steps and worry about this post 1.0 than stick with the current approach of allowing multiple outputs, as I think it is very limiting and not something we want to get stuck supporting.

@dmlloyd
Copy link
Member

dmlloyd commented Oct 24, 2019

There are other ways to get the same kinds of additional flexibility (which we'd want to enumerate anyway). I think we should have a discussion about whether we want to go this route.

In principle I like it because it's simpler - however it's also pretty different from what we had discussed previously from a usability standpoint. We basically have to be OK with telling the user that they need multiple modules or profiles or something to build multiple output types.

I'm OK with revisiting it later though if nobody else objects.

@stuartwdouglas
Copy link
Member Author

You could do it by having some way of filtering particular build items so they only apply to particular output types, but it is much more complex.

Technically an extension could actually force the generation of multiple output types if required, e.g. by consuming both the JarBuildItem and NativeImageBuildItem. I can't really think of a valid use case where you would want both Uber and Thin jars though, at the moment you can't do it and AKAIK nobody has asked for it.

@gsmet gsmet added the triage/on-ice Frozen until external concerns are resolved label Oct 25, 2019
@stuartwdouglas
Copy link
Member Author

@dmlloyd are you ok with this now after our conversation the other day?

@dmlloyd
Copy link
Member

dmlloyd commented Oct 28, 2019

Yes I'm OK with it. I think that even if we introduce the multi-augment strategy, this change would be a reasonable basis to start with.

@gsmet gsmet added triage/waiting-for-ci Ready to merge when CI successfully finishes and removed triage/on-ice Frozen until external concerns are resolved labels Oct 28, 2019
@stuartwdouglas stuartwdouglas merged commit 98288f8 into quarkusio:master Oct 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release/noteworthy-feature triage/waiting-for-ci Ready to merge when CI successfully finishes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants