-
Notifications
You must be signed in to change notification settings - Fork 821
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
Support for project output artifacts #4495
Conversation
599b74a
to
826ebec
Compare
added @lkishalmi since I'm going to make equivalent changes for Gradle in a separate PR. Please comment on the APIs. |
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.
Proposed API looks sane to me.
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.
Well, not finished with the review, just made some comments.
* contexts, individually for each project, to be active. | ||
* @return the target project. | ||
*/ | ||
public @NonNull Project getProject() { |
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.
There are many possible null returns in this class. That should be avoided unless it brings a meaning.
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.
this particular one does not ;)) the others were addressed in a1c2f1b
* @return Lookup with additional information or {@code null}. | ||
*/ | ||
public @CheckForNull Lookup getLookup() { | ||
return lookup; |
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.
Can return Lookup.EMPTY
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.
Hm, with all those nulls (including this one), I intended to signal explicitly "no value". For example, if no Lookup + properties + profiles are present the implementation can take a great shortcut (= no customization to the project model or services).
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.
Well if those shortcuts are important, then probably I'd intorduce a "boolean isShortcutAllowed()"
method here to do the necessary checks inside here. Of course I assume that you can do shortcuts if no lookup + properties + profiles are present.
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.
The little trouble is that it is the project implementation that decides which part may potentially affect the project data evaluation ... all stuff except the Lookup can be checked for 'no ops' (empty), the Lookup is ... well, peculiar. Eventually I could document that the exact value Lookup.EMPTY
may be used for optimizations.
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.
Addressed in a1c2f1b
* @return user properties, or {@code null} if unspecified. | ||
*/ | ||
public @CheckForNull Map<String, String> getProperties() { | ||
return properties == null ? null : Collections.unmodifiableMap(properties); |
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.
Collections.emtyMap()
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.
Addressed in a1c2f1b
* @return applied profiles or {@code null} if unspecified. | ||
*/ | ||
public @CheckForNull Set<String> getProfiles() { | ||
return profiles == null ? null : Collections.unmodifiableSet(profiles); |
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.
Collections.emptySet()
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.
Addressed in a1c2f1b
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.
Personally, I don't find adding new implementation and friend dependencies attractive.
</run-dependency> | ||
</dependency> | ||
<dependency> | ||
<code-name-base>org.netbeans.modules.maven.embedder</code-name-base> |
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.
These modules don't provide real API. Depending on them from foreign "micronaut" module is strange. Also adding a dependency on Maven support, when "micronaut" supports both, Maven and Gradle is strange.
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.
re. depending from Micronaut no Gradle | Maven - agreed, a solution would be to create eager micronaut-maven + micronaut-gradle modules.
re. no real API - hm, I need to use the maven model types, i.e. org.apache.maven.model.PluginExecution
. These are currently exposed to the module system by maven.emebdder
. Is there a better way how to import those types ?
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.
Well. I gave a few thoughts to some abstraction, but ... even if I make some declarative support in the layer, the Micronaut will not be entirely indepenent: it would need to provide dual declarations anyway, since maven + gradle identifiers for the native-image plugin differ; so maybe there would not be a formal dependency, but actual yes.
I thought about supporting some 'declarative' query, such as "give me a variable Foo", that would be registered in project type's XML area and translated to plugin-execution-goal-configuration (maven) or extension-property for gradle. But I'd fear that this query need not to satisfy further usecases from different future project-neutral queries: not all data are just variables to be picked up.
I'd better wait with such support after more users appear, but this could be the an abstract way to query project's setup.
} | ||
ActionProvider ap = project.getLookup().lookup(ActionProvider.class); | ||
return | ||
nbmp.getMavenProject().getPlugin("io.micronaut.build:micronaut-maven-plugin") != null && |
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.
I was hoping this (presence of some plugin) could be specified declaratively. Isn't there already a support for a plugin to trigger some configuration and configurations adding some actions?
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.
Hmmm, good catch, will try to use plain isActionEnabled
.
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.
Addressed in 2917d2b The project action ID is enabled based on plugin's presence, so the UI action will enable/disable itself accordingly.
* @param all | ||
* @return | ||
*/ | ||
public Result findArtifacts(ProjectArtifactsQuery.Filter query); |
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.
I prefer singletonizer SPI over ProjectArtifactsImplementation
and Result
inner class.
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.
addressed in 70dcb7f
@@ -32,16 +29,9 @@ | |||
* @author sdedic | |||
*/ | |||
public interface ProjectDependenciesImplementation { |
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.
Removing methods from spi
package? Isn't that an API change?
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.
This "API" was never published, even as a friend.
* @throws IOException when the selected configuration cannot be persisted. | ||
* @since 1.89 | ||
*/ | ||
public <C extends ProjectConfiguration> boolean setActiveConfiguration(@NonNull Project p, @NonNull C cfg) throws IOException { |
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.
This would type the same without generics.
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.
The project API itself is using generics ... (?)
<build-prerequisite/> | ||
<compile-dependency/> | ||
<run-dependency> | ||
<implementation-version/> |
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.
Implementation dependency!?
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.
Yes, since the project.dependency
is still not public - planning to open it before NB16
CompletableFuture result = new CompletableFuture(); | ||
METADATA_PROCESSOR.post(() -> { | ||
try { | ||
ArtifactsResult arts = ProjectArtifactsQuery.findArtifacts(p, filter); |
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.
All the changes just to get name of artifact to VSCode TypeScript side...
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.
erm. not just the name, but also the location of the potential output. Yes, it's large, but NetBeans Project APIs themselves lack a description of usual project metadata, some identifier etc. The location of to-be-compiled output could be useful for packaging steps in NB, too.
@@ -182,6 +182,7 @@ | |||
<friend>io.github.jeddict.runtime</friend> | |||
<friend>io.github.jeddict.rest.generator</friend> | |||
<!-- XXX <subpackages> not permitted by schema --> | |||
<friend>org.netbeans.modules.micronaut</friend> |
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.
I don't like this. micronaut
module shall not have direct dependency on Maven (embedder).
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.
would be gradle+maven micronaut bridge modules more approppriate ?
* prescribe different profiles, properties etc than have been used for the default model. | ||
* | ||
* @param context the loading context | ||
* @return evaluated project |
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.
Missing versioning, I assume.
One more reviewer / approver would be awesome... |
* the file is the library consumed, e.g. from a local repository. For outputs, the artifact | ||
* represents the build output, usually in project's build directory. Note that FileObject for | ||
* a dependency and its corresponding Project may not be the same. | ||
* |
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.
Probably there should be mentioned, that it can return null, if the local file doesn't exist?
2 approvals; going to merge soon, after rebasing to master (minor clash) + consolidating fixes (less clutter). |
…r declarative project actions.
a063662
to
adfae72
Compare
This PR actually consists of a series of features. I should probably make separate requests for that, but ... that would prolong the review process, I believe.
The overall goal was to add a project query to get the project's output in form of an identifiable artifact(s) and their supposed (not built yet) or existing locations - and expose the analysis result through LSP.
Overview:
ProjectActionContext
This one I wanted for a long time already. Many project APIs or services are likely to be affected by project configurations - and it's rather hard to invoke the services for a specific configuration, which is not the active one. By default UI works against the active configuration, which is fine, but API access should have an option to specify it.
Adding configurations to all APIs is hard, and sometimes it is even impossible when the execution goes through some uncooperative layers.; the
ProjectActionContext
is a way how to pass the information 'out of band'.I needed to pass specifically project action to distinguish different types of build; for example Micronaut plugins (gradle, maven) support a regular build, and a native-image build, that produces different output(s).
Important note for reviewers: while the ProjectActionContext allows to specify a profile or user properties, which can be ignored by the underlying project system, and give almost complete control over project model evaluation to the caller (at least for gradle and maven), I am not 100% sure if the properties + profiles is a good idea. A conservative approach would be to insist on creating a
ProjectConfiguration
for the profile+properties setup and use that.Reviewers, share your opinion on this; these ProjectActionContext properties could be eventually added (compatibly) later.
ProjectArtifactsQuery
this is a project query as usual, asking for project output with GAV coordinates (if applicable). I am almost sure the
ArtifactSpec
will satisfy PHP, too ... but I still keep it private until Gradle implementation is complete.The
Filter
specification does not cover all query cases, but works for the basic ones. When one wants "any output" and a specific output type / classifier.Micronaut support for native image build
Added a project action and project action mapping for Maven to execute native-image build of the micronaut app. The action becomes available (and is configurable in project customizer) when one adds Micronaut plugin to the maven project build configuration.
Maven fixes
During implementation I've realized that the native-build action, contributed to the Maven by
MavenActionsProvider
is not visible in the customizer. I was little shocked when I realized that the customizer essentially hardcodes the list of (standard) actions and only displays user-defined ones in addition to the hardcoded list. I've corrected the behaviour ofM2Configuration
in that it lists just config-specific mappings, the others are retrieved throughActionToGoalUtils
.I've also realized that the declarative actions lack proper support for I18N; so I've added a convenience lookup in
Bundle.proeprties
using the action's (profile's) id, and in additiona specifyingdisplayName
that starts with '#' also makes a lookup, with the possbility to identify the Bundle path as well.