bndtools / bnd

Bnd/Bndtools. Tooling to build OSGi bundles including Eclipse, Maven, and Gradle plugins.
https://bndtools.org
Other
531 stars 305 forks source link

Service-Component error #2521

Closed pkriens closed 6 years ago

pkriens commented 6 years ago

Making Service-Component an error was a big error. We should never remove something because it causes this horrible problem that you checkout an old project and it stops working. A warning is the worst we can do. We did the same mistake with FixedIndexRepo. The FixedIndexRepo could easily have extended the OSGiRepository

For tools like bndtools we do not have the luxury to turn ok behavior in bad behavior.

Second, the Service-Component error seems to 'hang'. Even if you fix the cnf file it requires a clean build.

timothyjward commented 6 years ago

We should never remove something because it causes this horrible problem that you checkout an old project and it stops working.

I disagree with this. If we follow this model then we continue to accumulate code and bugs. The Service-Component header was made an error because bnd 4.0 has long been targeted to remove the old style bnd annotations and the header processing. See https://github.com/bndtools/bnd/commit/818cb33393cc1d8fef383bec85f2a1ddbec63b43#diff-6e52e3b5978b9b75ffa287b3a250faec

For tools like bndtools we do not have the luxury to turn ok behavior in bad behavior

If someone wants to use an old workspace then they have the option of using an old bndtools, or upgrading their workspace. I am not aware of any other tool that commits to 100% backward compatibility between major versions. It would not, for example, work if you tried to build one of these projects with the latest Gradle.

bjhargrave commented 6 years ago

Since Bnd 3.2, we deprecated the old support and announced our intentions to remove it in 4.0. Which we did.

People don't need to use the latest Bnd to continue to build their old existing projects. But if they want to update to the very latest Bnd, then they may need to update there workspace. I had to do this many times in the OSGi build. Particularly when the more strict properties file parsing was added that broke the build because Bnd had previously tolerated mistakes in the bnd files. People have to deal with similar issues when they update JDKs as the newer JDK tools may be more strict and report errors where there were none reported before.

Why is this suddenly a big issues for you?

djencks commented 6 years ago

Indeed. At one point, I think when stricter property parsing was installed, I couldn’t upgrade bnd for something like a year while I tried to figure out how to interpret the nonexistent error information from running bnd through ant and figure out what the problems in the (hundreds of) bnd files were. I didn’t get much sympathy at that point….

david jencks

On Jul 15, 2018, at 2:12 PM, BJ Hargrave notifications@github.com wrote:

Since Bnd 3.2, we deprecated the old support and announced our intentions to remove it in 4.0. Which we did.

People don't need to use the latest Bnd to continue to build their old existing projects. But if they want to update to the very latest Bnd, then they may need to update there workspace. I had to do this many times in the OSGi build. Particularly when the more strict properties file parsing was added that broke the build because Bnd had previously tolerated mistakes in the bnd files. People have to deal with similar issues when they update JDKs as the newer JDK tools may be more strict and report errors where there were none reported before.

Why is this suddenly a big issues for you?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bndtools/bnd/issues/2521#issuecomment-405118792, or mute the thread https://github.com/notifications/unsubscribe-auth/AAix3gf-0SKSNrKORas4sP3MxVVKX40Oks5uG7BbgaJpZM4VQM45.

pkriens commented 6 years ago

I am ok to give a warning but projects might be dormant for many years. One of the worst times to get errors is if you have to recompile a project you've not touched for many years. You've no idea what is going on. I really dislike this idea that you continuously have to chase the latest and the greatest.

Neil costed me a lot of time with the replacement of his templates, some unsolvable because the new model is less powerful than it replaced, and the changes in Service-Component header were simpler to fix but were imho a waste of my time. The absence of FixedIndexedRepo was silly to not keep it or make it extend the new one.

I also do not understand at all what we really gain. Deprecated code can be moved to a separate graveyard project and live there. It is not as if it costs money? I think in 4.0.0 the desire to clean up won over the desire to be backward compatible.

I think we need to clearly distinguish between compiling against bndlib and running bndlib in Eclipse/gradle/bnd/ant. Maybe we should start a graveyard project?

As David says, in these situations people postpone upgrading so you reach the opposite of what you want.

bjhargrave commented 6 years ago

I am ok to give a warning but projects might be dormant for many years. One of the worst times to get errors is if you have to recompile a project you've not touched for many years. You've no idea what is going on. I really dislike this idea that you continuously have to chase the latest and the greatest.

A project that has been dormant for years should still be using the same version of Bnd it was last built with since the Bnd version is part of the build. Either via maven plugin version or gradle plugin version.

But other things on the internet change. Companies like GitHub remove old ciphers and your SSL connections can then fail to negotiate. Unless everything you need to build is in your repo, there is o guarantee that your build will work years from now.

