This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.

Bug 34699 - Add nonordering equiv. of OpenIDE-Module-Requires
Summary: Add nonordering equiv. of OpenIDE-Module-Requires
Status: RESOLVED FIXED
Alias: None
Product: platform
Classification: Unclassified
Component: Module System (show other bugs)
Version: 3.x
Hardware: All All
: P2 blocker (vote)
Assignee: Jaroslav Tulach
URL:
Keywords: API_REVIEW_FAST
: 22507 56600 72383 (view as bug list)
Depends on: 80702
Blocks: 34019 78605 80475
  Show dependency tree
 
Reported: 2003-06-30 18:50 UTC by Jesse Glick
Modified: 2008-12-22 21:44 UTC (History)
3 users (show)

See Also:
Issue Type: ENHANCEMENT
Exception Reporter:


Attachments
Could not this get a little bit more priority? The patch with ok api, ok test and funny and not working impl (15.45 KB, patch)
2006-06-28 21:03 UTC, Jaroslav Tulach
Details | Diff
Relatively nice patch, needs work on disabling (14.22 KB, patch)
2006-06-28 22:04 UTC, Jaroslav Tulach
Details | Diff
Reasonable implementation and removal of Requires for openide/io (76.03 KB, patch)
2006-06-29 10:17 UTC, Jaroslav Tulach
Details | Diff
I like the Recommends approach, it can be generally useful (85.41 KB, patch)
2006-07-13 16:20 UTC, Jaroslav Tulach
Details | Diff
Patch ready for integration (104.17 KB, patch)
2006-07-18 13:15 UTC, Jaroslav Tulach
Details | Diff
Commit message (9.71 KB, text/plain)
2006-07-19 07:54 UTC, Jaroslav Tulach
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jesse Glick 2003-06-30 18:50:13 UTC
In some cases it is desirable for an SPI-defining
module to require that some impl of an interface
be available (in cases where the interface module
is otherwise useless). The module cannot use
OpenIDE-Module-Requires, since that would produce
a cyclic dependency. It can be documented that all
clients which use the SPI individually require the
impl, but this is somewhat cumbersome in practice.

Suggest some variant of OIDE-M-R which still
requires >=1 provider to be enabled, but which
does not count as a dependency for purposes of
cycle detection nor topological ordering.

TBD whether this intuitive notion can actually be
formalized and implemented safely. For example, if
A requires T without ordering, B depends on A and
provides T, and C depends on A, and C's
ModuleInstall M calls T.getDefault(), it could
happen that the install order is A-C-B, in which
case M might get a null or uninitialized T impl.
One possible solution would be to treat the attr
as meaning that any module like C with a dep on A
would implicitly require T without needing to
declare it; this would make the attr just be
syntactic sugar (prevents the author of C from
forgetting the OIDE-M-R attr).


A different approach altogether is to permit the
manifest tag to be dropped, and some method in the
module system added, e.g.:

public static void ModuleInfo.requireDuring(String
token, Runnable r) throws NoProviderFoundException;

which would guarantee that at least for the
duration of the runnable (and possibly beyond it,
at the discretion of the impl), one or more
providers of the token be enabled. This could
permit impls to be left disabled until called.
However the implications of encouraging modules 
being enabled at random points during NB runtime
might be bad (e.g. for performance). And the impl
could not provide any kind of GUI or the effect
would be very disturbing.
Comment 1 Jesse Glick 2005-04-06 19:39:42 UTC
*** Issue 56600 has been marked as a duplicate of this issue. ***
Comment 2 Jesse Glick 2006-02-08 18:53:44 UTC
*** Issue 72383 has been marked as a duplicate of this issue. ***
Comment 3 Jaroslav Tulach 2006-06-28 21:03:18 UTC
Created attachment 31483 [details]
Could not this get a little bit more priority? The patch with ok api, ok test and funny and not working impl
Comment 4 Jaroslav Tulach 2006-06-28 21:08:48 UTC
*** Issue 22507 has been marked as a duplicate of this issue. ***
Comment 5 Jaroslav Tulach 2006-06-28 22:03:35 UTC
I have a better patch that works fine during enablement. Disabling still needs 
some work...
Comment 6 Jaroslav Tulach 2006-06-28 22:04:45 UTC
Created attachment 31485 [details]
Relatively nice patch, needs work on disabling
Comment 7 Jaroslav Tulach 2006-06-29 10:15:30 UTC
Let's implement this useful improvement.
Comment 8 Jaroslav Tulach 2006-06-29 10:17:16 UTC
Created attachment 31501 [details]
Reasonable implementation and removal of Requires for openide/io
Comment 9 Jaroslav Tulach 2006-06-29 10:18:30 UTC
Reviewers please review the changes:
http://www.netbeans.org/nonav/issues/showattachment.cgi/31501/X.diff
Comment 10 Jesse Glick 2006-06-30 19:24:43 UTC
Could also be used for:

org.openide.execution.ExecutionEngine
org.netbeans.modules.project.uiapi.*

