Adding/removing libraries from Java Build Path can go out of sync

[chanseokoh]

Steps to reproduce:

1. Create a new project.
2. Right-click on the project > Properties > Java Build Path > Libraries
3. Add Libraries... > Google Cloud Platform Libraries > Objectify > Finish
   • While you're still in the Libraries tab, you'll see that the master container has been added and it contains Objectify when expanded.
4. Add Libraries... > Google Cloud Platform Libraries > Google Cloud Endpoints > Finish
   • Note that, in the tab, you don't see Endpoints when expanding the container.
5. OK to exit.

Observations:

1. Endpoints is not added.
2. On the other hand, if you right-click on the master container > Properties, then Endpoints is checked.

I fear this might be tricky to fix.

[briandealwis]

I think this may be because I removed IClasspathContainerPageExtension2 from CloudLibrariesPage in #2553 as it didn't seemed to be necessary. In this case, although you clicked on Add Libraries…, we're modifying the existing entry as we only support one entry. ClasspathContainerWizard#performFinish() calls CloudLibrariesPage#getSelection() to return the new entry, but our getSelection() returns the edited classpath entry as the method is documented to return the new or edited entry. The one advantage of IClasspathContainerPageExtension2, which is relevant here, is that it has getNewContainers() which is documented to only return the new classpath entries and avoids getSelection().

[chanseokoh]

Closely related to this issue, there is a problem in how our library feature works.

When in the Libraries tab of the Java Build Path properties, adding/removing/modifying the master container should not take effect unless the user presses OK or Apply. This is true about adding and removing the container entry itself, but unfortunately, selecting and modifying the library list takes effect immediately (i.e., as soon as collectLibraryFiles() is called, the library selection list is serialized and persisted).

To demonstrate the problem,

1. Add Objectify to a project. Close any dialog to make the change permanent.
2. Properties > Java Build Path > Libraries
3. Double-click on Google Cloud Platform Libraries. You'll see Objectify already selected.
4. Select Endpoints > Finish to close our wizard page
5. Cancel to close the Properties dialog

You'll notice Endpoints JARs have been added, even though you canceled your changes.

[chanseokoh]

From a quick look, https://github.com/GoogleCloudPlatform/google-cloud-eclipse/issues/2688#issuecomment-353439221 seems tricky to make it right with the current single-master design where we serialize the container info in separate settings files. Clicking the Finish button in our wizard page means the library wizard is closing while calling performFinish(). From the perspective of the wizard, everything is done and its world is collapsing. Yet, if we want to fix the problem, we should outlive the destruction of the wizard and know the outside world about if the wizard was called inside the Properties page, which will probably require serious not-so-elegant hacks and postponing finalization of library changes only after Apply or OK was pressed on the Properties page.

With the single-master container design, the only way I can think of (for now) to fix all the issues, including https://github.com/GoogleCloudPlatform/google-cloud-eclipse/issues/2688#issue-283292814, is to save all the info about the container (the huge JSON we save about JARs and such) in the container entry itself, instead of saving it on the filesystem. That way, if the user reverts changes in Properties by clicking Cancel, all the changes will naturally be reverted with the intermediate classpath entry going away. I think theoretically we can save any info into a classpath entry in the form of classpath attributes (using JavaCore.newClasspathAttribute()). However, this means a complete overhaul of our current library framework. We need to give some thoughts on this.

[chanseokoh]

... the only way I can think of ... is to save all the info about the container ... in the container entry itself

Nah, the problem is that when doing Add Libraries..., whether doing so on Properties or not, the library wizard is really about adding a new entry rather than updating an existing classpath entry. So you'll end up seeing the "adding a duplicate classpath entry" error. Technically, the error makes sense in that you clicked Add Libraries... rather than Edit... (or opening the Properties of the classpath entry), but in our library flow, the error may look weird.

[chanseokoh]

Another problem I noticed, which feels almost like an Eclipse bug, when utilizing classpath attributes is that attributes aren't being updated when updating a classpath in the Libraries tab. On the other hand, they are properly updated, as expected, when ALT+F5 on the master container on the Project Explorer. Seems like only the container path can be updated reliably. We might be able to squash all library info into the container path, but that seems impractical, if not impossible, and a torture.

[chanseokoh]

Another problem I noticed, which feels almost like an Eclipse bug

