You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@netbeans.apache.org by Jaroslav Tulach <ja...@gmail.com> on 2022/08/20 06:37:03 UTC
Cleanup PRs shall not touch `.sig` files was: API snapshot review for NetBeans 15 - possible issue?
Hello guys,
thanks a lot for dealing with the issue so nicely in PR-4498.
> Re: the bitwise shifts
The easiest way to avoid breaking an API is to not touch it. I have to admit I
don't understand why the need to clean something up also includes innocent API
constants?
> yeah this sounds like something we should revert. Since it is a cleanup
> PR, reverting it should be safe.
+1
I'd also suggest to remember that cleanup PRs shall not touch API! There is no
justification in breaking API just because we want to clean our codebase.
Thank you: https://github.com/apache/netbeans/pull/4498/files#r950657498
Executive summary: If a "cleanup PR" has to touch `.sig` files, then it is not
a cleanup!
-jt
Dne čtvrtek 11. srpna 2022 19:50:30 CEST, László Kishalmi napsal(a):
> I agree. I've only done the minimum between two meetings. Feel free to
> create a more complete one.
>
> On Thu, Aug 11, 2022 at 10:06 AM Scott Palmer <sw...@gmail.com> wrote:
> > The entire way those bit shifts are coded is error prone.
> >
> > Should be:
> > /** Operating system is Windows NT. */
> >
> > public static final int OS_WINNT = 1 << 0;
> > /** Operating system is Solaris. */
> > public static final int OS_SOLARIS = 1 << 1;
> > public static final int OS_SOLARIS = 1 << 3;
> > /** Operating system is Linux. */
> > public static final int OS_LINUX = 1 << 4;
> >
> > …
> >
> > Not using references to earlier constants and only changing the amount of
> > the shift. Makes it easier to read and see whaat the actual value should
> > be. It’s clear that each value represents a single bit without having to
> > go back to see what the original value was, and removing any of the
> > constants will never cause subsequent values to change. Maybe add a
> > comment
> > that 1 << 2 is skipped for whatever reason. Perhaps leave a comment
> > documenting the old w95/98 values so we know why those values aren’t used
> > anymore.
> >
> > Scott
> >
> > > On Aug 11, 2022, at 12:48 PM, László Kishalmi
> > > <la...@gmail.com>
> >
> > wrote:
> > > Anyway, here it is. I hope it helps:
> > > https://github.com/apache/netbeans/pull/4497
> > >
> > > On Thu, Aug 11, 2022 at 9:15 AM László Kishalmi <
> >
> > laszlo.kishalmi@gmail.com>
> >
> > > wrote:
> > >> OMG!
> > >> Just had some time to look at this. Unfortunately, I think it is
> >
> > serious.
> >
> > >> Due to wrong bit-shifts the id-s for MAC_OS and LINUX has been changed,
> > >> which would mean a serious incompatibility as AFAIK the compiler puts
> > >> actual value of these fields into the bytecode.
> > >> That would cause third-party plugin compatibility issues whenever the
> > >> plugin would use those constants directly.
> > >> I hate to say it, but I think it is a reason for RC4
> > >>
> > >> Do we have a PR fixing those bit-shifts or shall I create one?
> > >>
> > >> On Wed, Aug 10, 2022 at 1:34 AM Neil C Smith <ne...@apache.org>
> > >>
> > >> wrote:
> > >>> Hi,
> > >>>
> > >>> I generated the API snapshot sigtest file for review yesterday.
> > >>> Ideally we need to review earlier in the release process, but for a
> > >>> number of reasons they haven't been generated until now ...
> > >>>
> > >>> https://github.com/apache/netbeans/pull/4487
> > >>>
> > >>> One thing that stands out to me is the changes in compile time
> > >>> constants inside Utilities introduced by
> > >>> https://github.com/apache/netbeans/pull/4025
> > >>>
> > >>> I'm not sure how much of an issue that might be in practice, and
> > >>> whether it's a reason to run an rc4? Review welcomed!
> > >>>
> > >>> Thanks,
> > >>>
> > >>> Neil
> > >>>
> > >>> ---------------------------------------------------------------------
> > >>> To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
> > >>> For additional commands, e-mail: dev-help@netbeans.apache.org
> > >>>
> > >>> For further information about the NetBeans mailing lists, visit:
> > >>> https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
> > For additional commands, e-mail: dev-help@netbeans.apache.org
> >
> > For further information about the NetBeans mailing lists, visit:
> > https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
For additional commands, e-mail: dev-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: Cleanup PRs shall not touch `.sig` files was: API snapshot review for NetBeans 15 - possible issue?
Posted by Jaroslav Tulach <ja...@gmail.com>.
Dne pátek 26. srpna 2022 13:29:54 CEST, Neil C Smith napsal(a):
> On Sat, 20 Aug 2022 at 07:37, Jaroslav Tulach <ja...@gmail.com>
wrote:
> > Executive summary: If a "cleanup PR" has to touch `.sig` files, then it is
> > not a cleanup!
>
> Totally agree!
>
> However, the interesting thing here is that if it wasn't for the
> bitshift value changes showing up in
> https://github.com/apache/netbeans/pull/4487 this might not have been
> picked up.
>
> Shouldn't the constant value changes have also caused test failures in
> the original cleanup PR?
Hmm. Good point. The explanation very likely is: we are running "binary
compatibility" check. E.g. not what happens after re-compilation, but what
happens during linkage time.
`public static final int` constants are copied to the usage place by value at
compilation time. E.g. removing such constant isn't binary incompatible! Just
source incompatible! The bitshift made the change also binary incompatible as
the previous values of the constants and the new values were different.
Interesting behavior of sigtest. I wasn't aware of it.
> Maybe in Michael's CI changes we could also look at failing PRs that
> touch .sig files unless they have a particular label set on them (eg.
> API change)?
Labeling API changes by CI would be great!
-jt
> Best wishes,
>
> Neil
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
> For additional commands, e-mail: dev-help@netbeans.apache.org
>
> For further information about the NetBeans mailing lists, visit:
> https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
For additional commands, e-mail: dev-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: Cleanup PRs shall not touch `.sig` files was: API snapshot review for NetBeans 15 - possible issue?
Posted by Neil C Smith <ne...@apache.org>.
On Sat, 20 Aug 2022 at 07:37, Jaroslav Tulach <ja...@gmail.com> wrote:
> Executive summary: If a "cleanup PR" has to touch `.sig` files, then it is not
> a cleanup!
Totally agree!
However, the interesting thing here is that if it wasn't for the
bitshift value changes showing up in
https://github.com/apache/netbeans/pull/4487 this might not have been
picked up.
Shouldn't the constant value changes have also caused test failures in
the original cleanup PR?
Maybe in Michael's CI changes we could also look at failing PRs that
touch .sig files unless they have a particular label set on them (eg.
API change)?
Best wishes,
Neil
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
For additional commands, e-mail: dev-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists