Bug 170056

Branch: http://hg.netbeans.org/prototypes/rev/separate-lookup-170056
Builder job: http://deadlock.netbeans.org/hudson/job/prototype-separate-lookup-170056/

Created attachment 90561 [details]
hg diff --git -r fa9887b26708:5662c2e3f7b9

The patch introduces new module openide.util.lookup and changes dependencies of related platform modules to use it. It also improves the fix-dependencies task and runs the task before compilation (so the update of dependencies is done automatically). There is no  ModuleAutoDeps right now, as the new module will stay on JDK classpath and there shall be no runtime issues.

The proposed change in versioning is to increase both versions of openide.util and openide.util.lookup to 8.0 and split their apichanges.xml appropriately. Dependencies of all modules in the netbeans.org source tree will need to be updated as shown in patch
http://deadlock.netbeans.org/hudson/job/prototype-separate-lookup-170056/lastSuccessfulBuild/artifact/nbbuild/changes.diff

As this is a source incompatible change, a change in apisupport should be introduced together with this change to warn the users / help them fix the dependency their projects have on lookup.

Re. TN01: Right now there is a change in harness to automatically modify the project.xml to contain dependency on openide.util > 8.0 as well as openide.util.lookup > 8.0. So it is enough to compile and the dependencies are fixed.

a change in harness will not cater to maven users unfortunately. OTOH it should be sort of easy to find the artifact for classes once people recognize the problem. Can we make this incompatible change visible in documentation somehow? In addition to the apichanges document which very few people actually read.

Why introduce this a week before Code freeze BTW?

Re. dependencies on bug 83734 and bug 167817: I consider those nice to have. The current diff achieves the same without any general infrastructure.

Just a couple of comments:

RM01: The attachment doesn't contain patch to fix-dependencies.

RM02: Is the case of newer harness used with older platform covered? Dependencies should not be updated in this case and this is the default when someone builds against non-IDE platform.

RM03: It will nonetheless break build of projects with custom and/or older harness and newer platform, e.g. JavaFx.

RM04: Build process that changes version-controlled files without being told so can cause confusion and some problems for users.

I also agree with Milos that this change needs to be documented visibly, especially because of points 3 and 4.

> Why introduce this a week before Code freeze BTW?

This change is not for 6.8!

Created attachment 90782 [details]
 hg diff --git -r fa9887b26708:5662c2e3f7b9

Providing correct diff. Sorry.

[JG01] -1 on any special handling for this refactoring in harness. Please use ModuleAutoDeps only. We are in no rush so there is no reason to do this the wrong way.


[JG02, seconding RM04] Do not call fix-dependencies from basic-init, nor fix-test-dependencies from build-test-dist. Modifying source files during a build is unacceptable. If a module has obsolete dependencies acc. to ModuleAutoDeps, correct them transiently in <jarwithmoduleattributes> and issue a warning suggesting that the user run fix-dependencies at their convenience. Optionally, improve apisupport.project to put a warning icon on Libraries in case a MAD rule is triggered and add a context menu item to run fix-dependencies.


[JG03] Do not use <implementation-version/> for deps on org.openide.util.lookup. Please correct all instances in your patch to use <specification-version/> with the exception of core pseudomodules which were already using an impl dep on org.openide.util for reasons of access to some class in org.netbeans.modules.openide.util now moved to openide.util.lookup, such as NamedServicesProvider. In particular, openide.nodes does not appear to need any impl dep on openide.util.lookup; do not blindly transfer impl deps from openide.util, evaluate each individually (and check whether it is possible to remove the impl dep on openide.util if one is being added on openide.util.lookup).


[JG04] Do not use Logger.getLogger("global") or its equivalent. Use a logger based on the containing class name.


RM02 is I think just too bad; if you want to build against 6.8+1 platform, you will need to either use 6.8+1 harness, or manually update your dependencies to include org.openide.util.lookup. We make no promises that all future Platform releases can be built using an arbitrarily old harness.

