-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
[GR-36905] Implement --link-at-build-time and @<prop-values-file>. #4305
Conversation
010e771
to
3dd0030
Compare
432cb37
to
041c43f
Compare
Many thanks for the huge progress! Wondering though, is there any way to apply I haven't had the chance to explore this in more depth yet, but from the description it seems I would need to set |
Hi @Sanne
That would not make sense. If you use
to the Note that using
As the error messages indicates, the image builder refuses jar files on classpath that use
then the builder would accept that. In this case, since |
@christianwimmer, re:
This is currently not enforced. If you use the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Elegant design. We can use the same approach for class initialization which will make it well-behaved.
...src/com.oracle.svm.core/src/com/oracle/svm/core/configure/ReflectionConfigurationParser.java
Show resolved
Hide resolved
substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/option/OptionUtils.java
Outdated
Show resolved
Hide resolved
substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/option/OptionOrigin.java
Show resolved
Hide resolved
public String errorMessageFor(ResolvedJavaType type) { | ||
Class<?> clazz = ((OriginalClassProvider) type).getJavaClass(); | ||
if (clazz == null) { | ||
return "This error is reported at image build time because class " + type.toJavaName(true) + " is registered for linking at image build time."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An origin for this decision would be useful. Can we pass it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't. There is none.
It's just the fault behaviour that is responsible for that. I.e. if we have a ResolvedJavaType
that has no corresponding Class instance, we are dealing with a purely synthetic ResolvedJavaType
created by the builder itself. Thus the only sane default here is to require such ResolvedJavaType
s always to be fully resolved.
substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/LinkAtBuildTimeFeature.java
Outdated
Show resolved
Hide resolved
substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/LinkAtBuildTimeFeature.java
Show resolved
Hide resolved
2a87716
to
6429db2
Compare
...tevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/phases/SharedGraphBuilderPhase.java
Outdated
Show resolved
Hide resolved
@olpaw my understanding is that this is by design in order to prevent libraries from forcing their consumers to link everything at build time. However, this was the default until now and it would be nice to have the option to enforce it even after this patch goes in. To prevent libraries from abusing it we could require it to be done programmatically instead.
I think it makes sense if you want everything but a couple of classes/packages to be fully resolved at build time. Instead of having to create an exhaustive list of packages required to be resolved at build time, which might as well become incomplete in a future release, you would only need to define the classes/package that you don't require to be resolved at build time. |
Really happy to see movement on improving this! What I don't grok is how you foresee this change to be used in practice by frameworks like Quarkus, Micronaut, SpringNative etc. that has the need to extend/customize libraries that not yet have native-image configuration or for cases where a library config is not good for specific usecases. With the proposed solution we now move from being able to explicit control to something where libraries/jars themself are the one to do customization and it is not possible to change that per library without modifying the jar or generate massive lists of classes/packages? |
That would be a full include/exclude config design. The current PR does not have that. |
… to junit macro-option
…me and adjust comments
3678586
to
1655cb2
Compare
oracle/graal#4305 makes `--allow-incomplete-classpath` the default behavior (removing the flag) and introduces a new flag `--link-at-build-time` which enables us to set which classes/packages we require to be resolvable at build time. Passing `--link-at-build-time` without arguments to `native-image` requires all classes/packages to be resolvable at build time, essentially bringing the pre-22.1 behavior back.
Adds the following option to
native-image
:Depending on where and how the option is used it behaves differently:
Using the option with args is always possible, e.g.,
--link-at-build-time=foo.bar.Foobar,...
. The arguments are fully qualified class names, or package names (note: no package prefix names).When used on the module path or class path (embedded in
native-image.properties
files) only classes and packages defined in the same .jar file can be specified.@<prop-values-file>
can be used to redirect values for the option coming from a file relative to the place where the option is used.When being used without args
--link-at-build-time
it depends on where it is used:native-image.properties
files) all classes defined in that module are treated as link-at-build-time classesnative-image.properties
files) an error message is show:The PR also deprecates the option
--allow-incomplete-classpath
. Allowing an "incomplete classpath", i.e., link at image run time, is the new default and therefore does not need to be specified explicitly. The option is still allowed for compatibility reasons, but has no effect.Motivation for this change (see also the discussions in #2762 (comment), #3932 (comment), and #3929): Before this PR, support for an incomplete class path must be enabled by manually via
--allow-incomplete-classpath
. This ensures that no linking errors are thrown at run time. But since in practice many large projects needs to specify this option because optional dependencies are not always on the class path. So it is necessary to flip the default and enable the support by default.Still, well-structured libraries should be able to do all linking at image build time, to avoid surprising errors at image run time. And well-structured applications should be able to do the same for all classes of the application. This is enabled by the new option
--link-at-build-time
. It is designed in a way so that libraries can only configure their own classes, i.e., avoid any side effects on other classes. The restrictions of the--link-at-build-time
argument mentioned above ensure that.When using the Java Platform Module System, it is easy for a library to link all its classes at image build time by specifying
--link-at-build-time
without listing classes and packages explicitly. But when using the class path, this is not possible because for .jar files on the class path there is no 1:1 mapping between packages and .jar files - multiple .jar files can contribute to the same package, and multiple .jar files can be merged into a single large .jar file. Therefore, packages for libraries used on the class path need to be listed explicitly. To make this process easier and allow, e.g., automatic generation of package lists, the@<prop-values-file>
syntax can be used to provide the package list (or class list) in a separate file.