www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.internals - Regression control & breaking changes policies

reply Dicebot <public dicebot.lv> writes:
Moving the discussion from the PR 
(https://github.com/dlang/dmd/pull/6290#issuecomment-264706485) 
for better exposure.

Recently I tried to get a bit more involved with preparing and 
maintaining releases but I see that there are a lot of responses 
that imply that at least some of my methodologies are either 
misunderstood or disapproved. So probably better to discuss it to 
see if we can get on a common ground.

There are several fundamental goals I'd like to pursue:

1) At any given point of time the master branch should be as 
close to release quality as possible
2) Lack of implementation quality in merged PRs should not become 
burden of maintainers
3) Deprecation system has to be used for any breaking changes to 
the point it can be relied on

(1) and (3) are supposed to make life of commercial users easier 
directly, (2) aims for the same goal by reducing maintenance 
burden and thus improving release fixup latency. Those all are 
areas that are currently lacking a lot in D development process 
and that is rather painful.

Keeping this goal list in mind, I want to propose some consequent 
practical applications:

a)

     If any breaking change is detected in both master and stable 
that doesn't
     come at least with detailed explanation in the changelog, it 
has to be
     reverted unconditionally.

This is a very simple and harmless way to improve on goals (1) 
and (2). I have noticed that reverting of merged code is often 
viewed as something damaging and even disrespectful - it isn't! 
By reverting a PR one immediately fixes the problem in relevant 
branch causing less trouble for other developers while providing 
more valuable information (and regression test case) to revive 
original PR. Compared to trying to fix the problem in place it 
also puts much less stress on developer as one can take all the 
time needed.

Similar reasoning applies to fixing regressions in point releases 
- it is not worth spending time on that unless fix is immediately 
obvious and trivial. World won't burn if those fixes will get 
delayed a bit, but providing public release with minimal amount 
of issues known is urgent.

b)

    Any breaking change that causes compile-time error has to 
undergo
    deprecation stage. It applies to bug fixes too. If breaking 
change changes
    code semantics but keeps compiling, it has to provide 
`-transition=XXX`
    diagnostics to help with migration AND changelog entry 
explaining what to
    look for.

This is a long standing source of complains from me but I had no 
idea that purpose of `-transition` flags is not widely obvious. 
Martin Nowak has drawn this nice table that explains it better:

before change  |  after change | transition process
===================================================
valid          | invalid       | deprecate-then-error
invalid        | valid         | none
invalid        | invalid       | none
valid          | valid         | -transition=feature to find 
semantic changes

One key thing about this is that adding deprecation is in most 
cases trivial and adds no extra effort if done right from the 
beginning. Tracking changes and fixing it later, on the contrary, 
is a killer for combined productivity.
Dec 04 2016
next sibling parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 12/4/2016 3:52 PM, Dicebot wrote:

 before change  |  after change | transition process
 ===================================================
 valid          | invalid       | deprecate-then-error
 invalid        | valid         | none
 invalid        | invalid       | none
 valid          | valid         | -transition=feature to find semantic changes

 One key thing about this is that adding deprecation is in most cases trivial
and
 adds no extra effort if done right from the beginning.


The table makes sense and looks right. But there's a problem with the return 
scope changes. It's just too much to just wholesale deprecate things all at 
once. People need time to ease into it, not be faced with a pile of diverse 
deprecation messages.

Having -transition=safe makes that practical, and also ensures time to correct 
any unrecognized issues with it.
Dec 04 2016
parent reply Dicebot <public dicebot.lv> writes:
 protected-headers="v1"
From: Dicebot <public dicebot.lv>
Newsgroups: d,i,g,i,t,a,l,m,a,r,s,.,D,.,i,n,t,e,r,n,a,l,s
Subject: Re: Regression control & breaking changes policies
References: <tiasmffqbwmqnejckemz forum.dlang.org>
 <o22epj$2nsg$1 digitalmars.com>
In-Reply-To: <o22epj$2nsg$1 digitalmars.com>

--QjH0iFKauRnQH98VTpSv7iOA4FwKTIcfi
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

On 12/05/2016 03:10 AM, Walter Bright wrote:

 The table makes sense and looks right. But there's a problem with the
 return scope changes. It's just too much to just wholesale deprecate
 things all at once.


It will need to happen at some point anyway. Without having those
deprecation implemented we can't even know how much existing code is
affected.


 People need time to ease into it, not be faced with
 a pile of diverse deprecation messages.


We can introduce `-preview=3DXXX` flag for that purpose but the key thing=

is that it still has to use deprecations and not errors for all scope
related changes. Otherwise you introduce dangerous disparity between
what gets previewed and what will eventually become the default.


 Having -transition=3Dsafe makes that practical, and also ensures time t=


o

 correct any unrecognized issues with it.


1) `-transition` flag is not for this purpose. You want this behavior -
please introduce new flag group, for example `-preview=3Dscope`. Desire i=
s
legit but you have to use a different flag for such purpose.