Jesse, Richard. Thanks for your comments. However most of them are depending on better handling of ModuleAutoDeps in harness. I do not feel I have power to implement that. Will one of you help me?

The new ModuleAutoDeps stuff works fine. Thanks Jesse. I removed the modification of the build script before compilation. I'll process your other comments by Dec 17 and then I'd like to integrate the change.

Re. RM02: The new patch achieves that by using ModuleAutoDeps. Nothing is updated until user explicitly invokes fix-dependencies. Then the dependencies are updated to last state known by the harness. In case this is not what people want, they can always update their dependencies manually.

Re. RM04, JG02: OK. No longer the case.

Re. RM03: You mean it will break old, custom modified harnesses. Well, that is said, but such people just have to properly upgrade.

Re. JG01: Done now, thanks to Jesse's help.

Re. JG03: Fixed in ac605796011f for openide.nodes and openide.awt

Re. JG04: Common, it is just in a test.

I am providing the current diff to tentatively scheduled for integration on Dec 17. In addition to previous, it mostly contains fixes of javadoc links: http://hg.netbeans.org/prototypes/rev/3011e63021b9

JG01 does not seem to be done in FixTestDependencies, which continues to have a fixOpenideUtil method. Nor does it seem to be done in nbbuild/templates/common.xml, which also has special handling for this separation.


[JG05] Second hunk in nbbuild/build.xml (<subant-junit buildpath="${nb_all}${file.separator}performance"...>) looks suspicious and is probably wrong. Mishandled merge conflict perhaps? Remove.


[JG06] Misplaced indentation in o.n.bootstrap/nbproject/project.xml. Fix, and make sure it is not the fault of a tool like fix-dependencies (which should really be using XMLUtil to read/write structure, not doing textual substitutions).


[JG07] Bogus URL in openide.util.lookup/src/org/openide/util/package.html.


[JG08] Typo in openide.util/module-auto-deps.xml#//description.


[JG09] openide.util/nbproject/project.properties#spec.version.base is missing an extra ".0".

Thanks for throughout review. I plan to integrate on Saturday, Sunday Dec 19-20, 2009. Latest diff:
http://hg.netbeans.org/prototypes/rev/8a6d6f1041c1


Re. JG07: No, this is correct URL which will be replaced during javadoc to proper reference.

Re. JG08: Fixed in b66ff57336d7

Re. JG09: Fixed in eb209c9e1d24

Re. JG06: The problem in o.n.bootstrap fixed in 0c0a09c2444a

Re. JG01: The fix-dependencies is now not executed automatically, it needs to be triggered manually. That also lowers the size of the problem. The fact that by default there is no "fixing" by default made me claim JG01 as fixed in my previous comment. I do not see the current state as blocker. The harness (common.xml in fact) already contains conversion rules for fixing dependencies. Adding new one for a module that almost everyone is depending on is OK. It is not general solution, but in case of openide.util it is justifiable. If there is a need for general solution, let's track it as separate issue. 

Re. JG05: Maintaining the branch is costly and not without errors (fixed in 989ad948c240). Let's integrate this highly desirable change now (when there is quiet time full of vacations). Remaining polishing can be tracked as separate issues.

JG07 - http://top/org/openide/util/lookup/doc-files/lookup-api.html is replaced? By what? I think you meant @TOP@/org/openide/util/lookup/doc-files/lookup-api.html or similar.


JG01 - I filed bug #178807.

Re. javadoc. The meaning of http://top/ is the same as @TOP@, but for some reason @TOP@ does not work in package.html

Re. JG06: I found the indentation problem and fixed it in 9303708a12f8

Merged as core-main#58d27b9032b8

Integrated into 'main-golden', will be available in build *200912210200* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/58d27b9032b8
User: Jaroslav Tulach <jtulach@netbeans.org>
Log: Merge of #170056: Splitting Lookup into own module