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.
Summary: | Add nonordering equiv. of OpenIDE-Module-Requires | ||
---|---|---|---|
Product: | platform | Reporter: | Jesse Glick <jglick> |
Component: | Module System | Assignee: | Jaroslav Tulach <jtulach> |
Status: | RESOLVED FIXED | ||
Severity: | blocker | CC: | apireviews, jtulach, mkrauskopf |
Priority: | P2 | Keywords: | API_REVIEW_FAST |
Version: | 3.x | ||
Hardware: | All | ||
OS: | All | ||
Issue Type: | ENHANCEMENT | Exception Reporter: | |
Bug Depends on: | 80702 | ||
Bug Blocks: | 34019, 78605, 80475 | ||
Attachments: |
Could not this get a little bit more priority? The patch with ok api, ok test and funny and not working impl
Relatively nice patch, needs work on disabling Reasonable implementation and removal of Requires for openide/io I like the Recommends approach, it can be generally useful Patch ready for integration Commit message |
Description
Jesse Glick
2003-06-30 18:50:13 UTC
*** Issue 56600 has been marked as a duplicate of this issue. *** *** Issue 72383 has been marked as a duplicate of this issue. *** 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
*** Issue 22507 has been marked as a duplicate of this issue. *** I have a better patch that works fine during enablement. Disabling still needs some work... Created attachment 31485 [details]
Relatively nice patch, needs work on disabling
Let's implement this useful improvement. Created attachment 31501 [details]
Reasonable implementation and removal of Requires for openide/io
Reviewers please review the changes: http://www.netbeans.org/nonav/issues/showattachment.cgi/31501/X.diff 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. Created attachment 31832 [details]
I like the Recommends approach, it can be generally useful
The previous patch implements Jesse's suggestions. If there are no strong objections I'll integrate it early next week. 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. Created attachment 31957 [details]
Patch ready for integration
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. Created attachment 31980 [details]
Commit message
Implemented. |