2) You must not use the very same flag for bug fixes as an excuse to not
bother of deprecations. https://github.com/dlang/dmd/pull/6290 is the
last one remaining now there was no justification to merge it in first
place.


--QjH0iFKauRnQH98VTpSv7iOA4FwKTIcfi--
Dec 06 2016
next sibling parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 12/6/2016 5:54 PM, Dicebot wrote:

 On 12/05/2016 03:10 AM, Walter Bright wrote:

 The table makes sense and looks right. But there's a problem with the
 return scope changes. It's just too much to just wholesale deprecate
 things all at once.


 It will need to happen at some point anyway. Without having those
 deprecation implemented we can't even know how much existing code is
 affected.


That's the point of the flag. It can be done on the user's timescale, not ours.



 People need time to ease into it, not be faced with
 a pile of diverse deprecation messages.


 We can introduce `-preview=XXX` flag for that purpose but the key thing
 is that it still has to use deprecations and not errors for all scope
 related changes. Otherwise you introduce dangerous disparity between
 what gets previewed and what will eventually become the default.


I agree, but it doesn't have to be deprecations until the flag becomes the
default.



 Having -transition=safe makes that practical, and also ensures time to
 correct any unrecognized issues with it.


 1) `-transition` flag is not for this purpose. You want this behavior -
 please introduce new flag group, for example `-preview=scope`. Desire is
 legit but you have to use a different flag for such purpose.


It originally was just `-safe`, but I got a lot of flak for that and pressure
to 
use `-transition`. I don't really care what the flag is. It just needs a flag.

I doubt users will really care either if it is -safe, -dip1000, 
-transition=safe, or -preview=safe.



 2) You must not use the very same flag for bug fixes as an excuse to not
 bother of deprecations. https://github.com/dlang/dmd/pull/6290 is the
 last one remaining now there was no justification to merge it in first
 place.


There isn't a hard line between what is a bug and what is not. It'd be nice if 
there was. Pretty much all of these fall under fixing unsafe code.
Dec 06 2016
next sibling parent reply Martin Nowak <code dawg.eu> writes:
On Wednesday, 7 December 2016 at 03:34:02 UTC, Walter Bright 
wrote:

 On 12/6/2016 5:54 PM, Dicebot wrote:

 On 12/05/2016 03:10 AM, Walter Bright wrote:

 The table makes sense and looks right. But there's a problem 
 with the
 return scope changes. It's just too much to just wholesale 
 deprecate
 things all at once.


 It will need to happen at some point anyway. Without having 
 those
 deprecation implemented we can't even know how much existing 
 code is
 affected.


 That's the point of the flag. It can be done on the user's 
 timescale, not ours.


Deprecations already provide for that. As a user you can delay 
updating a release for a short while, then you can still ignore 
or even silence deprecations until you have time to deal with 
them. That's the whole point of deprecations, they inform you 
than sth. is going to change and give you a timescale until when 
it needs to be fixed.
Hiding stuff behind a switch doesn't do much, but delaying the 
time until we inform people.
If you want provide a preview, we can build preview releases.


 People need time to ease into it, not be faced with
 a pile of diverse deprecation messages.






Maybe we could improve our deprecation output to classify the 
deprecation categories with some numbers, limit the amount of 
deprecations shown per category, and allow to silence specific 
deprecations.
The important difference, it's opt-out not opt-in, and doesn't 
add a pointless delay.

Also I doubt that anyone is using those switches.
Only found a 2 D user projects on github using DIP25.
https://github.com/search?l=Makefile&p=1&q=dip25&type=Code&utf8=✓
https://github.com/search?utf8=✓&q=dip25+extension:sdl+extension:json&type=Code&ref=advsearch&l=&l=


 We can introduce `-preview=XXX` flag for that purpose but the 
 key thing
 is that it still has to use deprecations and not errors for 
 all scope
 related changes. Otherwise you introduce dangerous disparity 
 between
 what gets previewed and what will eventually become the 
 default.


 I agree, but it doesn't have to be deprecations until the flag 
 becomes the default.


That's just extra confusing. Also it doesn't work for dub, where 
one user might want to use that feature but a dependent library 
isn't yet compatible,
Again deprecations work as designed here (as in any other 
language) and it remains unclear why you need something different.


 Having -transition=safe makes that practical, and also 
 ensures time to
 correct any unrecognized issues with it.






That's quality of implementation and our testing duty. Also since 
hardly anyone will test that switch, the main effect it has has 
is delaying the time until the issues pop up.


 1) `-transition` flag is not for this purpose. You want this 
 behavior -
 please introduce new flag group, for example `-preview=scope`. 
 Desire is
 legit but you have to use a different flag for such purpose.


 It originally was just `-safe`, but I got a lot of flak for 
 that and pressure to use `-transition`. I don't really care 
 what the flag is. It just needs a flag.

 I doubt users will really care either if it is -safe, -dip1000, 
 -transition=safe, or -preview=safe.