As David says, in these situations people postpone upgrading so you reach the opposite of what you want.

So we should also back out the strict properties parsing?

pkriens commented 6 years ago

I agree that the problem is bigger than bndtools but OSGi enRoute was pretty recent and was totally cratered for no obvious reason. Same for my customers. You just create a lot of havoc generating an error on a header that was not even wrong. I am puzzled I have to argue for this?

Clearly the gold standard is to have every tool archived. However, that does not mean we need to crater projects for no real benefits to us except maybe the pleasant feeling things have cleaned up? We're not a library, we're part of a tool chain. Making it harder for people to upgrade does not sound like good marketing to me. Actually, it is really bad marketing in an already complicated market.

So we should also back out the strict properties parsing?

But with hindsight we probably should have made the properties parser respect -upto so David's errors could have turned into warnings until he got his act together.

However, this class of errors is different since those were actual errors in the workspace, they just had not been reported until then. Neither the FixedIndexRepo nor the Service-Component: * was in any way an error before. Those were perfectly ok workspaces but we made them crater workspaces for no benefit to the customer. Not good imho.

rotty3000 commented 6 years ago

maybe what we need here is a "update/upgrade" process :|

Something that would recognize a certain class of errors, and apply a known fix.

On Mon, Jul 16, 2018 at 8:17 AM, Peter Kriens notifications@github.com wrote:

I agree that the problem is bigger than bndtools but OSGi enRoute was pretty recent and was totally cratered for no obvious reason. Same for my customers. You just create a lot of havoc generating an error on a header that was not even wrong. I am puzzled I have to argue for this?

Clearly the gold standard is to have every tool archived. However, that does not mean we need to crater projects for no real benefits to us except maybe the pleasant feeling things have cleaned up? We're not a library, we're part of a tool chain. Making it harder for people to upgrade does not sound like good marketing to me. Actually, it is really bad marketing in an already complicated market.

So we should also back out the strict properties parsing? But with hindsight we probably should have made the properties parser respect -upto so David's errors could have turned into warnings until he got his act together.

However, this class of errors is different since those were actual errors in the workspace, they just had not been reported until then. Neither the FixedIndexRepo nor the Service-Component: was in any way an error before. Those were perfectly ok workspaces but we* made them crater workspaces for no benefit to the customer. Not good imho.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bndtools/bnd/issues/2521#issuecomment-405228853, or mute the thread https://github.com/notifications/unsubscribe-auth/AAI9TChujwUWjEnv6SkN3odltGHJ93otks5uHIRxgaJpZM4VQM45 .

-- Raymond Augé http://www.liferay.com/web/raymond.auge/profile (@rotty3000) Senior Software Architect Liferay, Inc. http://www.liferay.com (@Liferay) Board Member & EEG Co-Chair, OSGi Alliance http://osgi.org (@OSGiAlliance)

pkriens commented 6 years ago

@rotty I think your idea would be great if these things were warnings and the quick fix would correct them. However, that still would not permit us to remove functionality, however much we dislike it today. Note that if you run in such an error you usually haven't touched the workspace in quite some time and you're likely already in panic mode.

This issue because I am slowly coming to the conclusion that a breaking change is always bad. If you need a breaking change you change the name (like Go) and leave the shit behind. However, it should always be there, maybe in a graveyard project.

timothyjward commented 6 years ago

This issue because I am slowly coming to the conclusion that a breaking change is always bad. If you need a breaking change you change the name (like Go) and leave the shit behind. However, it should always be there, maybe in a graveyard project.

But then how would we ever update things like the Repository interface? Following this logic we are forced to keep a set of antiquated implementations that we actively tell people not to use, but we're forced to add new features to anyway whenever we make a provider breaking change.

bjhargrave commented 6 years ago

However, it should always be there, maybe in a graveyard project.

I am not sure what this means on a practical basis. Is the graveyard project a separate thing that is not part of Bnd? Or is it part of Bnd but we wont ever fix it or take problem reports on it?

If you need a breaking change you change the name (like Go)

I don't think we have ever treated Bnd like API. And the renaming thing is appropriate for packages rather than jars.

Note that if you run in such an error you usually haven't touched the workspace in quite some time and you're likely already in panic mode.

Why are you in a panic mode when trying to update to a newer version of Bnd? That seems hardly a panic state. Your build worked with Bnd before and will still work with old version of Bnd configured into your build. Updating to a newer version of Bnd (or any build tool) is a deliberate decision and generally some changes are necessary. Every time I have ever updated the JDK version used to build Bnd, I have had to make a number of source code changes to deal with changes in javac. From 6 to 7, 7 to 8, and 8 to 9. Every time.

