Community
Participate
Working Groups
The INDEX_LOCATION_ATTRIBUTE_NAME classpath attribute can be used to explicitly specify the location for the index file. However, this is currently used only for the pre-built indexes. When index has to be rebuilt, the index is stored in the default location and not as per the attribute value. When the index location is specified, the index should always be created there.
An IBM adopter product is interested in having this as we want to make use of the pre-build index functionality; however, the contents of the JARs for our classpath container can be modified by our clients (without us knowing). As a result, we'd need to have the ability to have them regenerated (if needed) in the same location as specified by the INDEX_LOCATION_ATTRIBUTE_NAME classpath attribute. With that said, I'd like to ask that this be included in the 4.2.2 release. Attached to this bug is a patch which fixes this problem but only if a product preference is specified and set to true; otherwise, it keeps the existing functionality that is there today. If the product preference JDT_MANAGE_MY_INDEXES is specified and set to 'true' then the indexer will regenerate the indexes into the location specified by the INDEX_LOCATION_ATTRIBUTE_NAME classpath attribute. Maybe in 4.3 it would be best to make this the default (i.e. remove the product preference and always generate to the index location) but in 4.2.2 it's understandable if you do not want to change the default behaviour which is why I introduced the product property.
Created attachment 224383 [details] possible patch
(In reply to comment #2) > Created attachment 224383 [details] > possible patch Running the JDT/Core tests results in an NPE, because the following returns null: Platform.getProduct().getProperty("JDT_MANAGE_MY_INDEXES") Can we fix it not to fail when the property doesn't exist?
(In reply to comment #3) > Can we fix it not to fail when the property doesn't exist? Sorry, I meant to ask Troy if he can fix that.
Created attachment 225343 [details] updated patch sorry, that was a silly mistake :) I've fixed the patch so the property is initialized in the constructor of IndexManager instead so that a null check can be performed. I also renamed the product property to indicate that it is for updating product provided indexes only as we do not want JDT to delete these indexes when/if the corresponding JAR is removed from the build path. I'm not sure if JDT will do that (with this patch applied) so I'll debug a bit more to confirm.
(In reply to comment #5) > sorry, that was a silly mistake :) I've fixed the patch so the property is > initialized in the constructor of IndexManager instead so that a null check > can be performed. I also renamed the product property to indicate that it > is for updating product provided indexes only as we do not want JDT to > delete these indexes when/if the corresponding JAR is removed from the build > path. I'm not sure if JDT will do that (with this patch applied) so I'll > debug a bit more to confirm. Actually, my bad too. I didn't fully test the patch. It's the Platform.getProduct() that is returning null since the tests are run as a headless application and not a product, which makes me think that we might run in to similar problems in other scenarios too. I think we should keep what you had written in one of your previous patches - Boolean.getBoolean(JDT_UPDATE_PRODUCT_INDEXES) What do you think?
Created attachment 225398 [details] patch using jdt.core preference Sorry, I didn't realize that Platform.getProduct() could return null... I should have read the javadoc more carefully :) Anyway, given that we cannot use a product preference, how do you feel about a "hidden" jdt.core preference (by hidden I mean one that is not changeable via the UI). If we go with this approach then products building on Eclipse can define (based on the patch supplied): org.eclipse.jdt.core/org.eclipse.jdt.core.index.updateProductIndexes=enabled in their products plugin_customization.ini file if they want the function enabled. This approach is a little cleaner than the system property approach. What do you think? A few notes about the patch: 1) It's not possible to query the jdt.core preferences via JavaCore.getOption(String) in the constructor of IndexManager as the JavaModelManager is not initialized yet. That is why the value of the preference is determined in the new method isManageUserIndexes. 2) The new method isManageUserIndexes is marked as synchronized as I was not sure about threading. I suppose it's possible to have multiple indexers running at once so I wanted to make sure there wasn't any issue when initially setting the manageUserIndexes flag. 3) I don't completely understand the Eclipse versioning for the 4.2 release (is it meant to be equal to 3.8?). The preference constant added to the JavaCore class called UPDATE_PRODUCT_INDEXES has been marked as @since 3.8 - that may need adjusting to the right value if 3.8 is wrong.
the changes for the indexing logic are not correct in this latest patch... I need to debug further; however, would the enablement logic be OK?
(In reply to comment #7) > Created attachment 225398 [details] > patch using jdt.core preference > ... > What do you think? +1 to that. This also means if and when we want to provide a UI option, it will be lot easier and consistent with other options. > A few notes about the patch: > .... > 3) I don't completely understand the Eclipse versioning for the 4.2 release > (is it meant to be equal to 3.8?). The preference constant added to the > JavaCore class called UPDATE_PRODUCT_INDEXES has been marked as @since 3.8 - > that may need adjusting to the right value if 3.8 is wrong. In JDT/Core we still use the 3.x versioning, which means the @since should be 3.9. The rest of the points look alright to me.
(In reply to comment #9) > In JDT/Core we still use the 3.x versioning, which means the @since should > be 3.9. The rest of the points look alright to me. Sorry, since this is going in for 4.2.2 (3.8.2), it should be 3.8.2. Dani, please correct me if I am wrong.
I agree it is too risky for SR2 to regenerate the index into INDEX_LOCATION_ATTRIBUTE_NAME by default but for 4.3 we should do this. Hence adding a public constant to SR2 is wrong (and would need PMC approval). In SR2 we can enable it either via preference which is not published as constant or via system property. And to answer your question: the correct @since version would have been 3.8.2.
(In reply to comment #8) > the changes for the indexing logic are not correct in this latest patch... I > need to debug further; however, would the enablement logic be OK? Hi Troy, did you mean you are going to come up with a new patch? Also, as Dani said, if we are going to make this is the default behavior at some point, we don't have to spend a lot of time ways to pass the preference. The simplest option would be via a system property.
(In reply to comment #12) > (In reply to comment #8) > > the changes for the indexing logic are not correct in this latest patch... I > > need to debug further; however, would the enablement logic be OK? > > Hi Troy, did you mean you are going to come up with a new patch? Yes, that is right. I'm still working on the proper patch.
Created attachment 225796 [details] updated patch This patch is a different attempt at fixing this problem. I am still using the jdt.core preference mechanism for enablement, but that can be easily changed to a system property if that is what you want. Anyway, an overview of the change is that I added a new internal method to IndexManager: public void indexLibrary(IPath path, IProject requestingProject, URL indexURL, final boolean forceUpdate); This method is only called by the delta processor when it notices that the timestamp between the old and new index are different. It then passes flag to AddJarFileToIndex which uses this value (or the state of indexFile.exists()) to determine if it should write the index to the existing index pre-specified location. This is a big change, but that is because of rather significant changes to AddJarFileToIndex. With that said, the change to AddJarFileToIndex is really just refactoring the existing code into a method so it can be re-used. I have done some basic testing with this patch and it seems to work fine for me (for pre-built indexes and normal indexes that go into the jdt metadata), but I suspect it would need more comprehensive junit tests run against it to be sure it is OK.
Additional note: this defect is blocked by bug 397818 in situations where the pre-build indexes reside in a directory with spaces.
Sorry but that patch is way too big for RC3. Something along the lines of the previous patch with options replaced by a system property could still be considered.
(In reply to comment #16) > Sorry but that patch is way too big for RC3. Something along the lines of > the previous patch with options replaced by a system property could still be > considered. The previous patch was functionally incorrect... I honestly don't know how it worked the first time I tried it. The latest patch is the only possibility I can see for making this situation work. I can reduce it a bit by using a system property instead of a preference, but understand that the size of the patch is purely because of the refactoring done to AddJarFileToIndex. I moved code from execute() to a new method as it needed to be common (for use in a different situation).
(In reply to comment #17) > (In reply to comment #16) > > Sorry but that patch is way too big for RC3. Something along the lines of > > the previous patch with options replaced by a system property could still be > > considered. > > The previous patch was functionally incorrect... I honestly don't know how > it worked the first time I tried it. > > The latest patch is the only possibility I can see for making this situation > work. I can reduce it a bit by using a system property instead of a > preference, but understand that the size of the patch is purely because of > the refactoring done to AddJarFileToIndex. I moved code from execute() to a > new method as it needed to be common (for use in a different situation). Yes, please try to attach a patch with minimal changes. We can decide again then. Otherwise we have to defer it to 3.8.2+.
Created attachment 225915 [details] updated patch This patch relies on the system property mechanism for enabling this function, rather than a preference. This was done to reduce the size of the patch. I've also removed the patch for bug 397818 from this (even though it's required) to ensure the patch is as small as possible. The patch is still large; however, that is because of the refactoring done in AddJarFileToIndex so that there is no duplicate code in this class. If you feel the patch is still too large then I can undo the refactoring and add in the duplicate code.
Created attachment 225923 [details] Failing tests (In reply to comment #19) > Created attachment 225915 [details] > updated patch I haven't looked at the patch in details, but couple of existing tests are failing. I am attaching the list. I will continue to look at the failures and review the patch.
(In reply to comment #20) > > I haven't looked at the patch in details, but couple of existing tests are > failing. I am attaching the list. I will continue to look at the failures > and review the patch. Looks like with the refactoring we are not setting the URL for a JAR index in one of the scenarios that is resulting in an NPE. Troy, can you please look at the failures? I have to say I would prefer a simpler patch without the refactoring. The other option, perhaps a better one, is to move it out of 3.8.2.
Created attachment 225953 [details] updated patch Sorry about that... there's a lot I don't know about the index manager which is why these problems pop up. I didn't realize the implementation of JarIndexLocation#getIndexFile() returned null. Anyway, I've attached an update patch which fixes this problem. I also ran the org.eclipse.jdt.core.tests.model.JavaIndexTests JUnit (which I also didn't know about) and they all pass now using this latest patch. I also wish I did not need to do the refactoring to AddJarFileToIndex but due to the way the execute() implementation was previously structured it was needed to be done to reduce the amount of duplicate code. I'll keep looking into this a bit more to see if there is a better way.
I looked at the latest patch and it's not ready for 4.2.2. Besides being too big and too complicated to be released at this point, I found several small issues while doing a quick review only: - code to get system property is too complicated. A private static final filed with Boolean.getBoolean("JDT_UPDATE_PRODUCT_INDEXES") is all that's needed - createOrUpdatePreBuiltIndex() declares to throw an IOE but it already catches it inside the method body, so there will never be an IOE thrown - forceUpdate is not the right parameter name: it should better be updateIndex (the new parameter that's passed through should be the same everywhere) And of course we should also add some new tests.
(In reply to comment #23) > I looked at the latest patch and it's not ready for 4.2.2. No problem, I understand. I'm going to re-write the patch so that it uses the preference mechanism instead of the system property as it's easier to manage a preference (i.e. using the plugin_customization.ini) within our product than a system property. I'm assuming the preference mechanism is OK (based on comment 11)? > Besides being too big and too complicated to be released at this point, I > found several small issues while doing a quick review only: > - code to get system property is too complicated. A private static final > filed with Boolean.getBoolean("JDT_UPDATE_PRODUCT_INDEXES") is all that's > needed > - createOrUpdatePreBuiltIndex() declares to throw an IOE but it already > catches it inside the method body, so there will never be an IOE thrown > - forceUpdate is not the right parameter name: it should better be > updateIndex > (the new parameter that's passed through should be the same everywhere) > > And of course we should also add some new tests. I'll work on these in the next patch update.
(In reply to comment #24) > (In reply to comment #23) > > I looked at the latest patch and it's not ready for 4.2.2. > > No problem, I understand. I'm going to re-write the patch so that it uses > the preference mechanism instead of the system property as it's easier to > manage a preference (i.e. using the plugin_customization.ini) within our > product than a system property. I'm assuming the preference mechanism is OK > (based on comment 11)? Using the preference *mechanism* would be OK, but not as public constant/API, since in 4.3 we would just use the new code/behavior. Of course using the preference mechanism makes the patch bigger again. Note that we are not less conservative with 4.2.2+ fixes than we are with 4.2.2 ones. Having said that, if it makes your life much easier, then we will accept this.
Created attachment 226433 [details] updated patch Attached is an updated patch which simplifies the changes a lot - as a result, the patch is much smaller (about 1/2 the size of the previous one). I also changed the mechanism to enable this function back to a hidden preference. I've also verified that it does everything I'm looking for in this bug report. Disclaimer: I am not able to successfully run all of the JavaIndexTests 100% of the time with and without this patch (although it seems to be worse with the patch). It seems to be threading related as the failing testcase is always different. If I run each test separately then it always succeeds... it's only when I run them all at once then random ones fail (and the order that they run in is always the same). I'm going to continue to investigate this a bit but if you have any information that may help guide me in the right direction it would be greatly appreciated (I've been looking at this for multiple days now).
(In reply to comment #26) > Created attachment 226433 [details] > updated patch Do we still need the JavaCore option? We can't be adding new API now, I suppose? It would be great if you can add some new tests or help me with writing some. > > Disclaimer: I am not able to successfully run all of the JavaIndexTests 100% > of the time with and without this patch (although it seems to be worse with > the patch). All test in JavaIndexTests pass consistently with the patch on my machine.
Created attachment 227564 [details] junit patch patch to test the fix for this defect.
(In reply to comment #27) > (In reply to comment #26) > > Created attachment 226433 [details] > > updated patch > > Do we still need the JavaCore option? We can't be adding new API now, I > suppose? It would be great if you can add some new tests or help me with > writing some. Ideally, yes, I would like to have he preference. It makes it a lot easier for us to manage the preference in our Eclipse adopter product than with the system property. Adding a new JavaCore option (API) shouldn't affect any consumers of jdt.core. Patch attached for the test case. > > > > Disclaimer: I am not able to successfully run all of the JavaIndexTests 100% > > of the time with and without this patch (although it seems to be worse with > > the patch). > > All test in JavaIndexTests pass consistently with the patch on my machine. Good to hear... I thought it was rather odd that they were (randomly) failing for me.
Created attachment 227624 [details] updated patch This is a minor update to the previous patch which does: 1) nulls out the Boolean flag 'manageUserIndexes' so that this function can be properly tested via JUnit; otherwise, the JUnit call to simulate restarting the product will not reset the value of this flag if the preference has been changed (needed for the previously attached JUnit testcase patch). 2) Adds a check to IndexManager#addIndex() to ensure that it does not try to re-add a pre-built index that had already been added before (found a scenario during testing).
(In reply to comment #30) > Created attachment 227624 [details] > updated patch I apologize for the delay. Here are some points on the patch: 1. The attached junit never managed to go through the scenario where the local variable forceIndexUpdate inside IndexManager#indexLibrary() is true. 2. When IndexManager#rebuildIndex is called, the null indexFileUrl being passed is forcing the state to REBUILDING_STATE and forcing the index update regardless of the preference setting. And this is demonstrated by the newly failing test - JavaIndexTests#testUseIndexInternalJarAfterRestart() 3. Method addPreBuiltIndex() is better named havePreBuiltIndex() or similar. Thanks for looking in to these.
Created attachment 228741 [details] updated junit patch This JUnit patch includes the following call: JavaCore.initializeAfterLoad(null); when simulateRestart() is called. This ensures that external archives are re-checked when the simlate*() methods are called. This fixes problem #1 mentioned in comment 31.
Created attachment 228742 [details] updated patch This is an updated patch for org.eclipse.jdt.core that fixes problems #2 and changes the method addPreBuiltIndex() to hasPreBuiltIndex() as requested in item #3 from comment 31.
(In reply to comment #33) > Created attachment 228742 [details] > updated patch > > This is an updated patch for org.eclipse.jdt.core that fixes problems #2 and > changes the method addPreBuiltIndex() to hasPreBuiltIndex() as requested in > item #3 from comment 31. Patch looks good apart from some extra white space and copyright updates. My previous concerns have been addressed as well. All index tests are passing. And rest of the tests are still running. I will update when the tests complete. Dani, would you like to have a look at the patch once?
I decided to have another look at the patch and I have been staring at this code for some time: if(!newIndexURL.equals(existingURL) || ((existingURL != null) && !existingURL.equals(newIndexURL))) { ... Is existingURL.equals(newIndexURL) likely to produce a different result than newIndexURL.equals(existingURL) ?
The tests revealed one problem with the fix. JavaSearchBugsTests#testBug323514a() and testBug323514b are failing. We probably need this code in IndexManager#indexLibrary(): boolean forceIndexUpdate = this.isManagingPreBuiltIndexes() && updateIndex; IndexLocation indexFile = forceIndexUpdate ? computeIndexLocation(path, indexURL) : ((indexURL != null ? IndexLocation.createIndexLocation(indexURL): null)); I am not sure if this fix is enough, though. Troy, can you please take a look at this? Thanks!
(In reply to comment #35) > I decided to have another look at the patch and I have been staring at this > code for some time: > > if(!newIndexURL.equals(existingURL) > || ((existingURL != null) && !existingURL.equals(newIndexURL))) { > ... > > Is existingURL.equals(newIndexURL) likely to produce a different result than > newIndexURL.equals(existingURL) ? The second part of that if statement clause can be returned: if(!newIndexURL.equals(existingURL)) { ... } is sufficient for this test.
(In reply to comment #36) > The tests revealed one problem with the fix. > JavaSearchBugsTests#testBug323514a() and testBug323514b are failing. We > probably need this code in IndexManager#indexLibrary(): > > boolean forceIndexUpdate = this.isManagingPreBuiltIndexes() && updateIndex; > IndexLocation indexFile = forceIndexUpdate ? computeIndexLocation(path, > indexURL) : > ((indexURL != null ? IndexLocation.createIndexLocation(indexURL): > null)); > > I am not sure if this fix is enough, though. Troy, can you please take a > look at this? Thanks! I think a simpler fix would be: IndexLocation indexFile = null; if(indexURL != null) { indexFile = computeIndexLocation(path, indexURL); } and in computeIndexLocation(IPath, URL) this condition should be removed: else { indexLocation = this.computeIndexLocation(containerPath); // cache updating is done in #computeIndexLocation(path) } I've tried this out in my own environment but I continue to have unreliable results (I don't understand why) so I'm not sure if the solution solves all problems.
Created attachment 228901 [details] Updated patch Updated patch with the previous changes discussed. The patch also includes another fix I made in IndexManager#removeIndex(). Troy, can you please take a look at the changes and also see if your test cases are working alright? All index/search tests pass and rest of the tests are still running. Will update once they complete.
(In reply to comment #39) > All index/search tests pass and rest of the tests are still running. Will > update once they complete. All tests are green.
Created attachment 228976 [details] mkdirs on directory holding the index I'm fine with the patch as-is (all my testing is good) but I'd also like to propose the following patch to FileIndexLocation to ensure that the directory holding the index exists before attempting to writing the index file. I can file that as a separate defect if you like for this request.
(In reply to comment #41) > Created attachment 228976 [details] > mkdirs on directory holding the index > > I'm fine with the patch as-is (all my testing is good) but I'd also like to > propose the following patch to FileIndexLocation to ensure that the > directory holding the index exists before attempting to writing the index > file. I can file that as a separate defect if you like for this request. Do you want this for 3.8.2+ ? Even though this is an existing issue, since it is also relevant to this bug and can affect our testing (none of the current tests fail, though) I am okay with including this here. Dani, I think it's safe to say you can review the patches now.
(In reply to comment #42) > (In reply to comment #41) > > Created attachment 228976 [details] > > mkdirs on directory holding the index > > > > I'm fine with the patch as-is (all my testing is good) but I'd also like to > > propose the following patch to FileIndexLocation to ensure that the > > directory holding the index exists before attempting to writing the index > > file. I can file that as a separate defect if you like for this request. > > Do you want this for 3.8.2+ ? > Yes please, if possible, as it is very relevant to this scenario.
(In reply to comment #41) > Created attachment 228976 [details] [diff] > mkdirs on directory holding the index > > but I'd also like to > propose the following patch to FileIndexLocation to ensure that the > directory holding the index exists before attempting to writing the index > file. I can file that as a separate defect if you like for this request. It's OK to ensure that the path exists, but the current proposed patch can result in semantic differences, e.g. it will return 'false' instead of throwing an IOE if anything goes wrong. ==> make sure this.indexFile.createNewFile() is always called. The variable names 'indexDirectory' and 'parentDirExists' are misleading. Either use "index" or "parent" and "Dir" or "Directory" for both variables. The patch also lacks a copyright update.
(In reply to comment #42) > Dani, I think it's safe to say you can review the patches now. I started to review the patch and I still don't like to see the preference being introduced as new API. Is this really something that a client (or product) is supposed to change during *runtime* (if so, we must also handle/document what happens if the value gets changed during runtime)? If I understood it correctly, we would always want this enabled except - not for 3.8.2+ and 4.3 since too late - if a problem arises in production and we need to back out the change Please clarify. If the API is not needed, then a system property would be preferred (can e.g. be set in configuration/config.ini or the start script). But I'd also accept if we introduce the preference, but not surface it as a public constant, if that really is much easier for your product.
Created attachment 228994 [details] mkdirs on directory holding the index updated patch based on comment 44
(In reply to comment #45) > (In reply to comment #42) > > Dani, I think it's safe to say you can review the patches now. > > I started to review the patch and I still don't like to see the preference > being introduced as new API. Is this really something that a client (or > product) is supposed to change during *runtime* (if so, we must also > handle/document what happens if the value gets changed during runtime)? If I > understood it correctly, we would always want this enabled except > - not for 3.8.2+ and 4.3 since too late > - if a problem arises in production and we need to back out the change > > Please clarify. No, I do not think this is meant to be something that is changed during runtime of a product. > If the API is not needed, then a system property would be preferred (can > e.g. be set in configuration/config.ini or the start script). But I'd also > accept if we introduce the preference, but not surface it as a public > constant, if that really is much easier for your product. After talking with our products installation team we can make it work with a system property. I'll update a new patch that removes the preference and defines a system property instead.
The other concern I have is in IndexManager.indexLibrary(...) where we now run much more/different code even if the new option is disabled: IndexLocation indexFile = indexURL != null ? IndexLocation.createIndexLocation(indexURL): null; gets replaced by computeIndexLocation(path, indexURL); i.e. instead of just creating the index location we do other/more stuff as well. I expect that with the new flag disabled, we run the (exact) same code as in 3.8.2. To proof, that the changed code would still exactly to the same thing as in 3.8.2, would take quite some time. So, please only call the new computeIndexLocation if the flag is enabled and the old code otherwise. The changes in AddJarFileToIndex are also not 100% the same if the flag is disabled, but in that case I can easily verify that they are OK. Though, I could argue, that e.g. the additional exists() check is either not required or a separate bug. But I'm fine leaving it as is. Some minor comments on the patch: - API Tools filter is missing on @since 3.9 tags and in the manifest - IndexManager.isManagingPreBuiltIndexes() is too complicated and not correctly indented - IndexManager.manageUserIndexes is overkill unless you explicitly wanted to prevent that a preference change is honored next time - AddJarFileToIndex.hasPreBuiltIndex(): this.indexFileURL != null does not need brackets around it - method calls should not be sent to 'this' That's all I have after going over the whole patch.
Created attachment 229073 [details] updated patch: index manager + JUnit + mkdirs call Updated patch to address concerns in comment 48. Some of my comments: * This patch contains the fix to FileIndexLocation in addition to the changes for the index manager. * I changed the logic in IndexManager#indexLibrary(...) to only make use of the new computeIndexLocation(path, indexURL) method with managing product indexes is enabled; otherwise, it uses the logic that was previously there. * I removed the preference and replaced it with a system property. I don't know if you have a naming convention so I went with jdt.manageProductIndexes. Feel free to rename it as you please. * The system property constant was defined as 'public' so that it can be re-used in the JUnit test. * I kept the method IndexManager#isManagingProductIndexes() so that the code does not constantly keep retrieving the system property value and casting it to a boolean. As well, the JUnit test can reset the value as needed so that we can test the enabling/disabling the function within the same JVM. Feel free to change this if you are OK with constantly querying the system property value each time this is called. * I did not change any of the project metadata - i.e. bundle version, API filter, etc. The attached patch is only code - please adjust this metadata as needed.
(In reply to comment #49) oh, I also ran all the previously mentioned JUnit tests (JavaIndexTests & JavaSearchBugsTests) and they were all green.
Created attachment 229088 [details] Troy's patch + some minor tweaks (In reply to comment #49) > Created attachment 229073 [details] [diff] > updated patch: index manager + JUnit + mkdirs call > > Updated patch to address concerns in comment 48. Thanks Troy. This new patch here applies my following comments. > Some of my comments: > * This patch contains the fix to FileIndexLocation in addition to the > changes for the index manager. Looks good, except for the superfluous brackets in if ((directory != null) && !directory.exists()) { Fixed that in this patch. > * I removed the preference and replaced it with a system property. I don't > know if you have a naming convention so I went with > jdt.manageProductIndexes. Feel free to rename it as you please. Renamed it to "jdt.core.manageProductIndexes" in this patch. > * I kept the method IndexManager#isManagingProductIndexes() so that the code > does not constantly keep retrieving the system property value and casting it > to a boolean. As well, the JUnit test can reset the value as needed so that > we can test the enabling/disabling the function within the same JVM. This is not good. See my previous comment 23. The field must be static. I don't want to pollute the code to ease tests. The patch fixes that. This change reveals a problem in the newly added test though: it still passes even though the property is 'false'. I would expect the test to fail in that case. Since the field is now static you need to set the property in a different way. Either via reflection or by running it in a separate test suite / JRE. ==> test(s) needs some work before the final commit. > * I did not change any of the project metadata - i.e. bundle version, API > filter, etc. The attached patch is only code - please adjust this metadata > as needed. This is no longer needed, since no API is changed.
+1 for 3.8.2+, assuming my patch is not changed and the test(s) get fixed before committing.
If you want to use the reflection approach, you should consider copying the following helper from JDT UI: org.eclipse.jdt.ui.tests.performance.views.Accessor
(In reply to comment #51) > Created attachment 229088 [details] > Troy's patch + some minor tweaks This patch only has JavaIndexTests.java. The rest are missing.
(In reply to comment #54) > (In reply to comment #51) > > Created attachment 229088 [details] [diff] > > Troy's patch + some minor tweaks > > This patch only has JavaIndexTests.java. The rest are missing. Strange. Just a sec.
Created attachment 229091 [details] Troy's patch + some minor tweaks (no tests)
(In reply to comment #56) > Created attachment 229091 [details] > Troy's patch + some minor tweaks (no tests) Patch looks good and all tests pass.
Created attachment 229146 [details] New test Updated test to use reflection to set the value. This doesn't yet fail when false is set. Looks like the path to get the index to a state to trigger rebuilding is complicated. A simple exit restart retains the index location. Even resetting the indexes don't help. Still working on this part. Troy, meanwhile if you provide some hint or would like to give it a try, that would be welcome.
(In reply to comment #58) > Created attachment 229146 [details] > New test Sorry, the patch still has some commented code, which need to be removed or used.
Created attachment 229157 [details] updated JUnit test The attached JUnit patch changes to use the Accessor class, as recommended in comment 53, to change the constant in the IndexManager. I copied it from the jdt.ui tests and added a new method, setFinal(String fieldName, boolean value), so that we can change a field with the final modifier. otherwise, it's the same code. The test has also been changed to not simulate restarting the product but rather just to refresh the external archives. Restarting a product will always re-load the indexes from disk so it was not a good test - refreshing is the proper action. Dani, With this change the testcase will still always succeed. This is because of the code in IndexManager: public void indexLibrary(IPath path, IProject requestingProject, URL indexURL, final boolean updateIndex) { IndexLocation indexFile = null; if(indexURL != null) { if(IS_MANAGING_PRODUCT_INDEXES_PROPERTY) { indexFile = computeIndexLocation(path, indexURL); } else { indexFile = IndexLocation.createIndexLocation(indexURL); } } If the property is not defined (and the indexURL is not null, because an index has been attached to the classpath entry) then the indexFile attribute will always be non-null and is used by the indexmanager... as a result, the index gets re-loaded. At the moment, I do not know what the proper fix is at the moment, but I do not see it being a major problem at this time. I'm still investigating.
(In reply to comment #60) > If the property is not defined (and the indexURL is not null, because an > index has been attached to the classpath entry) then the indexFile attribute > will always be non-null and is used by the indexmanager... as a result, the > index gets re-loaded. At the moment, I do not know what the proper fix is > at the moment, but I do not see it being a major problem at this time. I'm > still investigating. You mean, you don't know how to create a proper test? Otherwise, if the tests reflects your scenario, then no code changes and no backport are needed ;-). If it's just the test and we are close to the deadline, then I'm fine moving ahead with the backport and afterwards provide a valid test. Note that we have Easter holidays, Jay is expected to be back on Monday.
(In reply to comment #61) > (In reply to comment #60) > > If the property is not defined (and the indexURL is not null, because an > > index has been attached to the classpath entry) then the indexFile attribute > > will always be non-null and is used by the indexmanager... as a result, the > > index gets re-loaded. At the moment, I do not know what the proper fix is > > at the moment, but I do not see it being a major problem at this time. I'm > > still investigating. > > You mean, you don't know how to create a proper test? Otherwise, if the > tests reflects your scenario, then no code changes and no backport are > needed ;-). If it's just the test and we are close to the deadline, then I'm > fine moving ahead with the backport and afterwards provide a valid test. > > Note that we have Easter holidays, Jay is expected to be back on Monday. Right now I cannot figure out why the test passes in a JUnit environment when the flag is not set - it should be failing. The equivalent test in a UI situation (i.e. using Eclipse UI) passes my tests with the flag set and fails when the flag is not set, so it's working as expected. It is only the JUnit scenario where the behaviour is not working as expected.
(In reply to comment #62) > Right now I cannot figure out why the test passes in a JUnit environment > when the flag is not set - it should be failing. The equivalent test in a > UI situation (i.e. using Eclipse UI) passes my tests with the flag set and > fails when the flag is not set, so it's working as expected. It is only the > JUnit scenario where the behaviour is not working as expected. Thanks for confirming. I have pushed the fix alone to R3_8_maintenance. Tagged and maps files updated too. Master is yet to be updated, though.
And the fix for master is here: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=7e822612c4ed17c5f501a36859d0f7cd7f1c210d Will keep the bug open for the tests.
Troy, any luck with the test?
We have to look at one of the index test cases in and see if you can come up with a standalone junit test case similar to one of them. The bug has been verified by the bug reporter. Verified for 3.7 M7 using build I20130428-2000. The status of the bug has to be changed to RESOLVED.
Incorrect build id was specified. Verified for 3.7 M7 using build I20130501-2000.
The general idea with the testcase is do to the following: 1) Create a class file containing a single method, store it in a JAR file, generate an index for this JAR file, and add it to the classpath of a newly constructed Java project. Associate the index to the JAR on the classpath. 2) Perform a search for the single method and verify the results are correct. 3) Modify the method in the class file generated in step 1. 4) Restart the product. 5) Perform a search for the new method. Note: the index is NOT updated before/during step 5, so it is not synchronized with the content in the JAR. I would assume that step 5 *should* fail because the index from step 1 has not been updated. If the test is modified to set the flag IndexManager.IS_MANAGING_PRODUCT_INDEXES_PROPERTY to true after step 4 then step 5 should succeed (as the pre-build index will be regenerated using the latest content in the JAR) Correct me if I am wrong, but I think that is the expected results. Unfortunately, the test does not fail if the flag is not modified to true. The reason is because the search logic reads the contents directly from the JAR file - not the index. The search in step 5 will always succeed since it's reading the class file directly and not working off the content in the index. I'm continuing to debug this... but that's what I see so far and I wanted to pass it along in case anyone has any thoughts as to what could be wrong, or can correct me if any of my assumptions are wrong. for my info (in case it gets lost), adding a breakpoint in: org.eclipse.jdt.internal.core.search.matching.MatchLocator#process(PossibleMatch, boolean) will show where the class file is read during the search, due to this code: try { new ClassFileMatchLocator().locateMatches(this, classFile, info); } finally { this.patternLocator.mayBeGeneric = mayBeGeneric; }
Moving down from P1 because that indicates stop ship and we already decided to ship 3.8.2 without it.
We have to make an exception to this for not having a testcase as it has proven to be very difficult. I have reviewed the patch already and all looks good. No point in having this open.
Verified for Eclipse Mars 4.5 RC1 build id: I20150514-1000