Impl looks fine. My main concern is whether we even want this issue to be
implemented any more, at least in the case of something like IOProvider. Since
there is a default impl of the interface in the module, even code calling
IOProvider.getDefault() does not absolutely require an IOProvider in lookup.
More to the point, consider a unit test which tests code which contains calls to
IOProvider. No problem; either you do not care about testing the output (in
which case you do nothing) or you do want to check it (in which case you can
simply use MockServices etc.). Such tests will run today and they will run with
this patch. But what if you initialize the module system in the test later on,
e.g. to load layers? Suddenly the test will choke as the module system tells you
that IOProvider was not found.

Not sure how to solve this. The issue is really that

OpenIDE-Module: org.openide.io
OpenIDE-Module-Needs: org.openide.windows.IOProvider

is a recommendation that the app should contain some impl of this interface, but
no code will actually fail without it, while

OpenIDE-Module: org.netbeans.modules.projectuiapi/1
OpenIDE-Module-Needs: org.netbeans.modules.project.uiapi.ActionsFactory

really states that some impl of that interface has to be in lookup or there will
be assertion errors. The latter case is handled well by your impl. Perhaps the
former should be written as

OpenIDE-Module: org.openide.io
OpenIDE-Module-Recommends: org.openide.windows.IOProvider

meaning "if you can find a provider of this token, please enable it, but if you
can't, don't worry about it".

Alternately, perhaps the problem is limited to dummy implementations of
interfaces which are only of interest to unit tests (and we really want modules
in a live app to require an output window impl). If this is the case, then
perhaps we could change the impl so that no error is reported in case a token
cannot be found which is mentioned in the OIDE-M-Needs of a fixed (classpath)
module. This would mean that just adding org-openide-io.jar to your classpath
would never trigger an error, even if you loaded the module system; but if
loaded as a real autoload module it would force enablement of some OW impl.
Comment 11 Jaroslav Tulach 2006-07-13 16:20:01 UTC
Created attachment 31832 [details]
I like the Recommends approach, it can be generally useful
Comment 12 Jaroslav Tulach 2006-07-13 16:21:06 UTC
The previous patch implements Jesse's suggestions. If there are no strong 
objections I'll integrate it early next week.
Comment 13 Jesse Glick 2006-07-15 15:13:30 UTC
In

+                        boolean foundOne = false;
+                        Set<Module> possibleModules = new HashSet<Module>();
+                        for (Module other : providers) {
+                            if (other.isEnabled()) {
+                                foundOne = true;
+                                break LOOP;
+                            }
+                        }

did you really mean to break to LOOP? Skipping over other NeedsCheck's? The
control flow in the whole LOOP block is rather convoluted and I'm not sure I
understand it. Some comments would be nice. Probably needs more thorough tests,
I'm guessing; this kind of logic is easy to get wrong in less common cases. E.g.
what happens when a module requires one token, needs another, and recommends a
third. (Some day core parts of ModuleManager need to be rewritten to use some
kind of mathematically sound algorithm.)

BTW I don't recommend creating new JARs in the data dir for new tests for
ModuleManager. Haven't gotten around to rewriting existing tests yet, but I
would much prefer all test module JARs to be created in code on demand. Would
make it much easier to add new tests to check subtle variations of different
situations.


Please file a blocking issue for apisupport/project to have it somehow deal with
the new manifest keywords. I'm not really sure what the UI should look like, though.


Any particular reason why OpenIDE-Module-Specification-Version is incremented in
openide/io/manifest.mf but not in openide/execution/manifest.mf? And why
openide/io/api/doc/changes/changes.xml is patched but not
openide/execution/api/doc/changes/changes.xml?


Please file a dependent issue in projects/code to have projectuiapi/manifest.mf
specify

OpenIDE-Module-Needs: org.netbeans.modules.project.uiapi.ActionsFactory,
org.netbeans.modules.project.uiapi.OpenProjectsTrampoline,
org.netbeans.modules.project.uiapi.ProjectChooserFactory

and also remove from java/project/manifest.mf

OpenIDE-Module-Requires: org.netbeans.modules.project.uiapi.ActionsFactory

Or just do that as part of your commit, both as a demonstration of this new tag
and a sanity check that it is working in a complex situation.


There are probably other prov/req tokens that could better use one of the new
tags but we'll find those as we go. Overall, looks good.
Comment 14 Jaroslav Tulach 2006-07-18 13:15:09 UTC
Created attachment 31957 [details]
Patch ready for integration
Comment 15 Jaroslav Tulach 2006-07-18 13:23:02 UTC
I have added another test for chained recommends. That identified the problem 
with the "break LOOP" and also forced me to fix clearCache() method as well. 

The apisupport has not issue 80475.

Version numbers in openide/io and openide/exeuction has been incremented. 
Changes written.

I usedam suing  the "needs" in projects/code and removed that token from 
java/project/manifest.mf

I'd like to integrate tomorrow.
Comment 16 Jaroslav Tulach 2006-07-19 07:54:56 UTC
Created attachment 31980 [details]
Commit message
Comment 17 Jaroslav Tulach 2006-07-19 07:55:19 UTC
Implemented.