pkriens commented 6 years ago

First, I am ok to leave things around deprecated. I really do not understand the cost of some dead code. However, if you really want to clean up then having a separate project, maybe a fragment (shudder), could contain the dead code. bnd, Eclipse could then carry this project as well to be backward compatible.

And you do not seem to understand how this works in practice. There is an old project and it needs a small update. In many cases you need something of a later version of your tools. At that moment you really do not feel like fixing an error that you did not make, do not understand, do not know how to fix, or know what the implications will be.

If this had some real value then you could tell them it would be good for humanity but in this case it saves a few bytes?

bjhargrave commented 6 years ago

If this had some real value then you could tell them it would be good for humanity but in this case it saves a few bytes?

Removing old, deprecated since 3.2 code is not just byte savings. It is complexity savings, it is future bug avoidance, it is code which doesn't need attention when addressing cross cutting bugs or new features. Dead trees and branches are removed not because people don't like looking at them. They are removed to avoid future problems caused by them falling on your house :-)

What is done is done for 4.0. In the future, we can be more circumspect about removing features. But by the same token, we must be more circumspect about adding new features since they can become tomorrow's dead wood. I know you are in favor of adding new features as you please but then you must also appreciate that not all new features you add have to or can be reasonably supported forever. If it easy to add a new feature, it must also be possible to mark an old feature as deprecated and targeted for removal in a future release.

pkriens commented 6 years ago

Well, the basic difference is that I do not see a lot of problems with dead wood in the code? Our genes seems to have a very large percentage junk DNA and seem to do an awesome job? What problems did we have with the ancient 12 year old XML generator, or for that matter the FixedIndexRepo? We have a much better solution now but I do not see a what we really gained by removing those 2 classes? Isn't it worth a few extra potential theoretical problems on our side to prevent screwing other people over?

So I do not buy that we need to be extremely careful in adding functions. It is not as if the cases we've seen were failed experiments? The component annotations drove the OSGi spec, as did the manifest header annotations. The original Service-Component header XML was very useful but the bnd component annotations outran them. And that code inspired the OSGi annotations that replaced them. The FixedIndexRepo was a very useful addition by Neil. The reason we replaced it is because we could optimize the implementation with the new resource support in bnd that I had added, which gave us the OSGiRepository. It was just given another name to not cause compatibility problems. (!)

So none of the cases we talk about would fall under your proposal to be careful with experiments? With that attitude we'd likely never had a whole host of innovations like component annotations, manifest header annotations, contracts, baselining, consumer/provider types, executable jars, remote agent, conditional package, fixup messages, profiles, plugins, augments, maven pom repo, p2 repo, merged properties, wabs, and so much more? These were all experiments one day and I've still a number of things in my head like for example the JUnitFramework, annotations from XML to Java, and PDE import that I am not willing to sacrifice because they might not be needed anymore one day, or one day a better alternative might become available. I am really not ready to let bnd go in slow mode.

So if we need to find a way that we can move dead code out of the way while users can still use it (with proper warnings) but not part of the main code base, I am ok with a fragment graveyard project. And if there are conflicts, we could use a different package name for the new stuff ... The drivers could then optionally include this graveyard project.

I would also accept a list of instructions and macros that are considered experimental. They would get highlighted in the editor and generate a warning. As long as they are on that list they can be removed from the code. I think it is solving a non-existent problem but since you feel so strong about being able to break compatibility I could live with such a solution. As long as the user does not have to pay the price.

ViToni commented 6 years ago

I'm using bndtools 3.5 in my main proejct and by accident I upgraded bndtools in my main IDE instead of the of the experimental one. I couldn't get my project to run, but surprisingly the gradle build was just fine. As I had no visual indication it took me an hour until I discovered that it was in a way related to FixedIndexedRepo and the updated version somehow wouldn't see it, first thinking that it would be some kind of bug in the newer version.

So regarding the FixedIndexedRepo: Sure you can change your code but hive me a hint about things changing and I mean when I'm using it. You can't expect users of bndtools to have the same knowledge as you the developers. As for me I just want it to work. Might be that the code is better now but for me the experience was just, oh WTF!

If something like that is removed it should give me a least a hint in BIG RED LETTERS that I'm using something deprecated and should switch instead of just failing silently. It would be great to consider also the user experience when changing things.

DaHoC commented 6 years ago

Generally speaking from a user/developer point of view: If my tooling hides hints / warnings or errors (especially about deprecated / unsupported instructions) and even more just stops working for the formerly provided functionality, I consider this a BAD practice. Ideally software should be transparent in a way that the user is able to immediately grasp what is going on and why. Hiding away such important and necessary information is the wrong way to go.

bjhargrave commented 6 years ago

Fixed for 4.1.