Bugzilla – Full Text Bug Listing
| Summary: | [publisher] Branding iron - investigate code which catches and ignores Exception | ||
|---|---|---|---|
| Product: | [Eclipse Project] Equinox | Reporter: | DJ Houghton <dj.houghton> |
| Component: | p2 | Assignee: | P2 Inbox <equinox.p2-inbox> |
| Status: | CLOSED WONTFIX | QA Contact: | |
| Severity: | normal | ||
| Priority: | P3 | CC: | aniefer, irbull, jeffmcaffer, laeubi, loskutov, mn, pascal, t-oberlies, thomas |
| Version: | 3.7 | ||
| Target Milestone: | --- | ||
| Hardware: | All | ||
| OS: | All | ||
| See Also: |
https://git.eclipse.org/r/151888 https://bugs.eclipse.org/bugs/show_bug.cgi?id=552627 https://git.eclipse.org/r/c/equinox/rt.equinox.p2/+/151888 https://git.eclipse.org/c/equinox/rt.equinox.p2.git/commit/?id=d9557a77c5af92590bf0215b16bf3caa3bc4dbb2 |
||
| Whiteboard: | stalebug | ||
| Bug Depends on: | 351056 | ||
| Bug Blocks: | |||
|
Description
DJ Houghton
Bug 342489 revealed another error condition that does not lead to an error status but only an output to System.err (in org.eclipse.pde.internal.swt.tools.IconExe.main(String[])): [java] Error - 6 icon(s) not replaced in /tmp/p2.brandingIron3084785278937756120/eclipse.exe using /tmp/LauncherName8776581383509947268.ico This error condition should probably make the BrandingIron and EquinoxExecutableAction fail because the resulting launcher is not fully branded (also see org.eclipse.equinox.p2.tests.publisher.actions.EquinoxExecutableActionTest.testWin()) Fixing this behaviour will probably require changes to the IconExe class. Since the publisher refactorings, p2 and pde use the same class (before, they had an identical copy each). However the class comments indicate that the class was forked from yet another location. This needs to be checked, because we really don't want to work on a fork. (In reply to comment #1) > However the class comments indicate that the class was > forked from yet another location. This needs to be checked, because we really > don't want to work on a fork. This is the only copy of this class. Some of the static inner classes were copied from SWT (eg: ImageLoader, ImageData, PaletteData) but as a whole this diverged from the SWT original years ago. If we started being strict about having all the icon resolutions, we would likely break builds. Not sure if this is a good idea. I consider a broken build as something good if this reveals a hidden issue that I may only have noticed much later ("fail early"). The question is really, does anyone want to have partially replaced icons in the executable. I clearly doubt this. Therefore I'd say that p2 does not need to support that case.
Obviously we'll need to think about the impact on the users - the change should not block them. But I don't see this happening. Sure, their build will fail at first, but they can always remove their branding icon until they have added the missing resolutions.
I agree with Tobias. Revealing problems is always good. Hiding (deferring) potential problems and making them hard to find is always bad. That standpoint is likely very easy to defend if a user is annoyed that their build breaks because of issues like this. PDE/Build must remain backwards compatible, making a new build setup fail in this case is one thing but we can't make changes that will cause existing builds to suddenly stop working. I am worried about org.eclipse.equinox.p2.publisher.eclipse in general. This is largely a maintenance release for PDE/Build without many new features. If code that has remained mostly unchanged in PDE for years is suddenly under active development again, then I'm starting to think that PDE/Build may need to keep its own original copy of these classes. Bug 341537 is an example of the kinds of changes that worry me. If PDE/Build had its own copy, then we would be free to make the builds fail here since it is all new for p2 without backwards compatibility worries. (In reply to comment #6) > If PDE/Build had its own copy, then we would be free to make the builds fail > here since it is all new for p2 without backwards compatibility worries. I may have been a little confused over what was actually being proposed, if a build failure came out of org.eclipse.equinox.p2.publisher.eclipse/BrandingIron or EquinoxExecutableAction#fullBrandExecutables, this would be fine since PDE/Build is still using its own BrandingIron. (In reply to comment #6) > PDE/Build must remain backwards compatible, making a new build setup fail in > this case is one thing but we can't make changes that will cause existing > builds to suddenly stop working. This is a fair requirement. If the PDE wants to continue supporting partially branded icons, we could to add an option to enable the current, lenient behaviour. > Bug 341537 is an example of the kinds of changes that worry me. I thought that pde.build is a friend of p2.publisher.eclipse - I would have presumed that this means that the pde.build.tests need to be executed in case of changes in p2.publisher.eclipse... > If PDE/Build had its own copy, then we would be free to make the builds fail > here since it is all new for p2 without backwards compatibility worries. I thought the whole point of bug 331974 was to have PDE and p2 join forces on the publishers for the Eclipse formats - for bug fixes, and new features like license features (that p2 only supports since the refactoring). Can't we just write the test that ensures that we keep the old behaviour available for PDE? Since you were asking: the PDE currently only has its own copies of ProductFile and BrandingIron (cf. bug 342550). IconExe is already shared today. Tobias, are we planning more work in this area for 3.7? If not please differ to 3.8 or 3.7.x. (In reply to comment #6) > PDE/Build must remain backwards compatible, making a new build setup fail in > this case is one thing but we can't make changes that will cause existing builds > to suddenly stop working. Actually, I think the best solution to this problem is to have the product publisher action a typed warning. Then the callers can decide whether they consider the warning as fatal. Does something like this exist in the publishers today? We could use custom exception types and/or status code constants for the publishers. I don;'t think anything like that exist today, but the idea of returning a custom status or exception could work for this use case. Move all 3.8 bugs to Juno. Tobias, if you plan on working on this feel free to put it on the Kepler plan. This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie. Are there any plans to resolve this? Currently the behavior is identical as described 8 years ago, an error is omitted but build happily continues. Maybe it would be worth to get away from calling "main(args)" and instead add a dedicated method that includes File objects instead of String path and an error consumer. The the caller can decide if there should be an error output, exception thrown, or warning emitted to some kind of log. Beside of this, it would be really helpful if the error output would include some more information, e.g. why a icon was not replaced e.g.: - icon was not given at all - icon has wrong resoloution - icon has wrong color-deepth - icon has wrong/unsupported encoding For example i'm currently not able to replace 256-32 bit icons, all icons are replaced but IconExe complains that 256 is not replaced, as I created the icon in the same way as for example the 48x48-32 bit I have no idea what could be wrong. Christoph, feel free to contribute. (In reply to Andrey Loskutov from comment #16) > Christoph, feel free to contribute. I'll try to make some enhancements here starting with IconExe, the problem with BrandingIron is, that it has no way to propagate errors to the caller currently, so we need to change that also, next thing would be EquinoxExecutableAction what could propagate the error via multistatus to the caller, so if one likes to know if it really succeeds it must introspect the children of the status so it should not break "legacy" code, any objections here? New Gerrit change created: https://git.eclipse.org/r/151888 (In reply to Christoph Laeubrich from comment #15) > For example i'm currently not able to replace 256-32 bit icons, all icons > are replaced but IconExe complains that 256 is not replaced, as I created > the icon in the same way as for example the 48x48-32 bit I have no idea what > could be wrong. Hope these steps [1] are still valid. [1] https://github.com/neo4j-contrib/neoclipse/commit/2c0034cefcb4bc87afbc68d2c0e90ffdd8852d6c Thanks for sharing this, I'll try it out looks very promising. Its still worth to inform the user about errors while generating/replacing. With my provided gerrit I was able to find out why my bitmaps previously where rejected see Bug 552627. The Bad thing is, that GIMP has removed support for writing this format, but I was able to use imagemagic to write the desired format using > -define bmp3:alpha=true -define bmp:format=bmp3 I wonder if it would be worth to add some helper methods to IconExe that take e.g svg/png format and generates appropriate resources using imagemagic? Of course this can only be used when imagemagic is installed but could help people to save some headache. It would even be possible to generate appropriate Linux and Macos icons, haven't tested for solaris as I have no way to verify it. Gerrit change https://git.eclipse.org/r/c/equinox/rt.equinox.p2/+/151888 was merged to [master]. Commit: http://git.eclipse.org/c/equinox/rt.equinox.p2.git/commit/?id=d9557a77c5af92590bf0215b16bf3caa3bc4dbb2 This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. As such, we're closing this bug. If you have further information on the current state of the bug, please add it and reopen this bug. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie. |