Bugzilla – Full Text Bug Listing
|
Description
Lars Vogel
1) Following *internal* packages that want to be public API should drop the "internal" segment: org.eclipse.debug.ui.launchview.internal.launcher org.eclipse.debug.ui.launchview.internal.services org.eclipse.debug.ui.launchview.internal org.eclipse.debug.ui.launchview.internal.impl org.eclipse.debug.ui.launchview.internal.model org.eclipse.debug.ui.launchview.internal.view 2) org.eclipse.debug.ui.launchview.internal.services.LaunchModel is an interface but doesn't follow naming conventions (should start with "I"). 3) API should be documented, extension points including. 4) Whatever else after review of the code above. Can we have some clarity as to who is going to define the APIs and by when are we planning to have this? As the bug is marked as helpwanted, I am confused. I thought Markus will be the right person to do so. Yes, I will work on providing a cleaned up API here. I will extract interfaces where required, correct naming and document them properly. (In reply to Markus Duft from comment #3) > Yes, I will work on providing a cleaned up API here. I will extract > interfaces where required, correct naming and document them properly. Great! New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.debug/+/185799 Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.debug/+/185799 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.debug.git/commit/?id=bafaf82d058afcb6206c541de34c7905dff142e7 There is a new releng test failure: org.eclipse.releng.tests.BuildTests testPluginFiles Failure Plugin directory missing required files: /home/jenkins/agent/workspace/ep422I-unit-cen64-gtk3-java11/workarea/I20211002-0220/eclipse-testing/test-eclipse/eclipse/plugins/org.eclipse.debug.ui.launchview_1.0.2.v20211001-1417.jar; /home/jenkins/agent/workspace/ep422I-unit-cen64-gtk3-java11/workarea/I20211002-0220/eclipse-testing/test-eclipse/eclipse/plugins/org.eclipse.debug.ui.launchview.source_1.0.2.v20211001-1417.jar; java.lang.AssertionError: Plugin directory missing required files: /home/jenkins/agent/workspace/ep422I-unit-cen64-gtk3-java11/workarea/I20211002-0220/eclipse-testing/test-eclipse/eclipse/plugins/org.eclipse.debug.ui.launchview_1.0.2.v20211001-1417.jar; /home/jenkins/agent/workspace/ep422I-unit-cen64-gtk3-java11/workarea/I20211002-0220/eclipse-testing/test-eclipse/eclipse/plugins/org.eclipse.debug.ui.launchview.source_1.0.2.v20211001-1417.jar; @Lars, Markus: do you plan to address new test fail / fix new bundle build? The releng test is still failing on master: https://download.eclipse.org/eclipse/downloads/drops4/I20211003-1800/testresults/html/org.eclipse.releng.tests_ep422I-unit-cen64-gtk3-java11_linux.gtk.x86_64_11.html (In reply to Andrey Loskutov from comment #9) > @Lars, Markus: do you plan to address new test fail / fix new bundle build? > > The releng test is still failing on master: > > https://download.eclipse.org/eclipse/downloads/drops4/I20211003-1800/ > testresults/html/org.eclipse.releng.tests_ep422I-unit-cen64-gtk3- > java11_linux.gtk.x86_64_11.html Markus, can you have a look? I'm without computer for this week. My guess would be that the about.html is missing in the source build. Open another build properties and check how it is configured to get the missing files. New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.debug/+/186110 Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.debug/+/186110 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.debug.git/commit/?id=0c244122d37b581b8ec17cec98d6de3fd6929ec9 Thanks, there are no build failures anymore. The "API" itself is currently provisional. Is a review planned for proposed API? On quick check, I found org.eclipse.debug.ui.launchview.services.ILaunchObject.getImage() problematic - it is better to use ImageDescriptor to avoid leaking resources because of objects eagerly allocating images even if not used. Few review comments - 1. Please add the automatic module name in MANIFEST.MF 2. Please move the tests to a separate plugin https://git.eclipse.org/r/c/platform/eclipse.platform.debug/+/185803 3. Do we need the interface ILaunchObjectProvider and abstract class AbstractLaunchObjectProvider both exported? 4. I don't see DebugCoreProvider being used anywhere 5. I see a lot of code related to FileLogger but I don't see any place where Debug is passing the File as input, it goes as null. Any plans to add support for this? We should plan to fix this by M3 which means by 5th Nov. New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.debug/+/187267 (In reply to Sarika Sinha from comment #14) > Few review comments - > 1. Please add the automatic module name in MANIFEST.MF Done. https://git.eclipse.org/r/c/platform/eclipse.platform.debug/+/187267 > 2. Please move the tests to a separate plugin > https://git.eclipse.org/r/c/platform/eclipse.platform.debug/+/185803 Change updated. > 3. Do we need the interface ILaunchObjectProvider and abstract class > AbstractLaunchObjectProvider both exported? Moved AbstractLaunchObjectProvider to internal package and adapted upstream LcDSL which used it to copy the implementation. > 4. I don't see DebugCoreProvider being used anywhere It is used as service -> @Component(service = ILaunchObjectProvider.class) registers it, and org.eclipse.debug.ui.launchview.internal.model.LaunchViewModel uses @Reference(service = ILaunchObjectProvider.class, cardinality = ReferenceCardinality.MULTIPLE, policy = ReferencePolicy.DYNAMIC, policyOption = ReferencePolicyOption.GREEDY) to obtain all available providers, which may be MORE than just the "default" DebugCoreProvider, e.g. LcDSL > 5. I see a lot of code related to FileLogger but I don't see any place where > Debug is passing the File as input, it goes as null. Any plans to add > support for this? Again something that is currently only usable together with LcDSL, as it provides API to dynamically generate launch configurations (temporary or permanent) and redirect their output dynamically. In the future I for sure could see use in "standalone" debug as well, yes. > > We should plan to fix this by M3 which means by 5th Nov. Let me know if there is something open now. New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.debug/+/187271 Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.debug/+/187267 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.debug.git/commit/?id=408bd3704245626a91ca0c02ad2faabd2fd7c21d Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.debug/+/187271 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.debug.git/commit/?id=c0e1403a28cdf5e6d99a1d2df5f59ecc0694b7c6 Looks Good. Thanks Markus! I20211109-1800 |