Bug 210465 - Distinguish required projects from submodules
Distinguish required projects from submodules
Status: RESOLVED FIXED
Product: projects
Classification: Unclassified
Component: Generic Projects UI
7.2
All All
: P2 (vote)
: 8.0
Assigned To: Milos Kleint
issues@projects
: API, API_REVIEW_FAST, UI
Depends on:
Blocks: 162612 172849 203222 237834
  Show dependency treegraph
 
Reported: 2012-03-30 22:05 UTC by Jesse Glick
Modified: 2013-11-20 13:44 UTC (History)
5 users (show)

See Also:
Issue Type: TASK
:


Attachments
list of files using SubprojectProvider (6.14 KB, text/plain)
2012-09-17 12:33 UTC, Milos Kleint
Details
api diff change (7.38 KB, patch)
2013-03-15 13:33 UTC, Milos Kleint
Details | Diff
entire patch (37.48 KB, patch)
2013-03-15 13:34 UTC, Milos Kleint
Details | Diff
updated api diff change (10.88 KB, application/octet-stream)
2013-03-20 08:54 UTC, Milos Kleint
Details
updated entire patch (41.13 KB, patch)
2013-03-20 08:54 UTC, Milos Kleint
Details | Diff
updated api diff change, containing 2 new interfaces now (16.43 KB, application/octet-stream)
2013-11-06 09:04 UTC, Milos Kleint
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jesse Glick 2012-03-30 22:05:31 UTC
From bug #203222 comment #5 point #1:

a. Remove all GUI from Open Project dialog referring to "subprojects". If you
want to open them, open the root first, then use the context menu later.

b. Deprecate SubprojectProvider, whose current semantics is unusably vague,
and introduce two separate SPIs (or modes of a single SPI): list of project
dependencies (ReferenceHelper for Ant-based projects, <dependency>s mapped to
projects for Maven); and list of submodules (<modules> for Maven, also
idiosyncratic impls for Ant-based NBM suites and EARs). May be tricky since it
seems there are a couple dozen places where foreign code (i.e. outside the
project type module) is calling SubprojectProvider with some sort of
expectation of semantics. Might need to do some kind of compatibility work.

c. Refine existing context menu item "Open Required Projects" into two actions
corresponding to the two new SPIs. Decide for each kind of project which of
these actions to display (or neither, or both).

d. Either remove or refine the "Project and All Required Projects" option in
the "Create New Group" dialog.
Comment 1 Milos Kleint 2012-09-17 12:33:06 UTC
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)
Comment 2 Milos Kleint 2012-09-17 12:33:35 UTC
Created attachment 124450 [details]
list of files using SubprojectProvider
Comment 3 Quality Engineering 2012-10-01 12:02:04 UTC
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
Comment 4 Milos Kleint 2013-03-15 13:33:49 UTC
Created attachment 132645 [details]
api diff change
Comment 5 Milos Kleint 2013-03-15 13:34:11 UTC
Created attachment 132646 [details]
entire patch
Comment 6 Milos Kleint 2013-03-15 13:43:19 UTC
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
Comment 7 Jesse Glick 2013-03-15 14:18:04 UTC
[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.
Comment 8 Milos Kleint 2013-03-15 14:42:24 UTC
(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.
Comment 9 Jesse Glick 2013-03-15 15:31:24 UTC
(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.
Comment 10 Milos Kleint 2013-03-15 15:52:34 UTC
(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.
Comment 11 Milos Kleint 2013-03-15 16:05:01 UTC
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.
Comment 12 Jesse Glick 2013-03-15 18:45:48 UTC
(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).
Comment 13 Milos Kleint 2013-03-18 06:31:07 UTC
(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.
Comment 14 Jesse Glick 2013-03-18 14:10:25 UTC
(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.
Comment 15 Milos Kleint 2013-03-19 10:10:45 UTC
(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.
Comment 16 Jaroslav Tulach 2013-03-20 08:46:37 UTC
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()"?
Comment 17 Milos Kleint 2013-03-20 08:54:19 UTC
Created attachment 132820 [details]
updated api diff change
Comment 18 Milos Kleint 2013-03-20 08:54:47 UTC
Created attachment 132821 [details]
updated entire patch
Comment 19 Milos Kleint 2013-03-20 08:59:50 UTC
(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.
Comment 20 Jesse Glick 2013-03-20 11:18:04 UTC
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.
Comment 21 Milos Kleint 2013-03-20 12:07:36 UTC
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.
Comment 22 David Konecny 2013-03-20 21:53:16 UTC
Reverting unintended change of TM.
Comment 23 Jaroslav Tulach 2013-04-02 07:15:27 UTC
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>
Comment 24 Milos Kleint 2013-11-06 09:04:26 UTC
Created attachment 141884 [details]
updated api diff change, containing 2 new interfaces now
Comment 25 Milos Kleint 2013-11-06 09:06:34 UTC
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.
Comment 26 Milos Kleint 2013-11-18 13:13:55 UTC
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.
Comment 27 Milos Kleint 2013-11-20 13:44:08 UTC
http://hg.netbeans.org/core-main/rev/5c1f831c61fe


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo