a simple search find . -type f -name "*.java" -print0 | xargs -0 grep -il .SubprojectProvider

reveals quite a few classes that use subproject provider. Some are obvious (implementations of the interface), but there are quite a few places with unknown semantics.

attaching the result (with excluded cases in tests)

Created attachment 124450 [details]
list of files using SubprojectProvider

Integrated into 'main-golden', will be available in build *201210010929* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/abdb7ad4afa8
User: Milos Kleint <mkleint@netbeans.org>
Log: #210465 mostly adding comment warnings about subprojectprovider usage

Created attachment 132645 [details]
api diff change

Created attachment 132646 [details]
entire patch

please review this change to project system apis. There is a diff of the projectapi module and then the rest of the modules affected. Not all code is rewritten to use the new code, I will be filing issues against the affected codebases once the API is integrated. also not all module spec versions are increased, I will do that right before integration to avoid 3rd party upgrades to the same version in the mean time

[JG01] prj.getLookup().lookup(DependencyProjectProvider.class) violates the principle of separation of API from SPI. There should be an SPI interface with and an API final class with static utility methods.

That also means that some code, e.g. in IDEOutputListenerProvider, could be simplified: the API can take care of dealing with the old SubprojectProvider if it in fact appears in lookup and the new interface does not. (If we had done this to begin with, there would be no need to keep a SubprojectProvider in project lookup now.) And the API class would be a reasonable place to add convenience methods to replace things like ExecutionChecker.recurseSubProjects.


[JG02] Rather than keeping the original SubprojectProvider impl in project lookup, you should be able to just write an impl of the new SPI interface, and use a factory method to create an instance of the deprecated SPI.


[JG03] For compatibility, do not change the meaning of SubprojectProvider, e.g. for Maven projects. Instead create new API/SPI which can separately report both required projects and submodules. This way existing code will still be able to use SubprojectProvider to get dependencies, but will see a deprecation warning and be encouraged to migrate to the new more specific APIs.


[JG04] Project.getLookup Javadoc should mention all recommended SPIs.