Well -preview is really what this is about, but I'm convinced 
we're better off to provide preview releases/build instead of 
merging unfinished features into master and the next release.
Dec 09 2016
parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 12/9/2016 11:32 PM, Martin Nowak wrote:

 On Wednesday, 7 December 2016 at 03:34:02 UTC, Walter Bright wrote:

 On 12/6/2016 5:54 PM, Dicebot wrote:

 On 12/05/2016 03:10 AM, Walter Bright wrote:

 The table makes sense and looks right. But there's a problem with the
 return scope changes. It's just too much to just wholesale deprecate
 things all at once.


 It will need to happen at some point anyway. Without having those
 deprecation implemented we can't even know how much existing code is
 affected.


 That's the point of the flag. It can be done on the user's timescale, not ours.


 Deprecations already provide for that. As a user you can delay updating a
 release for a short while, then you can still ignore or even silence
 deprecations until you have time to deal with them. That's the whole point of
 deprecations, they inform you than sth. is going to change and give you a
 timescale until when it needs to be fixed.
 Hiding stuff behind a switch doesn't do much, but delaying the time until we
 inform people.
 If you want provide a preview, we can build preview releases.


Then preview it is.



 People need time to ease into it, not be faced with
 a pile of diverse deprecation messages.






 Maybe we could improve our deprecation output to classify the deprecation
 categories with some numbers, limit the amount of deprecations shown per
 category, and allow to silence specific deprecations.
 The important difference, it's opt-out not opt-in, and doesn't add a pointless
 delay.


Then we're still faced with reverting the deprecations, as what happened.



 Also I doubt that anyone is using those switches.
 Only found a 2 D user projects on github using DIP25.
 https://github.com/search?l=Makefile&p=1&q=dip25&type=Code&utf8=✓
 https://github.com/search?utf8=✓&q=dip25+extension:sdl+extension:json&type=Code&ref=advsearch&l=&l=


That's disappointing, but ok. It will take a while for us to explain how it 
works and get acceptance of it. The threads here are abundant evidence for
that. 
But by getting it in, even under a switch, means we can make a credible case 
that D is a solution for memory safe programming, which is a huge deal.



 We can introduce `-preview=XXX` flag for that purpose but the key thing
 is that it still has to use deprecations and not errors for all scope
 related changes. Otherwise you introduce dangerous disparity between
 what gets previewed and what will eventually become the default.


 I agree, but it doesn't have to be deprecations until the flag becomes the
 default.


 That's just extra confusing. Also it doesn't work for dub, where one user might
 want to use that feature but a dependent library isn't yet compatible,
 Again deprecations work as designed here (as in any other language) and it
 remains unclear why you need something different.


See above.



 Having -transition=safe makes that practical, and also ensures time to
 correct any unrecognized issues with it.






 That's quality of implementation and our testing duty. Also since hardly anyone
 will test that switch, the main effect it has has is delaying the time until
the
 issues pop up.


True, but it's still the most practical way, given our large user base.



 Well -preview is really what this is about, but I'm convinced we're better off
 to provide preview releases/build instead of merging unfinished features into
 master and the next release.


It is finished. There are no known issues with it.
Dec 10 2016
parent reply Mathias Lang via Dlang-internal <dlang-internal puremagic.com> writes:
On Dec 10, 2016 12:00, "Walter Bright via Dlang-internal" <
dlang-internal puremagic.com> wrote:

On 12/9/2016 11:32 PM, Martin Nowak wrote:

Well -preview is really what this is about, but I'm convinced we're better

 off
 to provide preview releases/build instead of merging unfinished features
 into
 master and the next release.


It is finished. There are no known issues with it.


Since scope class are not testable, I wouldn't say it's finished.
Dec 10 2016
parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 12/10/2016 6:14 AM, Mathias Lang via Dlang-internal wrote:

 Since scope class are not testable, I wouldn't say it's finished.


Please be more specific.
Dec 11 2016
parent reply Mathias Lang <pro.mathias.lang gmail.com> writes:
On Monday, 12 December 2016 at 01:05:20 UTC, Walter Bright wrote:

 On 12/10/2016 6:14 AM, Mathias Lang via Dlang-internal wrote:

 Since scope class are not testable, I wouldn't say it's 
 finished.


 Please be more specific.


