search

eclipse-equinox / equinox.bundles

Eclipse Public License 2.0
8 stars 16 forks source link

Bump versions for bundle and package-export #36

Closed laeubi closed 2 years ago

laeubi commented 2 years ago

Fix https://github.com/eclipse-equinox/equinox.bundles/issues/35

I incremented the versions even though API tools complains ... let see what the build tells here.

iloveeclipse commented 2 years ago

Regarding API tools: in original PR, you've added API filters. Now you most likely should remove these and add other API filter for current API tooling problem.

laeubi commented 2 years ago

I tried that but git claims the result is the same... maybe this requires just another one to get rid of that.

mickaelistria commented 2 years ago

Regarding API tools: in original PR, you've added API filters. Now you most likely should remove these and add other API filter for current API tooling problem.

I even believe that those API filters shouldn't have been placed in 1st place; and the version should have been bumped just like API Tools were suggesting first. Please avoid using API Filters as much as possible, they're most often right.

laeubi commented 2 years ago

I even believe that those API filters shouldn't have been placed in 1st place; https://wiki.eclipse.org/Platform/How_to_Contribute says:

Decision about which bundle version segment to change should be always based on [1], [2] and [3], not just on PDE "quick fix" proposals

And I don't think people will be happy with a major version bump due to adding some static constants to an interface.

https://wiki.eclipse.org/Evolving_Java-based_APIs_2 says:

Add API field | If interface not implementable by Clients | Binary compatible If interface implementable by Clients | May break compatibility (2)

Given that there are no implementations of IScopeContext other than the internal ones and this only adds new static final fields its very unlikely that any confusion/error occurs...

mickaelistria commented 2 years ago

And I don't think people will be happy with a major version bump due to adding some static constants to an interface.

Indeed, API Tools are quite protective in that case, but at least they say something useful. Any API addition at least requires a minor version bump.

Given that there are no implementations of IScopeContext

You really never know that. Interface has been exported and not final nor sealed for about 20 years, and documentation even clearly says "Clients may implement this interface.". There is not much room for interpretation about whether something is API or not, it's a technical definition that was usually taken when code was created; and we must obey it rigorously for compatibility reasons, even when we don't like it.

laeubi commented 2 years ago

You really never know that. Interface has been exported and not final nor sealed for about 20 years, and documentation even clearly says "Clients may implement this interface.".

Yeah Ive seen that but I really wonder if there is any practical use-case for that there , but lets assume I extend or implement the interface, I can define a property with the same name without a problem... so I really don't see any issue here that would be of any importance or even important enough to bump the major version and put too much effort in here....

mickaelistria commented 2 years ago

I can define a property with the same name without a problem...

Yes, but the same doc you pointed explains it's a bit risky. But indeed not worth a major bump IMO. Just a minor bump is fine here.

laeubi commented 2 years ago

Yeah but I can't find a reference beside that doc here, e.g. https://docs.oracle.com/javase/specs/jls/se7/html/jls-13.html#jls-13.4.10 says:

If a field that is not declared private was not declared static and is changed to be declared static, or vice versa, then a linkage error, specifically an IncompatibleClassChangeError, will result if the field is used by a pre-existing binary which expected a field of the other kind.

For sure if it is a class I can understand that but for interfaces? The only issue I can think of if we remove the field, because it might not be inline, but in all other cases it should:

  1. inline so java will never know
  2. just ignore it

There is just a small chance that there is a name clash if I do static-import everything from different sources, but that's really somehow theoretical given that this interface do not declared anything before so why should I have ever done this?

mickaelistria commented 2 years ago

Maybe the doc is wrong and the case of interface symbols vs class fields should be separated. I would recommend you suggest an edit to the doc and share the suggestion with the eclipse-dev list. If there is a general agreement from language experts that the suggestion is good, we apply it to doc and later cascade it to API Tools.

laeubi commented 2 years ago

if @tjwatson and @iloveeclipse are also fine with that I would then merge this.