(In reply to comment #7)
> [JG01] prj.getLookup().lookup(DependencyProjectProvider.class) violates the
> principle of separation of API from SPI. There should be an SPI interface with
> and an API final class with static utility methods.

That's what the old API did as well, so I used the same pattern minimizing the impact. There's really nothing to be proud of in this API change, it's just something that we need to get rid of JG03 case 2 (see below)

Yes, recursive processing of subprojects could be added as API along with wrapping SPI classes in the API but it's way more work and public exposure than I want to achieve here really. 
Bluntly speaking another option at my hands is to just remove JG03 cases (see below) from the implementation of maven's SubprojectProvider and let the maintainers of the code abusing SubprojectProvider deal with the bugs created by that..


> 
> [JG02] Rather than keeping the original SubprojectProvider impl in project
> lookup, you should be able to just write an impl of the new SPI interface, and
> use a factory method to create an instance of the deprecated SPI.
> 

I don't follow here. Both interfaces are currently supported but have different contracts. existing code in non-maven projects can exist as it did before without changed.

> 
> [JG03] For compatibility, do not change the meaning of SubprojectProvider, e.g.
> for Maven projects. Instead create new API/SPI which can separately report both
> required projects and submodules. This way existing code will still be able to
> use SubprojectProvider to get dependencies, but will see a deprecation warning
> and be encouraged to migrate to the new more specific APIs.
> 

That's an intentional non backward compatible change. Please note that 3 semantically incompatible changes were done
1. no longer are <modules> elements traversed recursively for given project. Merely a performance optimization, never was part of official contract. But some clients rely on it IMHO.
2. no longer are project's dependencies included for non-pom project. Without this part I would not even bother introducing the new API, this is the goal I want to achieve here. A nominally backward compatible way would be to create yet another interface and deprecate subprojectprovider entirely. That's yet another name to come up with.
3. pom packaging <module> elements show up as subprojects now. Consequence of 1. without it recursion would not work.

> 
> [JG04] Project.getLookup Javadoc should mention all recommended SPIs.

Sure, I can add it there.

(In reply to comment #8)
> A nominally backward compatible way would be to create
> yet another interface and deprecate subprojectprovider entirely.

Yes, that is exactly what I was recommending. Deprecate SubprojectProvider as too vague to be useful, which will let existing users continue to function unchanged; and introduce two new SPIs (probably with a single API class collecting all the necessary static methods): one for submodules, one for required projects. Could even have just a single SPI interface with two methods, I guess.

(In reply to comment #9)
> (In reply to comment #8)
> > A nominally backward compatible way would be to create
> > yet another interface and deprecate subprojectprovider entirely.
> 
> Yes, that is exactly what I was recommending. Deprecate SubprojectProvider as
> too vague to be useful, which will let existing users continue to function
> unchanged; and introduce two new SPIs (probably with a single API class
> collecting all the necessary static methods): one for submodules, one for
> required projects. Could even have just a single SPI interface with two
> methods, I guess.

Which one of the interfaces would be used by the "required projects" ui? it would *have* to be the currently not existing api that delegates to maven modules, otherwise we would not solve this issue. 

and maven support is the only known project type so far even requiring the distinction. for all other project types we have the subprojectprovider is more or less sufficient abstraction. Even though IMHO part of the uses of the API would be better off taking compile/runtime classpath and process it through FileOwnerQuery and/or SourceForBinaryQuery.

if there's general agreement that we need a new api and deprecate subproject provider, i'm fine with adding it, however it felt too complex change to make for a change I cannot even explain without mentioning maven concepts.

(In reply to comment #10)
> Which one of the interfaces would be used by the "required projects" ui?

As in comment #0 item (c) both can be used, for different purposes.

> maven support is the only known project type so far even requiring the
> distinction.

Not true I think. It is relevant for Ant-based EARs which have submodules but no required projects. Same for Ant-based NBM suites. See comment #0 item (b).

> part of the uses of the API would be better off taking compile/runtime
> classpath and process it through FileOwnerQuery and/or SourceForBinaryQuery

If there are no actual use cases for a generic “required projects” function, that is fine, but it still means SubprojectProvider should be deprecated as meaningless and a SubmoduleProvider introduced; any code currently using SubprojectProvider to find “required projects” should just be rewritten to call Java-specific APIs and specify the expected dependency types (e.g. compile-time only).

(In reply to comment #12)
> (In reply to comment #10)
> > Which one of the interfaces would be used by the "required projects" ui?
> 
> As in comment #0 item (c) both can be used, for different purposes.

Actually cannot. The maven's "find dependencies by open/known projects" is the case we want to get rid of here in the open project dialog. As it depends on previous context, it shows wrong stuff when changing context (different branch with same GAV, different checkout of same code etc). This was always a problem but used to have a clear workaround because at some stage we used only transient information. Currently we persist this across sessions which helps tremendously if you have the same stuff all the time but when changing contexts is counter productive.

So the workflow for opening multiple related maven projects should be to open just the parent pom + open required. any other maven packaging will not have any Required projects shown.


> 
> > maven support is the only known project type so far even requiring the
> > distinction.
> 
> Not true I think. It is relevant for Ant-based EARs which have submodules but
> no required projects. Same for Ant-based NBM suites. See comment #0 item (b).
> 

well, it wasn't causing problem there. it's hardwired there always knowing which one of the projects is referenced.


> > part of the uses of the API would be better off taking compile/runtime
> > classpath and process it through FileOwnerQuery and/or SourceForBinaryQuery

So this is what I suggest:

1, have an API method that either does the default behaviour of FOQ + SFBQ processing (with most likely selectable scope) or depends on the newly introduced ProjectDependencyProvider in the project where we add scope to the interface as well. PDP needs to be internally recursive because in maven projects I cannot recursively go down into dependencies of dependencies and get PDP. the root project in the chain can have overrides on versions. Not sure if ant projects can get a non-recursive impl though. Maybe we will need a Result object returned which lists projects and recursiveness state.
2. subproject provider would stay in current form, with adjusted/simplified maven implementation as that's good enough for open project ui (my personal take on it only) or at least it's not what we want achieve to solve in this issue and issue 203222. I'm definitely no targeting comment #0 item (b) here.
3. new api from 1 would be use most likely by the majority of current usecases.

> 
> If there are no actual use cases for a generic “required projects” function,
> that is fine, but it still means SubprojectProvider should be deprecated as
> meaningless and a SubmoduleProvider introduced; any code currently using
> SubprojectProvider to find “required projects” should just be rewritten to call
> Java-specific APIs and specify the expected dependency types (e.g. compile-time
> only).

my problem with SubmoduleProvider is it's apparent maven-ness, one cannot really define it without referencing maven, the 2 other cases you mention were fairly happy with SubprojectProvider.

(In reply to comment #13)
> in the open project dialog

As in comment #0 point (a), I advocate removing any functionality of this kind from the Open Project dialog: it should open the selected project(s) and nothing else.

> the workflow for opening multiple related maven projects should be to open
> just the parent pom + open required.

Open Submodules

>> It is relevant for Ant-based EARs which have submodules but
>> no required projects. Same for Ant-based NBM suites.
> 
> it wasn't causing problem there.

The particular problem you mentioned of course does not exist with these project types, but they still suffer from the lack of an API to specifically open either submodules, or dependencies.

> have an API method that either does the default behaviour of FOQ + SFBQ
> processing (with most likely selectable scope) or depends on the newly
> introduced ProjectDependencyProvider in the project where we add scope to the
> interface as well.

Seems fine.

> PDP needs to be internally recursive because in maven projects
> I cannot recursively go down into dependencies of dependencies and get
> PDP. the root project in the chain can have overrides on versions.

Makes sense then that the API/SPI should have a 'boolean transitive' flag.

> subproject provider would stay in current form, with adjusted/simplified
> maven implementation

So what would the new meaning of SubprojectProvider actually be? And how do you deal with the compatibility implications for code calling it on Maven projects with certain expectations?

> my problem with SubmoduleProvider is it's apparent maven-ness, one cannot
> really define it without referencing maven

Why? “Gets a list of all projects physically contained or aggregated by this one.” Seems perfectly sensible for EAR or suite projects, not to mention an Android project with accompanying test project, etc.

> the 2 other cases you mention were fairly happy with SubprojectProvider.

Not really. For example comment #0 item (d): this project group kind in its current implementation is useless for NBM suite projects, because adding a source association to an NB platform could suddenly cause source dependencies to appear in the project list, when all you really wanted was the suite and its modules. The situation there seems quite analogous to Maven.

(In reply to comment #14)
> (In reply to comment #13)
> > in the open project dialog
> 
> As in comment #0 point (a), I advocate removing any functionality of this kind
> from the Open Project dialog: it should open the selected project(s) and
> nothing else.
> 
> >> It is relevant for Ant-based EARs which have submodules but
> >> no required projects. Same for Ant-based NBM suites.
> > 
> > it wasn't causing problem there.
> 
> The particular problem you mentioned of course does not exist with these
> project types, but they still suffer from the lack of an API to specifically
> open either submodules, or dependencies.

I have my doubts that this is really as straightforward as you suggest, any such changes in the UI would have to be approved by the UI review group and for me it's well beyond the scope of issue, I just want to fix a problem with maven projects.

I'm not claiming we cannot eventually go all the way but the currently implemented half-way doesn't block us from continuing in the future. However I currently don't have such big rewrites of project UI in the plan for the current release.

Y01 Separate API and SPI: Rather than putting all the burden on clients and forcing them to call into two SPI(!) interfaces, please enhance ProjectUtils. Something in the style of ProjectInformation - e.g. static method(s) that know what to call and never return null. Let's add something like:

public static Set<Project> getDependencyProjects(Project p) - deals with new SPI
public static Set<Project> getSubprojects(Project p) - deals with old SPI
public static Set<Project> getRelatedProject(Project p) 
  - a merge of getSubprojects and getDependencyProjects

Clients should be encouraged to call these API methods rather than dealing with content of lookup directly. The names, meaning of the methods is tbd., but there should be one method that does exactly the same thing in new version as it did in 7.3.

Y02 Write a hint to convert direct calls to SubprojectProvider with the call to getReleatedProjects (if that is the replacement that remains compatible)

Y03 "Dependency projects" sound weird. Should not the method be named "getProjectDependencies()"?

Created attachment 132820 [details]
updated api diff change

Created attachment 132821 [details]
updated entire patch

(In reply to comment #16)
> Y01 Separate API and SPI: Rather than putting all the burden on clients and
> forcing them to call into two SPI(!) interfaces, please enhance ProjectUtils.
> Something in the style of ProjectInformation - e.g. static method(s) that know
> what to call and never return null. Let's add something like:
> 

Done in the attached new patch. However only the new approach gets API haircut, the old SubprojectProvider keeps on being used as before..

> 
> Clients should be encouraged to call these API methods rather than dealing with
> content of lookup directly. The names, meaning of the methods is tbd., but
> there should be one method that does exactly the same thing in new version as
> it did in 7.3.

That's not really possible for maven projects. The entire point of the issue is *not* to do the same as in 7.3 (in SubprojectProvider) and for those who relied on the old behaviour we have the new API now. And BTW the old SubprojectProvider javadoc is fairly vague in terms of what is inside that I actually could start returning different values for maven projects *without* any new API..

> 
> Y02 Write a hint to convert direct calls to SubprojectProvider with the call to
> getReleatedProjects (if that is the replacement that remains compatible)

out of scope, and not so many uses anyway..

> 
> Y03 "Dependency projects" sound weird. Should not the method be named
> "getProjectDependencies()"?

IMHO getProjectDependencies suggests to return the entire list of dependencies, but we only return the projects here. getDependencyProjects is more correct here.

I continue to maintain (as in JG03) that the current approach is off base. You are radically changing the behavior of some implementations of an existing SPI (SubprojectProvider). That does not really fix the issue, which is that some clients are looking for submodules and some clients are looking for dependencies and there should be an API which clearly provides either on demand. SP has traditionally provided the union of these (*) so it should be deprecated but its impls kept around for compatibility.

(Y02 is probably pointless; every caller of SP should see the deprecation warning and be forced to consider explicitly what it is they want. We need two new SPIs, and two matching new API methods in e.g. ProjectUtils. Each API method could check first for its new SPI, else fall back to returning whatever SP says. Also ProjectUtils.hasSubprojectCycles should be amended to call the new API method for dependencies, so as to ignore submodules for project types implementing the new SPIs.)

And again I will note that the API issue is not at all specific to Maven. Just look at apisupport.ant and compare SubprojectProviderImpl (lists dependencies) with SuiteSubprojectProviderImpl (lists submodules).

A proper set of APIs is also the foundation for fixing the poor UI.

I have expressed my opinion and it seems to have been rejected, so I am done reviewing this.


(*) Minus Maven submodules which happen to be <packaging>pom</>, something that could be fixed in a new submodule SPI.

fine, if there's no agreement that half way solution is a solution as well, let's pull the change from API review and keep the issue open.

Reverting unintended change of TM.

Y04 static method getDependencyProjects should probably return empty set instead of null

Y05 <code>SubprojectProvider</code> in javadoc should be {@link SubprojectProvider}. Ditto for <code>DependencyProjectProvider</code>

Created attachment 141884 [details]
updated api diff change, containing 2 new interfaces now

please see the updated api, now showing 2 distinct new interfaces one for dependency projects and one for projects contained in the current one (modules in maven)

I'm putting up just the api up for discussion, will work on implementation later when we agreed that this is the way forward.

I'm not entirely happy with the names of the interfaces, any suggestions welcome.

thanks for the review, I will proceed with the maven implementation of these interfaces and change some usages in project system to make use of the new apis.

http://hg.netbeans.org/core-main/rev/5c1f831c61fe