Bugzilla – Full Text Bug Listing
| Summary: | Aggregate Deployment Assembly fixes for Helios SR1 | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [WebTools] WTP Java EE Tools | Reporter: | Jason Levine <eclipse.org> | ||||||||||||||||||||||||||||||||||||||
| Component: | jst.j2ee | Assignee: | Konstantin Komissarchik <konstantin> | ||||||||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | Chuck Bridgham <cbridgha> | ||||||||||||||||||||||||||||||||||||||
| Severity: | major | ||||||||||||||||||||||||||||||||||||||||
| Priority: | P3 | CC: | cbridgha, ccc, danny.ju, david_williams, jasonpet, jfulghum.cs04, konstantin, larryisaacs, ludo, manderse, Michael.Stegherr, nagrawal, raj.alagumalai, ripsi_1, stryker | ||||||||||||||||||||||||||||||||||||||
| Version: | 3.2 | Flags: | david_williams:
pmc_approved+
konstantin: pmc_approved? (raghunathan.srinivasan) konstantin: pmc_approved? (naci.dai) konstantin: pmc_approved? (deboer) konstantin: pmc_approved? (neil.hauge) konstantin: pmc_approved? (kaloyan) cbridgha: review+ |
||||||||||||||||||||||||||||||||||||||
| Target Milestone: | 3.2.2 | ||||||||||||||||||||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||||||||||||||||||||
| Whiteboard: | PMC_approved | ||||||||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||||||||
|
Description
Jason Levine
Assigning to Chuck for initial investigation. Reassigning to Jason Peterson to investigate. This is a use-case that several of us have wished to god we could remove entirely, but simply cannot due to legacy concerns. This particular UI is nothing more than a desperate attempt to maintain SOME ui for the functionality in previous versions so that people who truly do depend on it can use it. Basically, in the past, the dynamic web project would mark, in its .classpath file, entries which it has on its build path but that it expects the parent (ear) to pull upwards into the parent. Rather than this information being stored in the Ear project's descriptors, telling the EAR to reach down and grab up what the EAR wants, the control was inverted and the dynamic web project was instead in charge of saying what it expected to be pulled out of it. I personally continue to feel that this is ridiculously backwards and nonsensical, but its there and we have to maintain it. What you are seeing in the UI you look at now is which .classpath entries have that flag marked. This flag is saying whoever the parent is should pull these entries up. So if DynamicWeb1 has, as a child project, Java1, it can pull up libs from inside Java1. Even if it looks like libs in Java2 are checked, DynamicWeb1 will *not* pull up these jars because Java2 is not a child project to DynamicWeb1. We'd love to find some way to mark this that allows it to be dependent on which parent project is looking, but based on legacy concerns this simply is impossible. Entirely impossible. The flag is currently stored on the .classpath entry itself and the expectation is that whatever project is the parent project will recognize this and pull up those jars. This can, however, be useful, if for example you have two different DynamicWeb projects (Dyn1 and Dyn2) and *both* have as a child UtilJar1 with such flagged jars. This way you only have to flag it once and not for both parent projects. Again, I would prefer if the usecase disappeared entirely, but some seem to have a problem with that idea ;) So this is what's left... a somewhat effective (if confusing) UI to maintain the use-case for those who depend on it. If you could think of some better way to make it clear to users what's going on, I'd love to hear your suggestions =] Resolving this as invalid since it is working as designed. Please see Rob's comment for details. I don't agree this is working as design.
Considering the use case where the user has two totally
independent project dependecies in workspace:
TestWeb1
+-- Test1
+--- JAR1
TestWeb2
+-- Test2
+--- JAR2
In Step 7 and Step 9, "Referenced Projects Classpath Entries"
show all projects in workspace, which is not correct.
The projects with classpath entries can be pulled from,
should be limited to the ones referenced the the parent project being selected.
In Step 7, for TestWeb1, should be only Test1
In Step 8, for TestWeb1, should be only Test2
Applying this filtering would solve the issue.
Re-opening. The way "Referenced Projects Classpath Entries" feature was implemented in Helios is very broken. We are still having trouble getting configurations to work that were trivial in Galileo. Lots of confusion on user forums. There is a very sound architectural reason for why the referenced project should be in control of what's exported into its container besides it. Consider the trivial case of a utility project with third-party dependencies that needs to be included in an EAR and separately (for another application) in a WAR. EAR and WAR should not have to track recursively what the utility depends on. We have changed nothing in the .classpath file or the publish method that has affected any of this. All that's been changed is the UI, and the UI simply does exactly what used to be done except a bit uglier, but it was needed to be done that way to avoid having multiple pages for each project. So I'm not sure what use-case you're having problems with, Konstantin. Do you want to add the flag to some .classpath entry to have it exported? You can do this in the current UI. The data is still stored in the .classpath file and the child project still "pushes up" what it expects into the parents. I agree that comment 5's suggestion may work, but currently there are a limited amount of ways (insufficient api) to be able to tell what kind of reference we have (fileset reference, jar reference, project reference, etc), which makes the filtering a bit hard. > We have changed nothing in the .classpath file or the publish method that has > affected any of this. That's correct, but UI was changed to make the existing classpath tagging facility practically inaccessible. What replaced it in the UI has a number of architectural issues. As such, the cases that used to work fine in Galileo now do not work. I am not referring to importing existing projects, which still works. I am referring to recreating similar project structures using the tools exposed to the user in Helios. > Do you want to add the flag to some .classpath entry to have it exported? You > can do this in the current UI. This needs to be done by WTP out of the box. I don't think that the architectural issues present in the replacement facilities can be fixed. The classpath container reference breaks third-party classpath containers as JDT is the only expected host of these. The containers cannot update themselves and may break in other ways. See another bug that I opened on this. The "Referenced Projects Classpath Entries" facility encourages users to create entangled project structures that will make it significantly more difficult for apps to be maintained. The consuming module should not have to know what libraries the consumed module depends on. This approach also makes it impossible to solve a number of fairly basic cases that were solvable before (as illustrated in this bug). This is due to embedding of parent destination paths into the referenced project. Flexibility is lost. I would recommend that at the earliest opportunity (ideally for Helios SR1 as this problem is rather serious), the new "Referenced Projects Classpath Entries" and "Classpath Container" facilities be removed from the UI. In their place there should be a single "Build Path Entry" option which lets the user pick from _existing_ classpath entries. The selected entry is tagged and shown in the same page along with other entries. When the entry from the table is removed, the tag is removed. There is no reason to have another tab for this. I am finding it difficult to understand what you want. Are you asking for a new (empty / shell / wrapper) IVirtualReference that says "Build Path Entry" which would serve to allow you to tag or untag the .classpath entries? How is this any different at all than the wizard page that's there now on "Referenced Projects Classpath Entries", which, functionally, does exactly the same thing by providing a UI to check or uncheck which entries have the flag in the .classpath file? What difference are you looking for? A word change? What? Also, if you're asking for these actual classpath entries to be presented in the viewer, I am not sure that's possible, as they are not IVirtualReferences of any sort and so far only IVirtualReferences can be shown here. Any references shown in this view are, naturally, things that would go INSIDE the deployment in question, and not things that will be "pushed up" to live "next to" the deployment. If it will live inside the .war file, it should be displayed here. If it will live outside of the web (in the ear, etc), then it should *NOT* be shown in this view. Showing things that will live outside the web project in the web project's assembly viewer would be very confusing and counterintuitive. So I'm still not sure exactly what you're asking for. Rob, let me try and "translate" here :) Konstantin is looking for a way with the new UI to do the thing that were possible in Galileo; show him how that is done in the current UI (if possible) and Konstantin can point out what is missing there. The fact that everything is virtual references is all nice technically, but if the users can't solve what they could do before then we have failed in doing the migration and need to fix it. > How is this any different at all than the wizard page that's there now on > "Referenced Projects Classpath Entries", which, functionally, does exactly the > same thing by providing a UI to check or uncheck which entries have the flag in > the .classpath file? What difference are you looking for? A word change? What? The Helios UI inverted the configuration, which has led to a lot of confusion. The user should not be going to a consuming project to configure classpath entry tags on the consumed project. That is very confusing and leads to an erroneous assumption that each consumed project can configure this separately. It is further confusing that after performing this step, the consumed project lists a "Pull in classpath elements from referenced projects" entry in its Assembly page which performs no function as that already happens for projects without this entry. In Galileo, the user configured what's exported by a project in that project's module dependencies page. When that project was consumed, the user saw the entries that came along with the project in the consuming project's module dependencies page. > Showing things that will live outside the web project in the web project's > assembly viewer would be very confusing and counterintuitive. Just show "../" in the Deploy Path. Most developers already understand what that notation means. Here are some of the other major bugs in this area for x-reference: Bug 320411 - new deployment assembly ui packs jars in jars Bug 319654 - utility project -> classpath container scenario appears to be broken Bug 319650 - classpath tagging and issues with new container reference system I'm changing target to Helios SR1 (WTP 3.2.2) just as a matter of bookkeeping, since 3.2.1 is done. Could we come up with another name to avoid confusion ? For me being able to *simply* add to a project all entries from related projects makes perfect sense - not all aggregating projects will be EAR projects. Renaming this option is not going to achieve anything tangible. I don't understand how we can expect users to understand that in order to affect properties of project A, they need to go to project B. What if there is no project B (user is building a library for someone else to consume)? What if there is a project C that also consumes A (why does manipulating UI in project B affects project C)? Let's stop trying to rationalize everything around the .component file which lead to this unnatural inversion and deal with assembly problem as a whole regardless of which file the metadata is stored in. (In reply to comment #15) > Renaming this option is not going to achieve anything tangible. I don't > understand how we can expect users to understand that in order to affect > properties of project A, they need to go to project B. What if there is no > project B (user is building a library for someone else to consume)? What if > there is a project C that also consumes A (why does manipulating UI in project > B affects project C)? Yes, that needs to be fixed. > Let's stop trying to rationalize everything around the .component file which > lead to this unnatural inversion and deal with assembly problem as a whole > regardless of which file the metadata is stored in. The option that is there now does not assume WTP should be messing with anything concerning classpaths etc.; it is just about the assembly nothing else. Its the "old" ones that has this problem of "going back and forth" - that we need to fix. Its two different non-exclusive usecases IMO. Using the .component file is the only possible way today to have some assembly that is not bound to specific server adaptors or project types. Lets have that meeting next week - i'll be on PTO the 2-3 weeks after. > Using the .component file is the only possible way today to have some assembly > that is not bound to specific server adaptors or project types. I am not sure what you are referring to. Classpath tags don't depend on specific server adapters or project types. Any server adapter that uses standard assembly traversal API will see contributions from classpath tags. > Lets have that meeting next week - i'll be on PTO the 2-3 weeks after. What time? Chuck? Carl? Some additional problems I found with "referenced project classpath entries" 1. Existing classpath tags are not shown in the deployment assembly page (only those added from the "referenced project classpath entries" wizard option) 2. Entries added to a web project do not have a default deployment path of "WEB-INF/lib" 3. Classpath entries for ALL projects should not show as valid entries to add from the assembly deploy page (should be limited to scope of EAR) 4. The EAR assembly deploy page should not show that it has pulled in entries from a module that is not part of the EAR 5. When an entry is added from the module assembly page a component file entry of type consumes is added to the module component file. However, when adding the same one from the EAR this entry is added to the EAR's component file, which is wrong. This in turn causes a duplicate "pull in classpath entries" entry in the EAR's deployment assembly page As mentioned on our call we are going to be addressing these issues by creating a new wizard that will handle the adding of classpath entries (and removing this one). Konstantin will be working on the new wizard. The new wizard will most likely not be reading or writing to the component file. Having similar "Deployment Assembly" page now showing error with the EAR/WEB/EJBModule Projects with a zip link from this forum page: http://forums.java.net/jive/thread.jspa?messageID=480703񵖿 using GlassFish as a server target. People are have issues there, more and more. My Eclipse log with these projects now contain many of these: !ENTRY org.eclipse.jface 4 2 2010-08-20 09:25:57.172 !MESSAGE Problems occurred when invoking code from plug-in: "org.eclipse.jface". !STACK 0 java.lang.NoClassDefFoundError: org/eclipse/wst/common/componentcore/ui/propertypage/IReferenceWizardConstants$ProjectConverterOperationProvider at org.eclipse.jst.servlet.ui.internal.WebModuleDependencyPageProvider.createPages(WebModuleDependencyPageProvider.java:53) at org.eclipse.wst.common.componentcore.ui.propertypage.ModuleAssemblyRootPage.createContents(ModuleAssemblyRootPage.java:161) at org.eclipse.jface.preference.PreferencePage.createControl(PreferencePage.java:232) at org.eclipse.wst.common.componentcore.ui.propertypage.ModuleAssemblyRootPage.createControl(ModuleAssemblyRootPage.java:183) at org.eclipse.jface.preference.PreferenceDialog.createPageControl(PreferenceDialog.java:1501) at org.eclipse.jface.preference.PreferenceDialog$14.run(PreferenceDialog.java:1258) Maybe my config is not clean, (I will try from a fresh build), but.... my install was not clean. Remove my last comment. Per discussion at the Java EE team meeting two weeks ago, the general approach that we agreed to is as follows: 1. Remove classpath container and inverted classpath tagging virtual components from the new assembly directive wizard. These components will still be supported at the core level since they shipped in Helios GA, but there will not be any way for the user to add them. 2. Add a new "Java Build Path Entries" wizard to the assembly directives wizard. This wizard will show available build path entries that can be included in the assembly. Once included, the build path entries will be shown along with other assembly directives in the deployment assembly table. Note that build path entry assembly directives will be serialized as classpath entry tags in the .classpath files rather than as content in the .module file. The above approach generally resolves a number of issues filed separately. I am turning this into the overall bug and will dupe other individual items against it. *** Bug 323153 has been marked as a duplicate of this bug. *** *** Bug 320411 has been marked as a duplicate of this bug. *** *** Bug 319654 has been marked as a duplicate of this bug. *** *** Bug 319650 has been marked as a duplicate of this bug. *** Created attachment 177144 [details]
Patch v1
Ok. Made some progress on this. Patch v1 includes the following: 1. Functional "Java Build Path Entries" wizard (from UI perspective). The available entries are displayed (lib, var and container type only). The container entries are filtered based on the existing extension point. Note that you cannot complete the wizard. That part isn't there yet. 2. Removed the wizards for classpath container virtual component reference and the inverted classpath tagging thingie. 3. Massive amount of UI cleanup all across the Deployment Assembly feature. Everything from setting banner images on all the wizards, to fixing margins, to setting proper widths on buttons, to improving the labels and messages. I wouldn't try reading through this patch. There are far too many changes. I encourage everyone with some spare cycles to load it up and provide feedback on how it feels. For the next phase I will tackle the following: 1. Getting the Java Build Path Entries wizard to actually do something. This includes showing tagged build path entries in the Deployment Assembly table. I think the approach that I am going to take there is to shift more work to the content provider. Since the content provider can be overriden by a subclass of Deployment Assembly page, we can have a custom one in Java EE land that knows about classpath tags. 2. The extension point used for registering deployment assembly directive wizards needs to support enablement expressions. In particular, I need to be able to preclude Java Build Path Entries option from showing up in non-java projects. 3. Need to locate better wizard banners for the first page in the add directive wizard and for the add manifest entries wizard. *** Bug 322542 has been marked as a duplicate of this bug. *** Created attachment 177158 [details]
Patch v2
Version 2 adds support for enablement expressions to referenceWizardFragment extensions point. This makes it possible to filter out Java Build Path Entries option from non-java projects.
Created attachment 177218 [details] Deployment Assembly display This patch is independent of the patch Kosta attached. I was working independently on bug 323153 before it was duped against this bug. This patch displays all tagged classpath entries. The UI allows the user to remove the entries and change the deployment paths. I'm not sure how this will collide with the above patch. Next step for me is to pull in the above patch, but before I did that I wanted to save off the progress I made, which I also encourage people to pick up and try. Likely these two patches will need to be integrated together. Created attachment 177222 [details]
Icons
Icons to go with the above patch
Took a quick look at your patch, Kosta; looks good. The good news is that it seems like it would be relatively simple to merge both of our patches. If the AddJavaBuildPathEntriesWizardFragment.performFinish() updated the J2EEModuleDependenciesPropertyPage.currentClasspathEntries to include the newly selected IClasspathEntries (and force a refresh of the page if necessary), that should do the trick. The J2EEModuleDependenciesPropertyPage already handles reading and writing the .classpath file independently of the IVirtualComponent API, so add/remove/modify/apply/revert already are fully working once the IClasspathEntries make it into the J2EEModuleDependenciesPropertyPage.currentClasspathEntries list. One thing I'm not sure we should do during 3.2.x is rename 'Add a Variable Reference', 'Add an Archive Reference', and 'Add an External Archive Reference'. I'm worried these name changes may confuse existing users. Also, depending on the direction we take this into 3.3, all of these options may be going away entirely in favor of the mechanism we're exposing with this bug. Thanks, Jason. I will take a look at merging the patches.
> One thing I'm not sure we should do during 3.2.x is rename 'Add a Variable
> Reference', 'Add an Archive Reference', and 'Add an External Archive
> Reference'. I'm worried these name changes may confuse existing users. Also,
> depending on the direction we take this into 3.3, all of these options may be
> going away entirely in favor of the mechanism we're exposing with this bug.
What Jason is referring to here is the label changes for three assembly directive wizards:
Original -> New
Archive -> Archives from Workspace
External Archive -> Archives from File System
Variable -> Archive via Path Variable
I made these changes for usability reasons:
These three logically perform very similar function, yet their labels don't make that clear. This is especially confusing for the "Variable" option. What is the user actually adding? It is not immediately clear from the label. The original labels came from JDT build path UI, but in that case, there is an additional context that is established by the libraries tab they are found under. We don't have a similar context to clarify these for the user. Further, once I added sorting to the list of choices, these three separated from each, which is not desirable.
In any case, I think these labels clarify the selection, but if others feel strongly that they should remain as they are in 3.2.x, then I would be fine with that. The usability problem I was addressing with that particular change isn't that severe.
Created attachment 177239 [details]
Patch v3
Merged mine and Jason's patches. The Java Build Path Entries wizard now hands-off selected entries to the page for display and further processing. The wizard also now further filters the available items based on what's currently displayed in the page.
Tweaked the display the of the build path entries in the assembly page:
1. Switched to using folder icon for the deploy path since the deploy path for tagged classpath entries is always a folder.
2. Switched to using post-fix notation (filename first, then path) for jar entries. This makes it easier to view the entries when paths are long and it is consistent with how JDT displays these.
At this point, this patch if fully functional and I expect only minor changes to be necessary.
All parties involved in the discussion of this feature should take a hard look at this patch and be ready to discuss it as this week's Java EE team meeting. We should aim to raise it for PMC approval shortly after the meeting.
Created attachment 177242 [details]
Patch v3
Oops... wrong patch.
Forgot to note... The images zip that Jason attached is no longer necessary as all of these images are exported via API by JDT. Patch v3 references these accordingly. Created attachment 177274 [details]
Patch v4
This patch builds on Kosta's merged v3 patch to provide tree functionality to display the classpath tagged attributes exported up to the parent. See screenshot.
Created attachment 177275 [details]
Util Exporting Dependencies
Created attachment 177276 [details]
Ear consuming Util
Created attachment 177277 [details]
War consuming Util
So I am thinking that the two columns in the deployment assembly UI should swap places. You can call it a left-to-right reading bias, but I keep wanting to first read what is being acted upon and then read where it is being placed. I think swapping the two columns would also make Jason's changes to display tag-along entries easier to read. The expand button would be next to the thing contributing all the entries and the target paths would all be nicely aligned in the second column whether they were contributed directly or indirectly. I also keep activating the cell editor for the deploy path unintentionally. This would largely be resolved by swapping the columns. I think (at least in the left-to-right countries), people will tend to click towards the left side of the table when selecting. The unintentional activation problem can also be handled by switching cell editor activation to double-click. I am for switching the columns as well. Good idea. I was also thinking about swapping the columns while converting it to a tree view. I'll code that up and post another patch with screenshots. Does anyone else have thoughts on swapping the columns? Created attachment 177338 [details]
Wizard Banner Image for Add Assembly Directive Wizard
Courtesy of our UI guy, Troy Beecroft.
Created attachment 177339 [details]
Wizard Banner for Add Manifest Classpath Entry Wizard
Courtesy of our UI guy, Troy Beecroft.
Jason, Could you add these wizard banners to the patch along with the changes you are currently working on? Alternatively, let me know when your revisions are done and I will add them. Created attachment 177352 [details]
Patch v5
This version switches the columns so the source is on the left side and the deploy path is on the right.
Also fixed the revert button to handle tagged classpath entries (thought it was working before, but it wasn't). And fixed the enablement of the remove button so it is disabled when selecting tagged classpath entries that are being pushed up from projects (i.e. you can remove nodes that are children in the tree).
Kosta, I did not merge the wizard banners in. Could you please merge them in now?
Created attachment 177367 [details]
Patch v6
Applies cleanly to head now.
Created attachment 177377 [details]
Patch v7
Integrates wizard banner images attached earlier.
Created attachment 177378 [details]
Patch Resources
The image files to go with the patch.
*** Bug 267774 has been marked as a duplicate of this bug. *** These changes look great, and I have been testing them over the past couple days now. We can discuss details in the JEE status meeting today Created attachment 177535 [details]
Patch v8
Resolves merge conflict with latest in head.
Raising for PMC review for 3.2.2 due to the extent of UI changes and impact on localization.
* Explain why you believe this is a stop-ship defect. Or, if it is a "hotbug" (requested by an adopter) please document it as such.
The deployment assembly feature in Helios was a big step forward, but a number of usability and functional issues have since been identified that needed correcting. This has been tagged as a problem by users on forums and Oracle is tagging this as an adopter issue.
* Is there a work-around? If so, why do you believe the work-around is insufficient?
While most of these problems can be worked around by informing users to stay away from certain functional areas or to do some set of manual steps, the combined weight of usability issues necessitate addressing in 3.2.2.
* How has the fix been tested? Is there a test case attached to the bugzilla record? Has a JUnit Test been added?
* Give a brief technical overview. Who has reviewed this fix?
Various versions of this patch have undergone review and manual testing by multiple Java EE project committers.
* What is the risk associated with this fix?
Medium risk due to extent of the changes.
Should we respin to get into this week's build? It is supposed to be a released candidate (RC2) for Helios SR2. I am fine with trying to get this into this week's build. The more time adopters have to test it the better. How many PMC votes do we need now before releasing? Created attachment 177551 [details]
Patch v9
Adds proper logging for caught exceptions.
After discussion at today's WTP meeting, we decided to move forward with getting this into this week's 3.2.2 build. Committed Patch v9 and released to 3.2.2. stream. *** Bug 319651 has been marked as a duplicate of this bug. *** *** Bug 320936 has been marked as a duplicate of this bug. *** *** Bug 320939 has been marked as a duplicate of this bug. *** |