- Due to a regression, we cannot have `scope`-allocated classes 
in ` safe` code (https://github.com/dlang/dmd/pull/6279).
- By design, `scope` rules are not enforced outside of ` safe` 
code

Ergo, one part of the feature (classes, a fairly important one) 
cannot be tested.
Dec 11 2016
parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 12/11/2016 11:15 PM, Mathias Lang wrote:

 On Monday, 12 December 2016 at 01:05:20 UTC, Walter Bright wrote:

 On 12/10/2016 6:14 AM, Mathias Lang via Dlang-internal wrote:

 Since scope class are not testable, I wouldn't say it's finished.


 Please be more specific.


 - Due to a regression, we cannot have `scope`-allocated classes in ` safe` code
 (https://github.com/dlang/dmd/pull/6279).


That's both testable, and has been fixed.


 - By design, `scope` rules are not enforced outside of ` safe` code


Yes - many checks are not done outside of  safe code.


 Ergo, one part of the feature (classes, a fairly important one) cannot be
tested.


Since it's by design, I don't see why that makes it unfinished.
Dec 12 2016
parent Mathias Lang <mathias.lang sociomantic.com> writes:
On Monday, 12 December 2016 at 08:01:19 UTC, Walter Bright wrote:

 On 12/11/2016 11:15 PM, Mathias Lang wrote:

 On Monday, 12 December 2016 at 01:05:20 UTC, Walter Bright 
 wrote:

 On 12/10/2016 6:14 AM, Mathias Lang via Dlang-internal wrote:

 Since scope class are not testable, I wouldn't say it's 
 finished.


 Please be more specific.


 - Due to a regression, we cannot have `scope`-allocated 
 classes in ` safe` code
 (https://github.com/dlang/dmd/pull/6279).


 That's both testable, and has been fixed.


 - By design, `scope` rules are not enforced outside of ` safe` 
 code


 Yes - many checks are not done outside of  safe code.


 Ergo, one part of the feature (classes, a fairly important 
 one) cannot be tested.


 Since it's by design, I don't see why that makes it unfinished.


Let's move this discussion to the DIP1000-related thread: 
https://forum.dlang.org/post/iphydbpprkuceqprkxhb forum.dlang.org
Dec 12 2016
prev sibling parent Dicebot <public dicebot.lv> writes:
 protected-headers="v1"
From: Dicebot <public dicebot.lv>
Newsgroups: d,i,g,i,t,a,l,m,a,r,s,.,D,.,i,n,t,e,r,n,a,l,s
Subject: Re: Regression control & breaking changes policies
References: <tiasmffqbwmqnejckemz forum.dlang.org>
 <o22epj$2nsg$1 digitalmars.com> <o27q4c$ph0$1 digitalmars.com>
 <o27vv7$11ve$1 digitalmars.com>
In-Reply-To: <o27vv7$11ve$1 digitalmars.com>

--sFfPL45he2iEO6VS7lVEdt3whIdXGitJT
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

On 12/07/2016 05:34 AM, Walter Bright wrote:

 On 12/6/2016 5:54 PM, Dicebot wrote:

 On 12/05/2016 03:10 AM, Walter Bright wrote:

 The table makes sense and looks right. But there's a problem with the=







 return scope changes. It's just too much to just wholesale deprecate
 things all at once.


 It will need to happen at some point anyway. Without having those
 deprecation implemented we can't even know how much existing code is
 affected.


=20
 That's the point of the flag. It can be done on the user's timescale,
 not ours.


Flags don't allow for user timescale, only deprecations do. Flag can be
one of possible preview options but you can't start adjusting your
projects while it is in flag mode because that will break all downstream
projects that depend on yours (and don't want to enable the flag yet).


 People need time to ease into it, not be faced with
 a pile of diverse deprecation messages.


 We can introduce `-preview=3DXXX` flag for that purpose but the key th=




ing

 is that it still has to use deprecations and not errors for all scope
 related changes. Otherwise you introduce dangerous disparity between
 what gets previewed and what will eventually become the default.


=20
 I agree, but it doesn't have to be deprecations until the flag becomes
 the default.


Practice has shown that if it isn't deprecation from the very start, you
won't change it later and it will be me or Martin adjusting it in hurry
mode before release. I don't want that. For you as original implementor
there should be no difference in effort required between implementing
checks as deprecations or errors - as such, I can't find any excuse to
not do it from the very beginning.


 2) You must not use the very same flag for bug fixes as an excuse to n=




ot

 bother of deprecations. https://github.com/dlang/dmd/pull/6290 is the
 last one remaining now there was no justification to merge it in first=





 place.


=20
 There isn't a hard line between what is a bug and what is not. It'd be
 nice if there was. Pretty much all of these fall under fixing unsafe co=


de.

It is irrelevant, you have misread the statement. Breaking change
requires deprecations, no matter if it is a bug fix or not. You can add
additional helpful information via -transition flag but it still has to
be deprecation and using any kind of flag can't be an excuse to not do so=
=2E


--sFfPL45he2iEO6VS7lVEdt3whIdXGitJT--
Dec 11 2016
prev sibling parent reply Martin Nowak <code dawg.eu> writes:
On Wednesday, 7 December 2016 at 01:54:18 UTC, Dicebot wrote:

 Otherwise you introduce dangerous disparity


Please refrain from using exaggerating adjectives, they don't 
make your arguments better.
Dec 09 2016
parent Dicebot <public dicebot.lv> writes:
 protected-headers="v1"
From: Dicebot <public dicebot.lv>
Newsgroups: d,i,g,i,t,a,l,m,a,r,s,.,D,.,i,n,t,e,r,n,a,l,s
Subject: Re: Regression control & breaking changes policies
References: <tiasmffqbwmqnejckemz forum.dlang.org>
 <o22epj$2nsg$1 digitalmars.com> <o27q4c$ph0$1 digitalmars.com>
 <wbbzddxacvcdtpeqtrcn forum.dlang.org>
In-Reply-To: <wbbzddxacvcdtpeqtrcn forum.dlang.org>

--Kq6NIeT9IfUwUdf8XRHKsdlaTwm48mvcS
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

On 12/10/2016 09:40 AM, Martin Nowak wrote:

 On Wednesday, 7 December 2016 at 01:54:18 UTC, Dicebot wrote:

 Otherwise you introduce dangerous disparity


=20
 Please refrain from using exaggerating adjectives, they don't make your=



 arguments better.


There is no exaggeration here. I have said what I believe and mean,
quite literally. For feature branch to work you need to be able to merge
it into master with no changes once ready. Any tiny step sidewards from
that condition compromises master branch quality in a same way as
merging PR with broken tests and trying to fixup it later.


--Kq6NIeT9IfUwUdf8XRHKsdlaTwm48mvcS--
Dec 11 2016
prev sibling parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 12/4/2016 3:52 PM, Dicebot wrote:

     If any breaking change is detected in both master and stable that doesn't
     come at least with detailed explanation in the changelog, it has to be
     reverted unconditionally.

 This is a very simple and harmless way to improve on goals (1) and (2). I have
 noticed that reverting of merged code is often viewed as something damaging and
 even disrespectful - it isn't! By reverting a PR one immediately fixes the
 problem in relevant branch causing less trouble for other developers while
 providing more valuable information (and regression test case) to revive
 original PR. Compared to trying to fix the problem in place it also puts much
 less stress on developer as one can take all the time needed.


This is a rather blunt hammer and I don't see how it is harmless. Reverting it 
in stable or for a point release, ok. But in master? It may just require a
tweak 
or two. Consider that the PR may fix a severe problem, yet introduce a minor 
regression. Having an inviolate rule removes judgement from the process.
Dec 04 2016
parent reply Dicebot <public dicebot.lv> writes:
 protected-headers="v1"
From: Dicebot <public dicebot.lv>
Newsgroups: d,i,g,i,t,a,l,m,a,r,s,.,D,.,i,n,t,e,r,n,a,l,s
Subject: Re: Regression control & breaking changes policies
References: <tiasmffqbwmqnejckemz forum.dlang.org>
 <o22f3p$2o9u$1 digitalmars.com>
In-Reply-To: <o22f3p$2o9u$1 digitalmars.com>

--mDMU2ofcwKuLvulUhJJBf5iCs9wdDp4c6
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

On 12/05/2016 03:15 AM, Walter Bright wrote:

 This is a rather blunt hammer and I don't see how it is harmless.


It is harlmess by a simple criteria that I challenge you to give an
example of harm it causes. Reverting a regression fix is only case that
I can imagine reverting is worse than fixing.


 Reverting it in stable or for a point release, ok. But in master? It ma=


y

 just require a tweak or two. Consider that the PR may fix a severe
 problem, yet introduce a minor regression.


THERE IS NO SUCH THING AS MINOR REGRESSSION
THERE IS NO SUCH THING AS MINOR REGRESSSION
THERE IS NO SUCH THING AS MINOR REGRESSSION
THERE IS NO SUCH THING AS MINOR REGRESSSION
THERE IS NO SUCH THING AS MINOR REGRESSSION


 Having an inviolate rule
 removes judgement from the process.





--mDMU2ofcwKuLvulUhJJBf5iCs9wdDp4c6--
Dec 06 2016
next sibling parent reply Dicebot <public dicebot.lv> writes:
 protected-headers="v1"
From: Dicebot <public dicebot.lv>
Newsgroups: d,i,g,i,t,a,l,m,a,r,s,.,D,.,i,n,t,e,r,n,a,l,s
Subject: Re: Regression control & breaking changes policies
References: <tiasmffqbwmqnejckemz forum.dlang.org>
 <o22f3p$2o9u$1 digitalmars.com> <o27q80$ppn$1 digitalmars.com>
In-Reply-To: <o27q80$ppn$1 digitalmars.com>

--A5jJ05OXC1tAcCB9mFACtSHv8lgqNhSeC
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

On 12/07/2016 03:56 AM, Dicebot wrote:

 On 12/05/2016 03:15 AM, Walter Bright wrote:

 This is a rather blunt hammer and I don't see how it is harmless.


=20
 It is harlmess by a simple criteria that I challenge you to give an
 example of harm it causes. Reverting a regression fix is only case that=



 I can imagine reverting is worse than fixing.
=20

 Reverting it in stable or for a point release, ok. But in master? It m=




ay

 just require a tweak or two. Consider that the PR may fix a severe
 problem, yet introduce a minor regression.


=20
 THERE IS NO SUCH THING AS MINOR REGRESSSION
 THERE IS NO SUCH THING AS MINOR REGRESSSION
 THERE IS NO SUCH THING AS MINOR REGRESSSION
 THERE IS NO SUCH THING AS MINOR REGRESSSION
 THERE IS NO SUCH THING AS MINOR REGRESSSION


On a more serious note - not breaking things is much more important than
fixing things. Even if existing issue is severe, the fact that it exists
for a long time means users have a way to mitigate it. You can't make
things worse by delaying it for one release. One the other hand, any
issue that causes existing projects to break has sever consequence for
ecosystem usability.

I am perfectly fine with "any rule must have exceptions approach". But
there has to be at least some basic rule. Productivity of D development
suffers from decision reluctance horribly exactly because there is no
way to say if something non-trivial is OK to merge unless you come and
say so.

https://github.com/dlang/dmd/pull/6290 is perfect example of how absurd
it gets. There was a simple task of reverting bunch of prematurely
merged pull requests so that their implementation can be improved. Half
a days job at most. And yet one month later we are still arguing and
discussing and considering and doing nothing. How does it even surprise
you after that scope PR doesn't get enough attention?


--A5jJ05OXC1tAcCB9mFACtSHv8lgqNhSeC--
Dec 06 2016
parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 12/6/2016 6:03 PM, Dicebot wrote:

 On a more serious note - not breaking things is much more important than
 fixing things. Even if existing issue is severe, the fact that it exists
 for a long time means users have a way to mitigate it. You can't make
 things worse by delaying it for one release. One the other hand, any
 issue that causes existing projects to break has sever consequence for
 ecosystem usability.


On the other hand, we cannot progress the language by freezing it. After all, 
one transitions to newer versions of the language for good reason. Furthermore, 
bugs (especially safety problems) can be a silent liability for any code that 
has them.

So, yes, delaying these sorts of fixes can most definitely make things worse
for 
users.

This is why I push hard on these safety fixes. I want them fixed before they 
suddenly cost somebody on code they thought was fine.



 I am perfectly fine with "any rule must have exceptions approach". But
 there has to be at least some basic rule. Productivity of D development
 suffers from decision reluctance horribly exactly because there is no
 way to say if something non-trivial is OK to merge unless you come and
 say so.


I've approved a lot of PRs that turned out to cause regressions. I don't know
of 
any way to prove that PRs do not introduce regressions. At least we have a test 
suite that gets continuously added to - that helps enormously.



 https://github.com/dlang/dmd/pull/6290 is perfect example of how absurd
 it gets. There was a simple task of reverting bunch of prematurely
 merged pull requests so that their implementation can be improved. Half
 a days job at most. And yet one month later we are still arguing and
 discussing and considering and doing nothing. How does it even surprise
 you after that scope PR doesn't get enough attention?


What surprises me is the reluctance to put it behind a switch.


type test7.d


struct A
{
     ~this(){}
}

 safe struct B
{
     A a;
}


..\dmd test7 -transition=safe


test7.d(6): Error:  safe destructor 'test7.B.~this' cannot call  system 
destructor 'test7.A.~this'


..\dmd test7


test7.d(6): Error:  safe destructor 'test7.B.~this' cannot call  system 
destructor 'test7.A.~this'

This is with my version of the code, which includes all of the scope related 
changes. One of the difficulties I have with all these branches is what the 
state of the code is in each. Arbitrary reversions make this situation even 
worse, making it much harder for me to figure out what the right fix is.

BTW, one of the difficulties is the change that stopped -transition=safe from 
setting -dip25. All of the scope changes were built on top of the dip25 changes 
- zero testing was done on setting those flags independently.

There are no reported issues with any of the other cases that PR dealt with, so 
I don't feel that throwing the whole thing out is the best way forward. The
best 
way is to file bugzilla issues and take a look at if just a tweak would suffice.
-----------------------

The tangle of branches on this has reached a point where I suspect the best way 
forward is to simply abandon them, and I can submit a new PR that is the 
difference between master and what I have.
Dec 06 2016
next sibling parent reply Martin Nowak <code dawg.eu> writes:
On Wednesday, 7 December 2016 at 04:39:52 UTC, Walter Bright 
wrote:

 The tangle of branches on this has reached a point where I 
 suspect the best way forward is to simply abandon them, and I 
 can submit a new PR that is the difference between master and 
 what I have.


This didn't work optimally because it's a mixture of the old 
approach to incrementally merging unfinished features (now spread 
over stable/2.072 and master/2.073) and transitioning the major 
part of the scope work to a feature branch.
We now have to live with some safety related changes already 
being released, but there really isn't much confusing about the 
branches, other than you using -transition=safe and Dicebot 
changing everything to deprecations.
So let's just resolve that argument and fix whatever branch 
differs from the outcome.
Dec 09 2016
parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 12/9/2016 9:06 PM, Martin Nowak wrote:

 On Wednesday, 7 December 2016 at 04:39:52 UTC, Walter Bright wrote:

 The tangle of branches on this has reached a point where I suspect the best
 way forward is to simply abandon them, and I can submit a new PR that is the
 difference between master and what I have.


 This didn't work optimally because it's a mixture of the old approach to
 incrementally merging unfinished features (now spread over stable/2.072 and
 master/2.073) and transitioning the major part of the scope work to a feature
 branch.
 We now have to live with some safety related changes already being released,
but
 there really isn't much confusing about the branches, other than you using
 -transition=safe and Dicebot changing everything to deprecations.
 So let's just resolve that argument and fix whatever branch differs from the
 outcome.


The problem is I just don't know what is different between the branches,
master, 
and what I have. The confusion about which bugs are fixed in which branch has 
also come up.

With a new PR based on what I have, things become straightforward. I can ensure 
the bugzilla issues are fixed and the autotester passes it and it all works
with 
the latest Phobos.
Dec 10 2016
parent Martin Nowak <code dawg.eu> writes:
On Saturday, 10 December 2016 at 09:08:53 UTC, Walter Bright 
wrote:

 The problem is I just don't know what is different between the 
 branches, master, and what I have. The confusion about which 
 bugs are fixed in which branch has also come up.


Easy as pie, we can merge master into scope and you're up to date 
with everything.
A feature shouldn't have anything to do with master, the fact 
that some scope work was already released, indeed complicates the 
situation a bit.


 With a new PR based on what I have, things become 
 straightforward. I can ensure the bugzilla issues are fixed and 
 the autotester passes it and it all works with the latest 
 Phobos.


Please not, it creates the old deadlock that we cannot 
incrementally merge your work on that feature without risking a 
completely broken release. The only reason we could merge the 
first 2 chunks after a somewhat coarse review without extensive 
testing was the feature.

Please bear with me for a while, adopting a better git workflow 
will help us a lot to improve quality of releases and scale to 
more contributors.

Just a short summary of how feature branches are supposed to work 
atm.
This is not set in stone, so if you see improvements, please 
mention them.

- a feature is implemented in a single branch or multiple 
identically named branches across the dlang repos (dmd, druntime, 
phobos,...)
- testing and building is done using all of those branches
- the master branch is used as a fallback if one of the repos 
doesn't have a branch of the feature name
- Travis-CI, CyberShadow/DAutoTest, and the project tester 
(ci.dawg.eu) will work with feature branches, not sure if someone 
can manage to add support to the auto-tester
- once a feature is complete (implementation, docs, final 
testing+review), we merge the complete feature back into master, 
so it will be released soon after that
Dec 10 2016
prev sibling next sibling parent Martin Nowak <code dawg.eu> writes:
On Wednesday, 7 December 2016 at 04:39:52 UTC, Walter Bright 
wrote:

 I've approved a lot of PRs that turned out to cause 
 regressions. I don't know of any way to prove that PRs do not 
 introduce regressions. At least we have a test suite that gets 
 continuously added to - that helps enormously.


Well, about half of the released regression could have been found 
by a slightly more extensive test suite including other code, 
hence https://ci.dawg.eu/job/projects/ (used to be green with 
2.071.2).
Dec 09 2016
prev sibling parent Dicebot <public dicebot.lv> writes:
 protected-headers="v1"
From: Dicebot <public dicebot.lv>
Newsgroups: d,i,g,i,t,a,l,m,a,r,s,.,D,.,i,n,t,e,r,n,a,l,s
Subject: Re: Regression control & breaking changes policies
References: <tiasmffqbwmqnejckemz forum.dlang.org>
 <o22f3p$2o9u$1 digitalmars.com> <o27q80$ppn$1 digitalmars.com>
 <o27qm5$qh9$1 digitalmars.com> <o283ql$17b9$1 digitalmars.com>
In-Reply-To: <o283ql$17b9$1 digitalmars.com>

--5BT3hhWS47LOXpF5g9nQixaSI39vuq782
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

On 12/07/2016 06:39 AM, Walter Bright wrote:

 On 12/6/2016 6:03 PM, Dicebot wrote:

 On a more serious note - not breaking things is much more important th=




an

 fixing things. Even if existing issue is severe, the fact that it exis=




ts

 for a long time means users have a way to mitigate it. You can't make
 things worse by delaying it for one release. One the other hand, any
 issue that causes existing projects to break has sever consequence for=





 ecosystem usability.


=20
 On the other hand, we cannot progress the language by freezing it. Afte=


r

 all, one transitions to newer versions of the language for good reason.=



 Furthermore, bugs (especially safety problems) can be a silent liabilit=


y

 for any code that has them.
=20
 So, yes, delaying these sorts of fixes can most definitely make things
 worse for users.


We will get much better progress by slowing it down and improving
stability. It will be slower, but consistent. Right now D development
happens in a "1 step forward, 0.5 to 2 steps back" mode. Last two
releases were terrible.

Again - you can't do harm by reverting a problematic change. If problem
was not dire enough to make previous release unusable, it can always
wait for one release more. We can't scale to more contributors if
working with master branch requires knowledge of all half-broken changes
that were not reverted.


 This is why I push hard on these safety fixes. I want them fixed before=



 they suddenly cost somebody on code they thought was fine.


And that is why I oppose so hard to your way of doing things. By attempt
to fix bugs we don't care about you are willing to straight up fuck our
working code that we do care about. It doesn't matter how much you make
things better if you also make things worse.


 I am perfectly fine with "any rule must have exceptions approach". But=





 there has to be at least some basic rule. Productivity of D developmen=




t

 suffers from decision reluctance horribly exactly because there is no
 way to say if something non-trivial is OK to merge unless you come and=





 say so.


=20
 I've approved a lot of PRs that turned out to cause regressions. I don'=


t

 know of any way to prove that PRs do not introduce regressions. At leas=


t

 we have a test suite that gets continuously added to - that helps
 enormously.


Well here is PR which does introduce regression. It exists in master for
months. It blocks further progress with syncing scope branch and master.
And yet you oppose reverting it. What is the point? You complain about
slow progress with dip1000 but your are also the main person putting
obstacles to move forward with it.


--5BT3hhWS47LOXpF5g9nQixaSI39vuq782--
Dec 11 2016
prev sibling next sibling parent Walter Bright <newshound2 digitalmars.com> writes:
On 12/6/2016 5:56 PM, Dicebot wrote:

 It is harlmess by a simple criteria that I challenge you to give an
 example of harm it causes.


Let's say there's a bug report that XXX generates corrupt code (such as not 
initializing a variable). This may work most of the time, or appear to work,
but 
exhibit an error only rarely. But when it does fail, it could cause much 
disruption to the user's business.

Suppose that a fix is created, but comes with it a regression that some unusual 
(but correct) code is now broken.

We have the benefit of fixing a silent potentially disastrous problem, while 
introducing a problem that may be a nuisance, but will at least be obvious to 
the user.

In other words, if the bug fixed is costlier than the bug introduced, then that 
is an example of net harm in reverting.

Almost all changes/fixes bring with them at least *some* difference that 
*somebody* may depend on, however remote the change and however we might roll 
our eyes that somebody managed to depend on it. :-)

(For example, somebody might be doing hashes on the object file generated.)
Dec 06 2016
prev sibling next sibling parent Walter Bright <newshound2 digitalmars.com> writes:
On 12/6/2016 5:56 PM, Dicebot wrote:

 THERE IS NO SUCH THING AS MINOR REGRESSSION


BTW, my uncle was a research chemist for a pharmaceutical company in 
Switzerland. He told me long ago that there was no such thing as a drug without 
harmful side effects. All drugs have a "therapeutic index", which is the ratio 
of the good they do vs the harm. The researcher's job was to make that index as 
high as he could.

It's the same with any real world product. Nothing could be built if, for 
example, safety is a top priority that trumps all else. Of course, companies
and 
governments always say that safety is their top priority, but that's because 
they're forced to by the media who will excoriate them for saying anything else.

We do need processes and rules, but they should serve as guides for good 
judgement, not as substitutes for good judgement.

And that's where we need your help and judgement - in deciding what that 
"therapeutic index" is for the PRs on a case by case basis.
Dec 06 2016
prev sibling parent Sebastien Alaiwan <ace17 free.fr> writes:
On Wednesday, 7 December 2016 at 01:56:15 UTC, Dicebot wrote:

 THERE IS NO SUCH THING AS MINOR REGRESSSION
 THERE IS NO SUCH THING AS MINOR REGRESSSION
 THERE IS NO SUCH THING AS MINOR REGRESSSION
 THERE IS NO SUCH THING AS MINOR REGRESSSION
 THERE IS NO SUCH THING AS MINOR REGRESSSION


I couldn't agree more.

Compromising on "minor" regressions maintains the whole codebase 
in an unstable state, as there's no more guarantee that the 
feature I rely on today will not be "broken in a minor way" 
tomorrow to enable a "better" feature.

This is equivalent to disabling the tests that you've just broke 
to force your feature into the codebase (BTW allowing any kind of 
regression also make it a lot harder to guarantee that the 
codebase is actually improving).

Users have no global vision of the product, and if an update 
breaks their workflow, they will only see the regression. They 
won't be able to appreciate the fact that as a whole, the product 
might be better today. In one word, each time a compiler upgrade 
forces the users to modify their codebase, some of them leave 
(and god kills a kitten).

However, only simple policies stand the test of time ; if you 
want to establish a policy and need to argue over it, you've 
already lost the war.

The only regressions you might be able to forbid are the ones 
that are detected by an authoritative test suite that anyone can 
run before submitting patches, allowing to establish the 
brokenness of a feature in a non-ambiguous way.

It's a lot easier to enforce the authority of a test suite than a 
systematic-revert policy (with which I would totally agree!).
Dec 07 2016