FTR, I think the bug is (Neon code) in org.eclipse.jdt.internal.ui.wizards.buildpaths.LibrariesWorkbookPage.openContainerSelectionDialog() (or I could say the CPListElement constructor depending on who's responsible).

private CPListElement[] openContainerSelectionDialog(CPListElement existing) {
  ...
      IClasspathEntry created= BuildPathDialogAccess.configureContainerEntry(...);
      if (created != null) {
        CPListElement elem= new CPListElement(null, fCurrJProject, IClasspathEntry.CPE_CONTAINER, created.getPath(), ! created.equals(existingEntry), null, null);
        return new CPListElement[] { elem };

Basically, created will be an edited entry with updated attributes, but note that no one attempts to read the attributes (via created.getExtraAttributes()). created is a local variable to be discarded, and attributes are just lost. (In practice, what happens is that old attributes will survive and preserved; see what happens in LibrariesWorkbookPage.editElementEntry() after returning from openContainerSelectionDialog()).

Maybe the reason it is keeping old attributes and ignoring new attributes could be an unfortunately backward compatibility. That said, there is no way to update attributes through the Libraries tab. (However, again, updating attributes outside the tab does work, showing inconsistency.)

[chanseokoh]

This is quite tricky to fix, I think. What if we ditch the Eclipse way of adding libraries and rather have our own standalone library add wizard? That may be easier...

[elharo]

No custom wizards please. We want to integrate with eclipse, not replace it. And that's way more effort and time than we should be dedicating to this. Have you tried the change @briandealwis suggested above?

[chanseokoh]

That did not help, unfortunately. (Also, I hit a new error in another situation when using IClasspathContainerPageExtension2.)

Even if it did, it cannot fix https://github.com/GoogleCloudPlatform/google-cloud-eclipse/issues/2688#issuecomment-353439221, which is another problem that is different but closely related.

[chanseokoh]

We need to discuss how we could tackle both https://github.com/GoogleCloudPlatform/google-cloud-eclipse/issues/2688#issue-283292814 and https://github.com/GoogleCloudPlatform/google-cloud-eclipse/issues/2688#issuecomment-353439221. (Actually, the former problem already suffers from the latter problem. It is just that pressing Cancel shows the latter problem clearly noticeable, while pressing OK makes it moot.)

These issues are fundamentally due to our unusual scheme of a dynamically changing container (the single master container).

What happens with the steps in https://github.com/GoogleCloudPlatform/google-cloud-eclipse/issues/2688#issue-283292814 is that, the container is resolved via LibraryClasspathContainerInitializer.initialize() only when adding Objectify. It is not called when adding Endpoints. The reason is explained in org.eclipse.jdt.core.ClasspathContainerInitializer.initialize():

     * This method is called by the Java model to give the party that defined
     * this particular kind of classpath container the chance to install
     * classpath container objects that will be used to convert classpath
     * container entries into simpler classpath entries. The method is typically
     * called exactly once for a given Java project and classpath container
     * entry. This method must not be called by other clients.

In other words, it is the Eclipse framework that calls this method only when necessary. On the other hand, our dynamically changing container scheme requires this method be called (to be precise, the container be resolved) whenever the container is changed. I believe we don't have any control over how this Eclipse framework works, so there seems nothing much we can do here.

However, one thing to note is that, at least we can explicitly resolve the container by calling BuildPath.runContainerResolverJob(). So, for example, if we schedule this job indefinitely and periodically (which is a very dirty hack that might not really work in practice), we could at least fix or mitigate the original problem. Even so, this won't fix the second problem (https://github.com/GoogleCloudPlatform/google-cloud-eclipse/issues/2688#issuecomment-353439221) anyway.

Another thing to note is that, all the problem happens only when using our libraries wizard in the Libraries tab. If users do right-click on a project > Build Path > Add Libraries..., it is OK.

[elharo]

This is not that high a priority. I don't want to burn an indefinite amount of time on it. If there's no obvious quick fix here, our time would likely be better spent on the installer.

[lak-proddev]

I believe It's a critical bug and this operation should be in an asynchronous thread. Since the UI is blocking while adding a new library. This is my initial impression only. I need to check more details about the bug and code base.

[lak-proddev]

while modifying the libraries for Non-Maven project

[briandealwis]

@lak-proddev This issue is specifically around inconsistencies in using the project Build Path properties to add GCP libraries to the class path. Sync/async issues should be opened as a new issue. That said, library resolving and attaching is now async with #3465. I see a lot of build-path related jobs appear even with a single project, so I don't

[chanseokoh]

@lak-proddev #3465 is merged recently (not released yet). Is the screenshot on recent master (HEAD)? That is, is it after #3465 or not?

[lak-proddev]

Yes, It's after #3465 (commit 00014d662b6240b9d4bb93af71ffbd7614d5492c). The change included in my workspace.

[briandealwis]

@lak-proddev I meant to add: if you were inquiring about the number of build path jobs, we attempt to download the source jars for each of the resolved dependencies in a separate job. So it's not unexpected to see a lot of jobs fired up.

I wonder if we need to do this as m2e provides a source connector that examines the jars and, if a maven artifact, will automatically download and configure the source. I don't remember when this was introduced though.

[lak-proddev]

@lak-proddev I meant to add: if you were inquiring about the number of build path jobs ......

@briandealwis Yes, I just want to let u know. Since multiple jobs running parallel. Nothing more than that. Thanks :)

[SINGHPRT]

Adding to following section in .classpath file made it appear in Build path.

<classpathentry kind="con" path="com.google.cloud.tools.eclipse.appengine.libraries/master-container">
        <attributes>
            <attribute name="maven.pomderived" value="true"/>
            <attribute name="org.eclipse.jst.component.dependency" value="/WEB-INF/lib"/>
        </attributes>
    </classpathentry>

[JoeWang1127]

close as not planned